Repository navigation
Conversation
… fallback mechanisms
…reduce_avg support checks
… function imports
…and platform detection
… hooks Per verl-hardware-plugin#26, the maintainer's ruling (2026-09-23) is to default new hardware backends to plugin-side monkeypatching, reserving new PlatformBase hooks for genuine hardware-agnostic blockers. XPU's reduce_avg workaround doesn't need one: - FSDP2's per-layer set_force_sum_reduction_for_comms() can be applied from the plugin's own EngineRegistry-registered engine, after super().initialize() returns -- ordinary method override, no core hook. - The remaining raw dist.all_reduce(op=AVG) call sites can be monkeypatched from the plugin (torch.distributed.all_reduce is looked up by attribute at call time, so patch order doesn't matter). See verl-hardware-plugin's experiment/xpu-monkeypatch-reduce-avg-attention branch for the plugin-side implementation. Keeps attention_utils_module()/profiler_markers()/dist_profiler_cls() -- those don't have a plugin-only alternative.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved profiler lifecycle issues and missing focused tests for the new plugin dispatch paths.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
Adds optional platform hooks for hardware-specific attention utilities and profiling, enabling Intel XPU integration without device-specific branches.
Changes:
- Adds attention and profiler hooks to
PlatformBase. - Routes attention and profiler dispatch through platform plugins.
- Updates engine, rollout servers, and model tests for plugin support.
| File | Review summary |
|---|---|
verl/workers/rollout/vllm_rollout/vllm_async_server.py |
Plugin profiler lifecycle is not invoked; custom tools may be inert. |
verl/workers/rollout/trtllm_rollout/trtllm_async_server.py |
Plugin profiler lifecycle is not invoked; custom tools may be inert. |
verl/workers/rollout/sglang_rollout/async_sglang_server.py |
Plugin profiler lifecycle is not invoked; custom tools may be inert. |
verl/workers/engine_workers.py |
Accepts plugin profiler tools. |
verl/utils/torch_functional.py |
Uses unified attention dispatch. |
verl/utils/profiler/profile.py |
Supports plugin profiler classes; focused dispatch tests are needed. |
verl/utils/profiler/__init__.py |
Adds lazy plugin marker resolution; focused tests are needed. |
verl/utils/attention_utils.py |
Dispatches attention helpers through plugins; regression tests are needed. |
verl/plugin/platform/platform_base.py |
Defines new hooks; plugin-path coverage is needed. |
tests/plugin/test_platform_abstraction.py |
Updates platform abstraction expectations. |
tests/models/test_transformers_ulysses.py |
Updates tests to use unified attention utilities. |
tests/models/test_transformer.py |
Updates tests to use unified attention utilities; PR description should reflect modified tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…hooks Adds focused unit tests for the three new PlatformBase hooks added in this PR, addressing Copilot review feedback that the new dispatch branches had no regression coverage: - attention_utils_module(): plugin-module dispatch and precedence vs. NPU - profiler_markers(): lazy resolution and forwarding of marker callables - dist_profiler_cls(): plugin tool selection and no-op fallback
The lazy-resolving wrappers for mark_start_range/mark_end_range/ mark_annotate/marked_timer had no docstrings, failing the validate_imported_docs.py sanity check.
…cher test_fsdp_ckpt.py and test_activation_offload.py still had the old if cuda/elif npu branch for unpad_input that test_transformer.py and test_transformers_ulysses.py were already updated to route through attention_utils.py's dispatcher. On any other platform (e.g. XPU), neither branch matched and unpad_input was never bound, raising a NameError.
… hooks to profiler_markers() Applies wuxibin89's review of verl-project#7917. Three changes, in the order they were raised: 1. Drop `PlatformBase.attention_utils_module()`. `npu_flash_attn_utils` is already verl-core and imports nothing but torch/F/einops, so a core->core indirection through the plugin bought nothing. `_get_attention_functions()` now uses it whenever `flash_attn` will not import, the duplicated `_fallback_*` helpers are deleted, and the module is renamed to the device-neutral `attention_padding_utils`. The `torch_functional.py` and test call-site cleanups are kept. 2. Drop `PlatformBase.dist_profiler_cls()`. `engine_workers.py` now converts `tool_config` for any configured tool instead of an allowlist -- `omega_conf_to_dataclass` returns None for a missing/empty config, so a tool with no `tool_config` entry still works and a plugin tool can name its own dataclass via `_target_`. The three rollout-server edits are reverted: `profiler_controller` there only gates, the real profiling goes through `engine.start_profile()`, so a plugin profiler's start/stop never ran. 3. Keep `profiler_markers()`, and fix a bug in it. Resolution checked `is_nvtx_available()` first, but nvtx ships in the `verl-core` extra, so on any box where it imports the hook was dead. The platform is now asked first, falling back to nvtx, then mstx, then the generic markers.
[hardware, perf, training_utils] move_attn to device agnostic and handle profiler_markers
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.

What does this PR do?
Extends
PlatformBase(verl/plugin/platform/platform_base.py) with two optional hook groups so a hardware backend that can't reuse CUDA's exact code paths plugs in cleanly, instead of the rest of verl growing moreif device == "..."branches. These were surfaced while bringing up an Intel XPU backend (companion PR: verl-project#7371) and all default to today's behavior, so CUDA/ROCm/NPU are unaffected. The XPU plugin (verl-hardware-plugin) depends on this PR for the new hooks below.Companion PRs in
verl-hardware-pluginPlatformXPU.attention_utils_module()/profiler_markers()/dist_profiler_cls()), plus docs — verl-hardware-plugin#22is_reduce_avg_supported()hook (xccl doesn't implementReduceOp.AVG), but per the maintainer's ruling on verl-hardware-plugin#26, new hardware backends should default to plugin-side monkeypatching and only ask verl-core for a hook when something genuinely can't be done from the plugin.attention_utils_module(). Lets a platform pointverl/utils/attention_utils.pyat its own flash-attn-equivalent module (a dotted path exposingindex_first_axis/pad_input/rearrange/unpad_input), checked after the NPU path and before falling back to theflash_attnpip package or the pure-PyTorch implementation. Also movedtorch_functional.py's two remaining hardcodedflash_attn/get_device_name() == "cuda"/"npu"branches onto this same dispatcher.profiler_markers()/dist_profiler_cls().profiler_markers()lets a platform supply its own(mark_start_range, mark_end_range, mark_annotate, marked_timer)tuple, picked up byprofiler/__init__.pywhen neither thenvtxpackage nor NPU's built-in markers apply.dist_profiler_cls(tool)lets a platform register aDistProfilersubclass under its ownprofiler.toolname; wired intoDistProfiler.__init__(profile.py) and into the three rollout HTTP servers (vllm_async_server.py, sglang'sasync_sglang_server.py,trtllm_async_server.py) so a plugin-supplied tool name isn't rejected there either.No public API changes — every new
PlatformBasemethod is an optional override with a safe default (-> None).Test
New unit tests, which inject a fake platform so no accelerator is needed:
tests/utils/test_attention_utils.py: checks thatattention_utils_module()sends all four helpers (index_first_axis/pad_input/rearrange/unpad_input) through the plugin module, that NPU still takes precedence, and that verl falls back when the hook returnsNone.tests/utils/test_profiler_plugin_hooks.py: checks thatprofiler_markers()is resolved lazily on first use and the returned markers are forwarded, thatdist_profiler_cls()selects the plugin tool and passes it rank/config/tool_config, and that an unrecognized tool falls back to the no-op profiler.Updated existing tests:
tests/models/test_transformer.py,tests/models/test_transformers_ulysses.py: import the padding helpers fromverl.utils.attention_utilsinstead of branching on device to pickflash_attn.tests/plugin/test_platform_abstraction.py: acceptintel(registered by the XPU plugin) as an auto-detected platform name.The XPU attention/profiler paths were also validated end-to-end on real Intel Arc B-series hardware as part of verl-project#7371's GRPO/PPO/SFT runs.
Checklist Before Submitting
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace. (If not accessible, please try the Feishu group (飞书群).)recipesubmodule, please also update the reference to the submodule commit viagit submodule update --remoteorcd recipe && git pull origin main.