CT-525 added profile.py for local file system profiling#203
CT-525 added profile.py for local file system profiling#203lawschlosser wants to merge 3 commits intomasterfrom
Conversation
flebel
left a comment
There was a problem hiding this comment.
LGTM with a few suggestions.
tests/profile.py
Outdated
tests/profile.py
Outdated
There was a problem hiding this comment.
The option flags should be prefixed by --, not -.
There was a problem hiding this comment.
on/off should be True/False.
tests/profile.py
Outdated
There was a problem hiding this comment.
This should be --csv_filepath /path/to/output_file.csv.
tests/profile.py
Outdated
There was a problem hiding this comment.
Parenthesis aren't needed here.
| use_api_key=True | ||
| ) | ||
|
|
||
| jobs = json.loads(r_body).get("data") |
There was a problem hiding this comment.
client.make_request returns a requests object, you should be able to parse its payload as JSON with r_body.get_json().get('data').
There was a problem hiding this comment.
This comment also applies to the other use of json.loads in this script.
There was a problem hiding this comment.
client.make_request does not return a response object (sadly). Only text (response.text)
tests/profile.py
Outdated
There was a problem hiding this comment.
How does argparse behave if accessing a property for an argument that doesn't exist?
Since the --xxhash flag isn't enabled if XXHASH if False, I'm wondering if an exception would be raised if this code was run on a systems that's missing the xxhash module.
There was a problem hiding this comment.
If you expand that statement to use multiple lines, you can more easily see why it's not problematic, e.g,
if XXHASH:
hash_xx=args.xxhash
else:
hash_xx=None
In other words, the arg.xxhash value will not be accessed if xxhash is not installed.
tests/profile.py
Outdated
There was a problem hiding this comment.
Can't we just use the default values defined for every argument? It feels redundant to redefine a second set of defaults here.
There was a problem hiding this comment.
This function was designed to stand on its own, and therefore provide its own default values. The fact that our shell/argparse logic happens to call it is irrelevant and should not dictate whether this function should define its own default arguments IMO. For example, if I wanted to import/reuse this function elsewhere, I would want this function to provide useful default arguments.
tests/profile.py
Outdated
There was a problem hiding this comment.
Can you leave a comment explaining what this is for?
There was a problem hiding this comment.
yep, good call.
tests/profile.py
Outdated
There was a problem hiding this comment.
files is already initiated above.
ccbd2d3 to
32382c2
Compare
70e65a3 to
eeb8e43
Compare
- also added device benchmarking option - also changed all file-reading operations to use io.open
eeb8e43 to
de1b6bd
Compare
- improve general cross-platform support - now flushes cache once per test run (not per file)
35ebf58 to
c9a4213
Compare
Usage
Profile files from a specific job
./profile.py job 00007Profile files from a specific directory
./profile.py dirs ~or multiple directories
./profile.py dirs ~ /tmp /optWrite verbose results to csv
/profile.py job 00007 --csv_path /tmp/profile_00007.csvUse multiple threads
/profile.py job 00007 --threads 4Example Output
Summary console output
Verbose CSV output
Option flags