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
Add nvtx.range() context manager #42925
Conversation
💊 CI failures summary and remediationsAs of commit 352a19e (more details on the Dr. CI page):
🚧 1 fixed upstream failure:These were probably caused by upstream breakages that were already fixed.
Please rebase on the
|
Hi. I see that the code review was approved. (Thanks!) Kinda new to PyTorch contributing. Is there anything else that's needed from my part to have this merged? I can't merge it myself ("base branch restricts merging to authorized users"), at least. Just making sure this change doesn't get lost... |
@peterjc123 @zhangguanheng66 This is my first PyTorch contribution, so I'm not sure what is or isn't a normal delay between pull request approval and merging. Is it one of you folks that merges this? Just making sure this hasn't slipped through the cracks... Thanks! |
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.
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
The context manager `torch.cuda.nvtx.range` has been around for about 4 years (see #42925). Unfortunately, it was never documented and as a consequence users are just unaware of it (see #121663). Pull Request resolved: #121699 Approved by: https://github.com/janeyx99
Small quality-of-life improvement to NVTX Python bindings, that we're using internally and that would be useful to other folks using NVTX annotations via PyTorch. (And my first potential PyTorch contribution.)
Instead of needing to be careful with try/finally to make sure all your range_push'es are range_pop'ed:
you can simply do:
or even use it as a decorator:
A couple small open questions:
I also added the ability to call
msg.format()
insiderange()
, with the intention that, if there is nothing listening to NVTX events, we should skip the string formatting, to lower the overhead in that case. If you like that idea, I could add the actual "skip string formatting if nobody is listening to events" parts. We can also just leave it as is. Or I can remove that if you folks don't like it. (In the first two cases, should we add that torange_push()
andmark()
too?) Just let me know which one it is, and I'll update the pull request.I don't think there are many places for bugs to hide in that function, but I can certainly add a quick test, if you folks want.