Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance Worker Coordinator Async Profiler #710
base: main
Are you sure you want to change the base?
Enhance Worker Coordinator Async Profiler #710
Changes from all commits
7708d5b
b173589
e047385
8642079
c21b420
63de8c2
f47d91e
234b514
334b6d2
495a3fc
756916e
89fb8d9
3e184a2
345e30d
4125111
c29ff67
9e0aaa5
512d052
e097528
7b0bfee
80d2adb
839ac6a
621ecd0
6451068
0c6b568
a297e83
5c276fb
1d61375
6ba7113
943e8aa
1f07e7f
06f1c77
5521e4a
51daee8
b7511c3
3a8d1ae
e668c98
7268568
823f14d
d92842d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May help to add a check to warn if this argument is used but profiling is not enabled.
Also, does it make sense to provide an option for sort order (asc/desc)?
Finally, it may be useful to be able to specify only specific tasks for profiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rishabh made a call out earlier in this PR that suggests moving the responsibility of checking if sort types exists to the argparser. This is doable and requires adding more classes and leveraging the
action
field in the arguments. This does add more complexity to benchmark.py though.I initially wanted to minimize the number of arguments for AsyncProfiler since it exists in the
execute-test
parser, which already has a lot of arguments. I could look to incorporate it still.This is a fair call out but similar to the question above, I'm concerned about the number of arguments in
execute-test
and wonder if users will be able to spot that these are for the AsyncProfiler. Still, both of these are arguments that users would actually want. Can aim to have this as a draft and see if there's other ways we can achieve this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enforce this check while accepting the params.
e.g.
This way argparser will throw exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would work if
--enable-worker-coordinator-profiling
was required inexecute-test
but it's currently optional.--sort-type
is not needed unless the user wants to specify the a column for the Yappi library to sort on.Looking at the argparse documentation, we'll need to create additional classes that inherit from
argparse.Actions
and some additional if statements inbenchmark.py
to enforce Argparser to throw the exception, which requires new changes as oppose to this if statement within the AsyncProfiler. @rishabh6788 Let me know if you have other thoughts on this?If we find an approach with argparse that we like and doesn't add too much to the existing benchmark.py, I can open a separate PR to address this as well so that there is less changes per PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even when you set a default value if flag not provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more test to verify any other sort type option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can but this test is more for seeing if the wrapper is called