Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[OpPerf] Fixed Python profiler bug #17642

Merged
merged 3 commits into from
Feb 26, 2020
Merged

[OpPerf] Fixed Python profiler bug #17642

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db46553
Changed arg structure in op func call
connorgoggins Feb 20, 2020
9cbf836
Length check to prevent index out of bounds error
connorgoggins Feb 21, 2020
850260a
Dropping args[2] as it is no longer used (only using kwargs)
connorgoggins Feb 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmark/opperf/utils/profiler_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ def python_profile(func):
@functools.wraps(func)
def python_profile_it(*args, **kwargs):
runs = args[1]
modified_args = (args[0], 1, args[2])
Copy link
Contributor

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?

Copy link
Contributor Author

@connorgoggins connorgoggins Feb 21, 2020 •
edited
Loading

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).

Copy link
Contributor

@ChaiBapchya ChaiBapchya Feb 21, 2020 •
edited
Loading

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

  1. we copy the number of runs in a variable in run and then run it that many number of times
  2. For each run, we use python time it function to time it and then take average, mean, max, etc values for each of those individual python time runs.

Makes sense? @apeforest

Copy link
Contributor

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?

Copy link
Contributor

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.

modified_args = (args[0], 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it looks good!

times = []

for _ in range(runs):
Expand Down