Repository navigation
[XPU] draft: reduce_avg support via pure monkeypatch, no core changes - #11
Merged
Merged
Conversation
Split out of experiment/xpu-monkeypatch-no-core-hooks: covers is_reduce_avg_supported (FSDP2 per-layer loop, no patch needed; raw dist.all_reduce(AVG), patched) and attention_utils_module. Both work with zero verl-core changes, independent of the VTune profiler half (docs/design/xpu-monkeypatch-vtune-profiler.md), which still has an open blocker. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…atch attention_utils_module() is staying a PlatformBase hook (see PR #22, and verl-project/verl#7917's updated description) -- it passed the maintainer's bar on verl-hardware-plugin#26 for a genuinely hardware-agnostic capability that couldn't be done cleanly from the plugin alone. Carrying a monkeypatch alternative for it here too just duplicates an already-accepted, working solution with no benefit. is_reduce_avg_supported() didn't pass that bar and is this branch's actual job: the xccl AVG gap can be fully handled from the plugin (FSDP2 via an engine-level method override, the 3 remaining raw dist.all_reduce(op=AVG) sites via a scoped, documented monkeypatch). Renamed the design doc to match (xpu-monkeypatch-reduce-avg.md) and folded in a note that this branch is now the sole owner of the fsdp_xpu.py self.model->self.module fix -- it's been dropped from #22's smaller, root-only version of the same fix now that #22 no longer touches this file for any other reason.
verl_hardware_plugin/__init__.py is shared across every vendor in this plugin, so calling apply_all_patches() there fired the patch on any host where torch.xpu merely happened to be importable -- regardless of which platform verl actually selected for the run. A mixed host with e.g. VERL_PLATFORM=nvidia set explicitly would still get its process-wide torch.distributed.all_reduce(op=AVG) semantics changed for no reason, compounding the design smell this patch already carries. verl.plugin.platform.platform_manager._create_platform() only constructs PlatformXPU() once platform detection has actually picked "intel" for this process, so moving the call into PlatformXPU.__init__ ties it to real platform selection instead. The narrow remaining edge case -- auto-detection probing PlatformXPU() to check availability, then falling through to a different platform -- is self-mitigating: the patch's own _xpu_available() gate fails in the same underlying probe that made is_platform_available() fail. Also: - Add unit tests for both the new wiring (PlatformXPU() triggers the patch; the plugin's top-level __init__ no longer binds it directly) and the reduce_avg_allreduce_patch_xpu wrapper itself (AVG -> SUM+divide, other ops pass through unchanged, async_op=True falls through unpatched, no-op when XPU unavailable, idempotent). 75 passed. - Remove docs/design/xpu-monkeypatch-reduce-avg.md. This repo has no precedent for keeping RFC/experiment-verdict docs in the merged tree (confirmed: no docs/design/ directory exists on main, in any prior PR). Its reasoning already lives in this PR's description.
…irectly apply_all() had exactly one caller (PlatformXPU.__init__) and exactly one patch module (reduce_avg_allreduce_patch_xpu) -- no other vendor in this plugin has any monkeypatches. The generic "apply every patch" name and loop implied a cross-vendor aggregator like register_all_platforms()/ register_all_engines(), which genuinely do need to register every vendor up front since verl-core's registries look classes up by name after the fact. Patches don't share that constraint: they only ever matter to the one platform that already got selected, so each platform's own __init__ calling its own patch module directly is both simpler and more honest about what's actually happening than routing through a same-package wrapper that only ever had one real implementation behind it. verl_hardware_plugin/patches/__init__.py is now a plain package docstring pointing at reduce_avg_allreduce_patch_xpu, with no code. Updated the wiring test to mock the real target (reduce_avg_allreduce_patch_xpu.apply) instead of the removed wrapper. 75 passed.
_patched_all_reduce only checked op != AVG and async_op -- it never checked which backend the target group actually uses. Once applied, it silently intercepted op=AVG for every process group in the process, not just xccl ones: a gloo group in the same process (e.g. a CPU-only Ray actor's coordination group alongside a GPU worker's xccl group) would get routed through SUM+divide too, even though gloo/nccl handle AVG natively and never needed the workaround. Added dist.get_backend(group) != "xccl" to the short-circuit condition so non-xccl groups fall through to the original, untouched. Short-circuit evaluation means only the actual substitution path needs get_backend mocked in tests -- the passthrough tests (non-AVG op, async_op=True) never reach it, since op != AVG or async_op already short-circuits first. Added a test for a gloo-backed group with op=AVG to confirm it now passes through unpatched. 76 passed.
Per kahlun's review-thread investigation on verl-project/verl#7917 (measured on real 4xB60 hardware, not reasoned from source): xccl does not uniformly lack ReduceOp.AVG. oneCCL implements it on its SYCL-kernel execution path but not on its scheduler path; path selection is internal to oneCCL and isn't capability-aware, so an AVG request can abort mid-collective ("average operation is not supported for the scheduler path") rather than reliably succeed or reliably fail. Fix tracked for oneCCL 2022.2 / torch 2.15, not yet available on the stack this plugin targets. Also note the separate, narrower double-division bug on very small messages (intel/torch-xpu-ops#3020) is fixed on that same stack and is not what either the FSDP2 engine override or this patch works around -- easy to conflate the two since both are filed against xccl's AVG support. The blanket "xccl does not support ReduceOp.AVG" phrasing this replaces was accurate enough to justify the workaround but not accurate about why, and was written before this investigation existed. Same mechanism, same correctness -- corrected reasoning only. platform_xpu.py has the same blanket phrasing but predates both this branch and #22; left alone here since fixing it isn't part of this PR's diff. 76 passed.
3 of 5 tasks
…duce_patch in xpu module
…tches/__init__.py
…tdated platform subpackage references
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.
Open question for reviewers, up front
reduce_avg_allreduce_patch_xpu.pypatchestorch.distributed.all_reduceprocess-wide whenop=AVG— the only mechanism available for verl-core's 3 rawdist.all_reduce(op=ReduceOp.AVG)call sites, since there's no per-site wrapper to hook into and this plugin doesn't own those call sites. This changesReduceOp.AVG's process-wide meaning for every caller in the process, XPU-related or not, for as long as the process runs (there's no narrower call-boundary to scope it to — see Changes below for why). That's a real design smell, not a clean win, and is the one piece here still worth a narrow core-hook ask if the process-wide semantics change is judged unacceptable — happy to split it into its own PR against #7917 if so.Summary
Plugin-side replacement for
is_reduce_avg_supported()— implemented with zeroverl-corechanges: no newPlatformBasehook, no dependency on verl-project/verl#7917.Motivation
verl-hardware-plugin#26 asked maintainers whether Intel XPU should extend
PlatformBaseinverl-core(the TPU precedent) or stay entirely inside this plugin via monkeypatch (the MLU precedent). The maintainer's answer (2026-09-23): default to the plugin, keep #7917 narrowed to only what truly can't be done from here.is_reduce_avg_supported/all_reduce_avghas just been dropped from #7917 and from #22 on that basis (xccl not implementingReduceOp.AVGdoesn't need a core hook). This PR is the plugin-side replacement.attention_utils_module()and the profiler hooks passed the maintainer's bar and stay asPlatformBasehooks — implemented for XPU in #22, which this PR does not touch or duplicate. (An earlier draft of this branch also monkeypatched attention as a "how far can pure monkeypatch go" comparison; dropped once it was clear the hook was staying and a plugin-only alternative for it served no purpose.)Changes
verl_hardware_plugin/engines/fsdp_xpu.py: extend the existing (non-hook, non-patch)initialize()override to callset_force_sum_reduction_for_comms(True)on the FSDP2 root and every individually-wrapped transformer layer, not just the root — eachfully_shard()'d module has independent FSDP state. This is ordinary method-override behavior on the plugin's ownEngineRegistry-registered engine, not a monkeypatch, so it's robust regardless of import order. Also fixes an unrelatedself.model→self.moduleattribute bug in the same function (FSDPEngineWithLMHead/WithValueHeadexpose the wrapped model asself.module, neverself.model) — [rebase to main] 13-10-2026 #22 had picked up the same fix independently since it lives in the same lines; dropped from [rebase to main] 13-10-2026 #22 in favor of this PR's superset version (root + per-layer, vs. root-only).verl_hardware_plugin/patches/reduce_avg_allreduce_patch_xpu.py(new): monkeypatchestorch.distributed.all_reduce(op=AVG)→SUM+ manual divide, process-wide, for the three remaining rawdist.all_reduce(op=ReduceOp.AVG)call sites in verl-core (sft_trainer.py,engine_workers.py,profiler/performance.py::reduce_timing). See the open question above.async_op=Truecall sites are intentionally left unpatched (falls through to original behavior) rather than risk returning a wrong result before the collective completes.verl_hardware_plugin/patches/__init__.py(new): plain package docstring, no code — holdsreduce_avg_allreduce_patch_xpuas the one patch module. No genericapply_all()aggregator: it had exactly one caller and one module, so it wasn't earning its keep (see the platform_xpu.py bullet below).verl_hardware_plugin/platforms/platform_xpu.py:PlatformXPU.__init__callsreduce_avg_allreduce_patch_xpu.apply()directly. Deliberately not called from the sharedverl_hardware_plugin/__init__.py(that file is common to every vendor this plugin supports) —PlatformXPU()is only constructed onceverl.plugin.platform.platform_manager._create_platform()has actually selected"intel"as the platform for this process. Calling it from the shared top-level__init__.pyinstead would fire on any host wheretorch.xpumerely happens to be importable, even if a different platform ends up selected for the run — e.g. a mixed host withVERL_PLATFORM=nvidiaexplicitly set would still get its process-wideall_reducesemantics changed for no reason. (Two earlier drafts of this PR had this wired the wrong way, then routed through an unnecessary same-packageapply_all()wrapper — both fixed.)Testing
pytest tests/ -vpasses (75 passed), including new coverage:PlatformXPU()instantiation triggersreduce_avg_allreduce_patch_xpu.apply(); the shared plugin__init__.pyno longer binds any patch call directly (regression guard against re-introducing the wrong wiring).reduce_avg_allreduce_patch_xpu:op=AVG→SUM+ divide by world size; other ops pass through unchanged;async_op=True+AVGfalls through unpatched; no-op when XPU unavailable; idempotent (callingapply()twice doesn't double-wrap).run_grpo_fsdp2_intel_gpu.sh(6/6 PASS with the fix, 6/6 FAIL without), independently re-confirmed onrun_grpo_colocate_rm_fsdp2_intel_gpu.sh. What's new here is the method-override framing — not yet re-run standalone in this exact form.reduce_avg_allreduce_patch_xpu.py's actual collective behavior is unit-tested (above) but not yet run on a B60 end-to-end. Opening as draft for that reason.Acceptance Baseline (for new hardware adaptation PRs)
scripts/baseline_grpo_gsm8k.shon target hardware (8 devices)critic/rewards/meanshows clear upward trend in first 100 stepsNot yet run standalone on this branch — will complete before requesting review to move this out of draft.
Checklist
pre-commitchecksdocs/design/file; this repo has no precedent for keeping RFC/experiment-verdict docs in the merged tree