-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[AUTOTUNER] adding simple report flag for autotuner runs #3411
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
Conversation
|
I'm OK with the change. Should it be a log or just print? Triton doesn't have a logging system though |
jlebar
left a comment
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 think this is a good idea!
python/triton/runtime/autotuner.py
Outdated
| :type warmup: int | ||
| :param rep: Repetition time (in ms) to pass to benchmarking, defaults to 100. | ||
| :type rep: int | ||
| :param report: Flag to enable printing the selected configuration |
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.
Rename the flag to print? "report" can be a noun or a verb, and that ambiguity is confusing here. (Do I pass the report in as a parameter? What is the report? Or is the autotuning report passed as an outparameter somehow?)
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.
ok, changed it to print_autotune_stats
python/triton/runtime/autotuner.py
Outdated
| :type warmup: int | ||
| :param rep: Repetition time (in ms) to pass to benchmarking, defaults to 100. | ||
| :type rep: int | ||
| :param report: Flag to enable printing the selected configuration |
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.
Nit: Reword to something like what you wrote in the commit message, which is more helpful. For example: "If print is true, Triton will print a log message each time it autotunes a function."
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.
Fair point, I updated it.
|
I wonder why this can't be done by using a profiler? |
Indeed this is a case I used the profiler a lot...as proton can rename kernels based on constants. The problem is probably because proton is not available yet... |
|
Personally I think something lightweight like this is nice to have even if we have heavier-weight like Proton. It's basically zero complexity overhead and can be really useful for quick-and-dirty debugging. |
I think it also depends on whether you want to check tuning time on CPU + GPU or just GPU time. This PR seems to get end to end tuning time. |
python/triton/runtime/autotuner.py
Outdated
| if self.report and not used_cached_result: | ||
| autotune_stop = time.time() | ||
| print( | ||
| f"Autotuner for function {self.fn} finished after {autotune_stop-autotune_start:.2f}s; best config selected: {self.best_config};" |
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.
Can we use self.bench_time?
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.
Good point, the hook won't make such a difference. I changed it
4731aad to
7d8b4b6
Compare
|
Thanks for your helpful comments!
Exactly, we were/are interested in the end-to-end time of the autotuner, so that we could tell easily if a variance in latency of our application was caused by the triton autotuner or smth else. |
I was also thinking if besides |
Hi @ThomasRaoux , since they are interested in end-to-end statistics. It might be fine to print some debugging information? What's your thought? |
|
Yeah I think this can be helpful to some people and really doesn't add much complexity since |
|
Having debug logs makes sense, do we want this to be a front end option or an env variable? In general debug features are controlled by env variables. |
|
I'm fine with either an envvar or the in-code flag. It sound like we'd have consensus if we went with the env var? I propose |
|
I agree, controlling the logging via an environment variable serves the purpose better. |
2918810 to
927f09f
Compare
jlebar
left a comment
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.
Thanks!
|
I can merge this once we make the final few remaining changes! |
927f09f to
23308be
Compare
|
Rebased and am trying to merge this. |
|
Thanks @jlebar! I was on a trip and didn't had the time to fix/react to your comments. |
For our application, we wanted to investigate when, how often, and for how long the triton autotuner is triggered (to asses the impact on the latency of the application).
Therefore, we implemented a simple report flag to the autotuner decorator:
If this flag is set to true, the autotuner will print the following statement, every time a new autotune run is triggered:
There are no prints if a cached configuration is used.
(We thought this could be also a helpful feature for others, therefore we created this PR directly and we can make/discuss requested changes here. However, if you think this should be discussed instead in an issue, let us know and sorry).