Fixed torch profiler API changes in python benchmarks#2139
Conversation
|
!build --pybench |
Priya2698
left a comment
There was a problem hiding this comment.
Thanks for updating!
Do we need to have different API based on torch version? In this PR: pytorch/pytorch#123247, I don't see any version update though, so this should be sufficient.
| sum( | ||
| [ | ||
| event.self_cuda_time_total | ||
| event.self_device_time_total |
There was a problem hiding this comment.
I think you'll need to guard this with NVF_TORCH_VERSION_GREATER. Otherwise we won't be able to build with stable torch release
There was a problem hiding this comment.
wait... this is python. wasn't paying attention to that.
But how did it work through CI? we did test against stable torch version right. Is benchmark not part of CI on older torch version?
There was a problem hiding this comment.
For torch stable build & test in CI, we run python tests but not python benchmarks.
There was a problem hiding this comment.
Should we add the check for torch version then?
There was a problem hiding this comment.
I think we should. Otherwise we won't be able to run benchmark against old torch version.
There was a problem hiding this comment.
torch==2.3 is still using self_cuda_time_total. Will add a version guard for torch 2.4.
|
!build --pybench |
Is this running benchmarking? Otherwise this run won't really check this API usage |
It runs pytest with |
|
!build |
This API change will be exercised only if we enable benchmarking and not during validation which is the case on the CI. Generally, we don't need to benchmark on the CI but PRs like these cannot be tested since the torch.profiler will not be used. |
Priya2698
left a comment
There was a problem hiding this comment.
This should avoid API failures for now, although seems like this PR was closed: pytorch/pytorch#123247. We can revisit and add torch version guards instead of attribute checks once the torch API is final.
Just to clarify that we shouldn't need to revisit this unless upstream renames the API to something else. That upstream PR is merged and Xiao's patch looks pretty solid to me. There's no need to version guard anything, since attribute query should work just fine. |
Re: pytorch/pytorch#123247
This PR changed
self_cuda_time_totaltoself_device_time_totalin API calls.