Repository navigation
feat(xpu): monkeypatch torch.profiler for XPU activity, no core hooks - #10
Merged
Merged
Conversation
Adds torch_profile_xpu.py, mirroring the existing MLU pattern (torch_profile_mlu.py): monkeypatches TorchProfilerToolConfig to accept "xpu" in profiler.tool_config.torch.contents, and wraps get_torch_profiler to collect torch.profiler.ProfilerActivity.XPU when requested. Verified on real B60 hardware: a torch.profiler run with contents=[xpu,cpu] produces a populated Chrome trace containing genuine Intel Level Zero driver activity (xpu_driver/xpu_runtime categories, zeDeviceGetRootDevice etc.). Replaces the PlatformXPU.torch_profiler_activity()/ torch_profiler_content_name() approach from #6, which depended on new PlatformBase hooks proposed in verl-project/verl#7988. Per the maintainer's ruling on #26, the default should be plugin-side monkeypatch, not new verl-core abstraction, and this feature has no dispatch point that requires a core change -- so no core PR is needed.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical profiler API and MLU/XPU patch conflicts, plus additional trace activity and path issues, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (6)
What changed in this PR
Adds plugin-side XPU support for torch.profiler without modifying verl-core.
Changes:
- Adds XPU profiler validation and activity collection.
- Registers XPU profiler patches alongside MLU support.
- Removes reliance on core platform hooks.
| File | Summary |
|---|---|
verl_hardware_plugin/profilers/torch_profile_xpu.py |
Implements XPU profiler monkeypatches; unresolved API compatibility, activity selection, path handling, and device-combination issues remain. |
verl_hardware_plugin/profilers/__init__.py |
Registers XPU support, but currently overwrites the MLU monkeypatches instead of composing or selecting backend patches. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot review on #10 caught real bugs: the previous version fully replaced TorchProfilerToolConfig.__post_init__ and get_torch_profiler rather than wrapping them, so it silently clobbered the MLU patch (rejecting contents=["mlu"], dropping MLU activity) whenever both patches were applied in the same process. It also hard-coded an older six-argument get_torch_profiler signature that verl-core has since outgrown (profile_step, name_mini_batch_window), reintroduced a save_path/role directory nesting that verl-core deliberately flattened, made CPU activity conditional on contents instead of always-on, and had an if/elif that dropped CUDA whenever XPU was also requested. Rewritten to wrap whatever __post_init__/get_torch_profiler is currently installed instead of replacing it: strip "xpu" before delegating validation (so any other vendor's allowed content names still validate correctly, in either registration order), and append ProfilerActivity.XPU to the returned profile's activities after calling through. This can never drift from verl-core's current signature/behavior since it forwards everything unchanged, and it can't clobber another vendor's patch since it never replaces __post_init__/get_torch_profiler outright.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #6 with a plugin-side monkeypatch instead of new
verl-corePlatformBasehooks.Motivation
#6 implements
PlatformXPU.torch_profiler_activity()/torch_profiler_content_name(), paired with a companion core PR (verl-project/verl#7988) that adds those two hooks toPlatformBaseplus wiring inverl/utils/profiler/config.pyandtorch_profile.py.Per the maintainer's ruling on
verl-hardware-plugin#26, the default should be plugin-side monkeypatch, not newverl-coreabstraction — a core PR is only warranted for a genuine dispatch-point blocker (like the profiler-config-nulled-before-a-plugin-can-react case). This feature has no such blocker:TorchProfilerToolConfig.__post_init__andget_torch_profilerare both plain, patchable functions.Changes
verl_hardware_plugin/profilers/torch_profile_xpu.py, mirroring the existing MLU pattern (torch_profile_mlu.py):TorchProfilerToolConfig.__post_init__to accept"xpu"inprofiler.tool_config.torch.contents.get_torch_profilerto collecttorch.profiler.ProfilerActivity.XPUwhen"xpu"is requested.profilers/__init__.py::register_all_profiles()alongside the MLU registration.PlatformXPUhook methods added in feat(xpu): add torch.profiler activity hooks for XPU #6 — no longer needed.verl-corechange required; [hardware, profile] add optional torch.profiler activity hook to Plat… verl-project/verl#7988 is not needed for this to work.Testing
Verified on real B60 hardware: a
torch.profilerrun withcontents=[xpu,cpu]produces a populated Chrome trace containing genuine Intel Level Zero driver activity (xpu_driver/xpu_runtimecategories,zeDeviceGetRootDeviceetc.) -- same verification as #6.