This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
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.
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 don't remember the reason why we need to write this way in the first place. @ChaiBapchya could you please review. @connorgoggins Have you run through existing tests to make sure this does not break any existing usage?
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.
@apeforest yes, ran full OpPerf suite with Python profiler with this change and everything passed (see results here).
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.
So the args that are passed to this function are
args[0] = op
args[1] = warmup / runs (number of times to run for warmup or number of times to run)
args[2] - rest of the args
https://github.com/apache/incubator-mxnet/blob/9dcf71d8fe33f77ed316a95fcffaf1f7f883ff70/benchmark/opperf/utils/benchmark_utils.py#L114
https://github.com/apache/incubator-mxnet/blob/9dcf71d8fe33f77ed316a95fcffaf1f7f883ff70/benchmark/opperf/utils/benchmark_utils.py#L121
The way it worked for native MXNet CPP profiler is that you could pass the runs (and it would capture the time for each value along with mean/max, etc)
But for Python's time it function, we had to manually run the
for loop
for the number of runs.So that's what I did there
Makes sense? @apeforest
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.
@ChaiBapchya So basically you are saying we don't need to do this modified_args for python profiler, right? So @connorgoggins change is valid?
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.
We do need to modify the args to meet the requirement for capturing per run timing info using python's time_it function. This needs to handled in a way that doesn't break existing native profiler.