[plugin] oot torch profiler activity support - #31580
Conversation
extends device mixin and and profiler utilities to accept oot torch profiler activities Signed-off-by: Devashish Lal <devcode@fb.com>
|
/tag-and-rerun-ci |
There was a problem hiding this comment.
Code Review
This pull request introduces support for out-of-tree (OOT) platforms in the profiler by adding get_torch_profiler_activity_str and get_torch_profiler_activity methods to DeviceMixin, and updating the profiler manager and utilities to conditionally load these custom activities. The review feedback suggests returning None instead of raising NotImplementedError by default to avoid forcing all OOT platforms to implement these methods. Additionally, the feedback points out several potential runtime issues (such as TypeError or dictionary key errors) if these methods return None, and provides code suggestions to safely handle optional values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def get_torch_profiler_activity_str(self) -> str: | ||
| """[Planned] Return the torch profiler activity string.""" | ||
| raise NotImplementedError | ||
|
|
||
| def get_torch_profiler_activity(self) -> torch.profiler.ProfilerActivity: | ||
| """[Planned] Return the torch profiler activity.""" | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Instead of raising NotImplementedError by default, it is safer to return None (using Optional). This prevents out-of-tree (OOT) platforms that do not require custom profiler activities from being forced to implement these methods, avoiding potential runtime crashes.
| def get_torch_profiler_activity_str(self) -> str: | |
| """[Planned] Return the torch profiler activity string.""" | |
| raise NotImplementedError | |
| def get_torch_profiler_activity(self) -> torch.profiler.ProfilerActivity: | |
| """[Planned] Return the torch profiler activity.""" | |
| raise NotImplementedError | |
| def get_torch_profiler_activity_str(self) -> Optional[str]: | |
| """[Planned] Return the torch profiler activity string.""" | |
| return None | |
| def get_torch_profiler_activity(self) -> Optional[torch.profiler.ProfilerActivity]: | |
| """[Planned] Return the torch profiler activity.""" | |
| return None |
| if current_platform.is_out_of_tree(): | ||
| if hasattr( | ||
| torch.profiler.ProfilerActivity, | ||
| current_platform.get_torch_profiler_activity_str(), | ||
| ): | ||
| activity_map[current_platform.get_torch_profiler_activity_str()] = ( | ||
| current_platform.get_torch_profiler_activity() | ||
| ) |
There was a problem hiding this comment.
If get_torch_profiler_activity_str() returns None (or if it is not implemented), calling hasattr(torch.profiler.ProfilerActivity, None) will raise a TypeError. We should retrieve the values first and check if they are not None before checking attributes and updating the map.
| if current_platform.is_out_of_tree(): | |
| if hasattr( | |
| torch.profiler.ProfilerActivity, | |
| current_platform.get_torch_profiler_activity_str(), | |
| ): | |
| activity_map[current_platform.get_torch_profiler_activity_str()] = ( | |
| current_platform.get_torch_profiler_activity() | |
| ) | |
| if current_platform.is_out_of_tree(): | |
| activity_str = current_platform.get_torch_profiler_activity_str() | |
| activity = current_platform.get_torch_profiler_activity() | |
| if activity_str and activity and hasattr(torch.profiler.ProfilerActivity, activity_str): | |
| activity_map[activity_str] = activity |
| if current_platform.is_out_of_tree(): | ||
| if current_platform.get_torch_profiler_activity_str() in activities: | ||
| inners.append( | ||
| _ProfilerTorch( | ||
| **kwargs, | ||
| activities=activities, | ||
| with_stack=with_stack, | ||
| record_shapes=record_shapes, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
To prevent potential TypeError or issues when get_torch_profiler_activity_str() returns None, we should check that the returned activity string is not None before checking if it is in activities.
| if current_platform.is_out_of_tree(): | |
| if current_platform.get_torch_profiler_activity_str() in activities: | |
| inners.append( | |
| _ProfilerTorch( | |
| **kwargs, | |
| activities=activities, | |
| with_stack=with_stack, | |
| record_shapes=record_shapes, | |
| ) | |
| ) | |
| if current_platform.is_out_of_tree(): | |
| activity_str = current_platform.get_torch_profiler_activity_str() | |
| if activity_str and activity_str in activities: | |
| inners.append( | |
| _ProfilerTorch( | |
| **kwargs, | |
| activities=activities, | |
| with_stack=with_stack, | |
| record_shapes=record_shapes, | |
| ) | |
| ) |
| if current_platform.is_out_of_tree(): | ||
| activity_map[current_platform.get_torch_profiler_activity_str()] = ( | ||
| current_platform.get_torch_profiler_activity() | ||
| ) |
There was a problem hiding this comment.
If get_torch_profiler_activity_str() returns None, this will set activity_map[None] = None, which can cause issues later. We should check that both the activity string and the activity itself are not None before adding them to the map.
| if current_platform.is_out_of_tree(): | |
| activity_map[current_platform.get_torch_profiler_activity_str()] = ( | |
| current_platform.get_torch_profiler_activity() | |
| ) | |
| if current_platform.is_out_of_tree(): | |
| activity_str = current_platform.get_torch_profiler_activity_str() | |
| activity = current_platform.get_torch_profiler_activity() | |
| if activity_str and activity: | |
| activity_map[activity_str] = activity |
|
/tag-and-rerun-ci |
|
/rerun-failed-ci |
Signed-off-by: Devashish Lal <devcode@fb.com> Co-authored-by: Devashish Lal <devcode@fb.com>
Signed-off-by: Devashish Lal <devcode@fb.com> Co-authored-by: Devashish Lal <devcode@fb.com>
Signed-off-by: Devashish Lal <devcode@fb.com> Co-authored-by: Devashish Lal <devcode@fb.com>
Motivation
Currently integrating HW specific torch profile activities required in tree changes, which can easily be driven by the device mixin from the platform plugins system
Modifications
extends device mixin and and profiler utilities
to accept oot torch profiler activities
Accuracy Tests
Speed Tests and Profiling
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ❌ Run #29622622859
Latest PR Test (Extra): ❌ Run #29622622748