Repository navigation
experiment: pure-monkeypatch XPU integration, zero PlatformBase hooks - #8
Conversation
Answers verl-hardware-plugin#26's own question concretely: implements attention_utils_module, is_reduce_avg_supported, and dist_profiler_cls (vtune) entirely inside this plugin, with no dependency on verl-project/verl#7917 ever merging. - attention_utils_module: patch attention_utils._get_attention_functions (re-invoked every call, so import order doesn't matter). - is_reduce_avg_supported (FSDP2): no patch needed at all -- extended fsdp_xpu.py's existing post-initialize() workaround to cover every FSDP2-wrapped submodule, not just the root. - is_reduce_avg_supported (3 raw dist.all_reduce(AVG) sites): patch torch.distributed.all_reduce process-wide. Flagged as a real design smell in the module docstring, not presented as clean. - dist_profiler_cls: patch DistProfiler.__init__ (the class method, not the class name) to add a vtune branch. profiler_markers and the rollout-server profiler allowlist are deliberately NOT patched here -- both are genuine blockers (import-time module selection / value nulled and consumed in the same method body), not solved by patching harder. See docs/design/xpu-monkeypatch-experiment.md for the full verdict table and why an earlier draft of this plan (wrapping apply_fsdp2 directly) had the same import-order problem as profiler_markers and was replaced with the fsdp_xpu.py approach instead. Does not touch feature/xpu-vtune-avg-attention (PR #22) -- this is a separate branch from main so the two approaches can be compared directly.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved correctness and safety issues remain in FSDP initialization, patch guarding, exception cleanup, and profiler labeling.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (4)
What changed in this PR
This PR adds a plugin-only Intel XPU integration using monkeypatches and FSDP extensions, without requiring core PlatformBase changes.
Changes:
- Adds attention, AVG reduction, and VTune profiler patches.
- Extends nested FSDP2 reduction handling.
- Documents supported capabilities and remaining blockers.
| File | Summary |
|---|---|
verl_hardware_plugin/profilers/itt_profile_xpu.py |
Adds VTune/ITT profiling support. |
verl_hardware_plugin/patches/reduce_avg_allreduce_patch_xpu.py |
Adds the XPU AVG reduction workaround. |
verl_hardware_plugin/patches/dist_profiler_patch_xpu.py |
Adds VTune profiler dispatch. |
verl_hardware_plugin/patches/attention_patch_xpu.py |
Routes XPU attention utilities. |
verl_hardware_plugin/patches/__init__.py |
Applies plugin patches. |
verl_hardware_plugin/engines/fsdp_xpu.py |
Configures nested FSDP2 reduction handling. |
verl_hardware_plugin/__init__.py |
Initializes the patches. |
docs/design/xpu-monkeypatch-experiment.md |
Documents the experiment and limitations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if hasattr(self.model, "set_force_sum_reduction_for_comms"): | ||
| self.model.set_force_sum_reduction_for_comms(True) | ||
| logger.info("Enabled force_sum_reduction_for_comms for XPU") | ||
| _force_sum_reduction_on_all_fsdp_modules(self.module) |
There was a problem hiding this comment.
Checked this against verl-core rather than the plugin docs, and it is inverted: self.module is correct and self.model is the bug.
FSDPEngine assigns the wrapped model in _build_model_optimizer() (called from initialize()), and grep -rn "self\.model\s*=" verl/workers/engine/ returns nothing anywhere in that tree — the only self.model* attribute on these engines is self.model_config. So the previous hasattr(self.model, "set_force_sum_reduction_for_comms") guard was silently always False, which is exactly why the xccl workaround never actually ran. Switching to self.module is what makes it fire.
You cited docs/development.md:583-586 as the contract, and that was the right thing to point at — the doc was the wrong artifact. Fixed all 7 occurrences there and stated the attribute contract explicitly in the template docstring so it does not get reintroduced. Added tests/test_fsdp_xpu_engine.py::test_verl_fsdp_engine_exposes_module_not_model, which inspects the real FSDPEngine class source, so a future verl rename fails a test instead of silently disabling the workaround again.
Verified on 2×B60 via devctl: the workaround now reports enabling force_sum_reduction_for_comms on the root plus every per-layer FSDP2 wrapper.
| try: | ||
| module.apply() | ||
| except Exception as e: | ||
| logger.debug("%s.apply() skipped: %s", module.__name__, e) |
There was a problem hiding this comment.
Agreed, fixed. apply_all() now splits failures by consequence instead of swallowing them:
- non-XPU host: nothing was going to install anyway (every module guards), so still debug-only, no raise;
- XPU host, correctness-critical patch (
reduce_avg_allreduce_patch_xpu,attention_patch_xpu): re-raised asRuntimeError— a missing reduce workaround means wrong gradients, which is precisely the silent-until-much-later failure you describe; - XPU host, feature patch (
dist_profiler_patch_xpu):logger.exceptionand continue, since losingprofiler.tool: vtunecosts observability, not correctness.
Also moved the patch-module imports inside the try, so a module-level ImportError (e.g. verl-core moving a symbol) follows the same policy rather than escaping apply_all() and breaking plugin load on every host.
Covered by tests/test_xpu_patches.py::test_apply_all_raises_when_required_patch_fails_on_xpu and ::test_apply_all_reports_but_survives_optional_patch_failure_on_xpu.
| def apply() -> None: | ||
| global _applied | ||
| if _applied: | ||
| return | ||
|
|
||
| from verl.utils.profiler.profile import DistProfiler, _NoOpProfiler |
There was a problem hiding this comment.
Correct, and this one had teeth — this repo has already been bitten by exactly this shape (the MLU profiler patch breaking other platforms). Added the guard.
Rather than adding a third copy of _xpu_available(), I factored it into patches/_xpu_guard.py and pointed all three modules at it, so the guard cannot drift between them again.
Verified both directions on the same image: on a CPU-only container all three patches report _applied is False, torch.distributed.all_reduce is unchanged, and DistProfiler.__init__.__qualname__ is still core's DistProfiler.__init__; on a 2×B60 pod all three install. Regression test: tests/test_xpu_patches.py::test_dist_profiler_untouched_without_xpu.
…ance Copilot review on PR #8, four findings; three were real: - dist_profiler_patch_xpu had no XPU guard, unlike the other two patches. DistProfiler is a process-wide class object, so importing this plugin on a CPU/CUDA/NPU host rerouted *that* host's `profiler.tool: vtune` to the Intel ITT implementation. Added the guard. Also factored the duplicated _xpu_available() helper into patches/_xpu_guard.py so all three modules share one definition and it cannot drift again. - patches.apply_all() caught every exception and logged at debug, so an XPU process could continue after a required workaround failed to install. Split by consequence: reduce_avg and attention are correctness-critical and now raise RuntimeError on an XPU host; the vtune patch is observability and is reported with logger.exception. Non-XPU hosts stay debug-only, since nothing was going to install there anyway. Patch modules are now imported inside the try so a module-level import failure follows the same policy. - itt_profile_xpu leaked an ITT range when the timed body raised, in both marked_timer() and VtuneProfiler.annotate(). A leaked range makes every later range on the thread nest inside it, so the trace is unreadable in exactly the runs worth reading. Both now pop in a finally; annotate() also stops discrete collection on the error path. The fourth finding (use self.model, not self.module, in fsdp_xpu.py) is inverted. verl's FSDPEngine assigns the wrapped model to self.module in _build_model_optimizer(), called from initialize(); there is no self.model on these engines, only self.model_config -- confirmed by grep over verl/workers/engine/, which has no `self.model =` anywhere. The old `hasattr(self.model, ...)` guard is why the workaround silently never ran. Copilot cited docs/development.md as the contract, so the docs were the wrong artifact: fixed all 7 occurrences there and stated the contract explicitly so it does not get reintroduced. Also adds the unit tests the PR body listed as outstanding, covering each fix: non-XPU no-op per patch module, the raise/log-and-continue split, ITT push/pop balance on the exception path, and the engine's root-plus-every-wrapped-layer recursion.
Ran PR #8's branch on 2xB60 via devctl, inside the image PR #2's Dockerfile builds (build context = this branch, so COPY . puts this code in the image). verl-core cbf4f31b, torch 2.13.0+xpu, vLLM 0.29.0. 85 unit tests pass in-image; all three patches install on the XPU host and none install in the same image on a CPU-only container; attention/vtune/AVG all behave as claimed; and verl-core's PlatformBase hooks are confirmed inert because this plugin overrides none of them, which is PR #8's whole premise. Also corrects 'xccl cannot execute ReduceOp.AVG at all' in the failure-policy text I added in the previous commit. A plugin-free baseline on the same pod showed unpatched xccl returning the correct mean for a small 2-rank AVG collective. That is consistent with the real reason, already established on the sibling branch from 4xB60 measurements: oneCCL implements AVG on its SYCL-kernel path but not its scheduler path, and path selection is internal and not capability-aware, so a small collective can land on the working path. Recorded the baseline result and that caveat in the design doc so nobody reads a passing 2-rank smoke test as evidence the patch is unnecessary. Note: this branch's reduce_avg_allreduce_patch_xpu docstring still carries the older blanket phrasing, and still patches every process group rather than scoping to xccl ones. Both were corrected on experiment/xpu-monkeypatch-reduce-avg-attention (4b90cec, 08fb6a9) and are left for a separate commit here -- neither was raised in this PR's review.
# Conflicts: # verl_hardware_plugin/patches/__init__.py
verl-core's set_numa_affinity() uses pynvml.nvmlDeviceSetCpuAffinity(), which is absent on XPU and silently no-ops, leaving every XPU worker unpinned and paying cross-socket memory traffic for the life of the job. This replaces it entirely from the plugin side (pyzes PCI BDF lookup -> sysfs numa_node -> cpulist -> os.sched_setaffinity), applied from PlatformXPU.__init__ like reduce_avg_allreduce_patch, with zero verl-core changes. Rebases the pure-monkeypatch result from experiment/xpu-numa-affinity-monkeypatch onto current main and the patches/xpu/ layout, dropping the unrelated attention/profiler/reduce_avg bundling that branch carried from PR #8's base. Verified end-to-end on B60 2026-09-24 (6/6 checks, 512->256 CPUs narrowed, bdf=0000:3d:00.0 numa_node=0) on the prior branch; same patch logic here, rebased.
Summary
Companion experiment to PR #22 and issue #26 — implements the same capabilities as #7917's 4 PlatformBase hooks (
is_reduce_avg_supported,attention_utils_module,dist_profiler_cls) entirely inside this plugin, with zero dependency on #7917 ever merging. Does not touchfeature/xpu-vtune-avg-attention; built frommainso it can be compared directly for footprint and how much it depends on core changes landing.See
docs/design/xpu-monkeypatch-experiment.mdfor the full per-capability verdict table.Works without any core change:
attention_utils_module— patchesattention_utils._get_attention_functionsis_reduce_avg_supported(FSDP2) — no patch at all; extendedfsdp_xpu.py's existing post-initialize()step to cover every FSDP2-wrapped submodule, not just the rootis_reduce_avg_supported(3 rawdist.all_reduce(AVG)sites) — patchestorch.distributed.all_reduceprocess-wide (flagged as a real design smell, not presented as clean)dist_profiler_cls(vtune tool selection) — patchesDistProfiler.__init__Deliberately NOT patched (genuine blockers, not solved by patching harder):
profiler_markers— ambientmarked_timer/mark_start_rangecalls throughout the trainer; this is a one-shot import-time module selection inverl/utils/profiler/__init__.py, no per-call dispatch to interceptReview fixes (Copilot round 1)
dist_profiler_patch_xpuhad no XPU guard, unlike the other two patches — on a CPU/CUDA/NPU host, importing this plugin rerouted that host'sprofiler.tool: vtuneto the Intel ITT implementation. Guard added, and the duplicated_xpu_available()helper factored intopatches/_xpu_guard.pyso all three modules share one definition.apply_all()caught every exception at debug level. Now split by consequence: correctness-critical patches (reduce_avg, attention) raiseRuntimeErroron an XPU host; the vtune patch reports vialogger.exceptionand continues; non-XPU hosts stay debug-only.itt_profile_xpuleaked an ITT range when the timed body raised, in bothmarked_timer()andVtuneProfiler.annotate(). Both now pop in afinally.self.model, notself.module) was inverted — verl'sFSDPEnginehas noself.modelat all, and the oldhasattr(self.model, ...)guard is why the workaround never ran. Copilot citeddocs/development.md, so the docs were the wrong artifact; fixed there instead. Details in the review thread.Hardware validation
Run on 2×B60 via devctl, inside the image built by PR #2's
docker/intel_gpu/Dockerfile.intel_gpuwith this branch as the build context. verl-corecbf4f31b, torch2.13.0+xpu, vLLM0.29.0.torch.xpu.is_available(), vLLM platformxpu, plugin entry point discoveredpatches/)attention_utilsreturns thenpu_flash_attn_utilsset;profiler.tool: vtuneresolves toVtuneProfiler; 2-rank xcclall_reduce(AVG)returns the true meancbf4f31bdoes define thePlatformBasehooks, and this plugin overrides none of them — so the hooks are inert and the patches did the workprofiler.tool: vtune— not done; the above exercises the patches directly, not a trainer loopOne caveat recorded in
docs/design/xpu-monkeypatch-experiment.md: a plugin-free baseline on the same pod showed unpatched xccl computingReduceOp.AVGcorrectly for a small 2-rank tensor. That does not contradict the patch — oneCCL implements AVG on its SYCL-kernel path but not its scheduler path, and path selection is internal — but it does mean a 2-rank smoke test cannot demonstrate the failure the patch prevents.Known divergence from the sibling branch
experiment/xpu-monkeypatch-reduce-avg-attentionhas two later fixes to the same patch that this branch does not yet carry, neither raised in this PR's review: the corrected oneCCL reasoning in the docstring (4b90cec) and scoping the patch to xccl process groups instead of all of them (08fb6a9). Left for a separate commit.