Add toggleCollectionDynamic for XPUPTI #1088
Conversation
|
Hi @frost-intel! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
@zejun-chen Could you provide any feedback? |
The assumption makes sense because when HAS_XPUPTI=true, only xpu device is active i think, so you can cast the base class to xpu class. |
It looks like there isn't any test coverage for this feature in kineto, but I'll edit test_profiler.py in pytorch to cover the XPU case when adding the toggle functionality. Update: Tests for XPU toggling have been added in pytorch/pytorch#155135 |
|
@sraikund16 Could you please review this PR? |
| @@ -1209,6 +1212,12 @@ void CuptiActivityProfiler::toggleCollectionDynamic(const bool enable) { | |||
| cupti_.disableActivities(derivedConfig_->profileActivityTypes()); | |||
| } | |||
| #endif | |||
| #ifdef HAS_XPUPTI | |||
| for (auto& session : sessions_) { | |||
There was a problem hiding this comment.
Can you leave a comment here explaining that you are making the assumption that all sessions will be XPUPTI since there are no downstream profilers for it? I think a TODO to determine session type is a good idea
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@frost-intel has updated the pull request. You must reimport the pull request before landing. |
|
@sraikund16 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@sraikund16 merged this pull request in 7fd7d56. |
This PR adds support for toggleCollectionDynamic for Xpupti.
Since Xpupti enables/disables activities based on the session rather than the profiler instance, I cast the
IActivityProfilerSessionto anXpuptiActivitySession. This assumes that all sessions are XpuptiActivitySessions. In upstream Pytorch, the design rule for now is that there is no simultaneous support for XPU and any other device (CUDA/Roc), so this assumption should be safe?