Repository navigation
Conversation
…te() Add `enable_return_routed_experts` config/CLI flag (default off). When enabled, scatter logical expert ids into a CUDA-graph-safe GPU buffer keyed by physical KV slot during fused MoE, then gather per-request int16 `[seq_len-1, num_layers, top_k]` tensors and include them in `RequestOutput.routed_experts`. Requires DCP=PCP=1; prefix caching stays on. Includes unit tests for scatter/gather, prefix hits, dummy negative slots, and length contract.
… and MXFP4 MoE - Extend `enable_return_routed_experts` gating to also require `pipeline_parallel_size == 1`. - Account for the routed-experts capture buffer in KV-pool planning: reserve per-block bytes and a one-time pad row. - Add a sacrificial pad row to the capture buffer so CUDA-graph dummy slots (`-1`) no longer clobber physical slot 0; centralize route trimming in `trim_routed_experts`. - Record logical expert ids in `Mxfp4MoEMethod` before the Triton-only return paths. - Update tests for PP gating, graph-dummy isolation, and per-block byte budgeting.
…ogy and capture from Triton routing - Extend `enable_return_routed_experts` gating to reject KV transfer, KV offload, and RapidServe prefill/decode disaggregation in addition to DCP/PCP/PP != 1; centralize the check in `check_return_routed_experts` and call it from `Config`, `Scheduler`, and `RapidServeModelRunner`. - Move routed-experts capture buffer initialization before the KV-pool expected-vs-actual byte check so the buffer is counted in the page budget. - Reconstruct logical top-k expert ids directly from the Triton routing outputs (`routing_data`/`gather_idx`) via `topk_ids_from_triton_routing` and feed them to `maybe_capture_routed_experts`; pass `routing_outputs` into `triton_kernel_moe_forward` to avoid computing routing twice. - Update CLI help and add tests for the new rejection rules, RapidServe decode skip, and Triton routing id reconstruction.
… and plugin-mode MoE wrappers - Reject `enable_return_routed_experts` when DP-attention is enabled; captured top-k ids align to the gathered token stream, not the local slot mapping, so exporting them would silently return another rank's routes. Update `Config`, `Scheduler`, `RapidServeModelRunner`, and CLI help to pass and document the new restriction. - Detect FusedMoE modules through `fused_moe_modules` / `is_fused_moe_module`, which also recognizes `LazyMoEWrapper` instances used in plugin mode. Dense checkpoints now raise a clear error instead of silently skipping capture. - Add a GPU e2e test covering tensor-parallel, CUDA-graph decode, and prefix-cache hit paths for `routed_experts` generation. - Add unit tests for the DP-attention gate and plugin-mode wrapper detection.
…ith async D2H - Replace GPU `buffer[block_table]` gather in `RoutedExpertsCapturer.export_batch` with a host `cpu_buffer` filled incrementally by `store_step`. `kv_slots_from_block_table` now returns host numpy so export is a numpy gather. - Add `store_step` to enqueue non-blocking D2H of the current step's `slot_mapping` rows on the token async copy stream, and `commit_pending` to flush rows once their token copy is complete. `keep_last` defers the in-flight step until the next recv. - Wire `store_step` into `ModelRunner` after `prepare_sampled_ids` and in the spec-decode verify path; call `commit_pending` before attaching routed experts. For deferred output, attach to `prev_batch` instead of the current batch. - Update and add tests for `store_step`, empty-step flush, pad-slot skip, and `commit_pending(keep_last)`.
… match current async D2H event usage
🏷️ CI GuideRuns automatically on every eligible PR before approval:
Heavy model tests:
|
There was a problem hiding this comment.
🟡 Changes recommended
GPU-buffer lifecycle and PAGE-backed checkpoint accounting issues remain unresolved, with additional avoidable overhead in the default path.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Lite
| page_pool( | ||
| capture_bytes_per_kv_block(self.block_size, num_layers, top_k) | ||
| ) |
There was a problem hiding this comment.
The geometry concern is right, though DeepSeek-V4 can't hit it.
DeepSeek-V4 / V4.1: R3 refuses these builders before any surcharge is added. Both inherit CommonAttentionBuilder, where supplies_token_slots = False, so _routed_experts_geometry() raises at startup.
Where it does cause an issue: Kimi's MLA+GDN builders (kimi_linear, glm5_next_text). They supply token slots, use PAGE checkpoints, and don't override checkpoint_page_unit_bytes(). R3 refuses pipeline parallelism and RapidServe, which are the only cases where Kimi turns checkpoints off, so with R3 on, page_unit_bytes took the merged "kv" charge including the route rows. PageUnitGeometryMixin._page_unit_regions then failed the equality check. Qwen GDN was unaffected because its override already narrows the unit to its MHA regions.
Fix: I kept the route rows in "kv". They grow with the block count, so charging them per block is what keeps the capture buffer inside --gpu-memory-utilization. What changed is where the checkpoint unit comes from: the new checkpoint_page_unit_bytes(builder) in page_unit_checkpoint.py sums only the target builder's own PAGE specs, and a backend override can still narrow that further. Sizing still uses the merged charge.
Why not refuse R3 when state_transfer().copies is on: that would also rule out R3 on Qwen3-Next and Qwen3.5-MoE, which work.
| from atom.model_ops.fused_moe.routed_experts_capturer import ( | ||
| _current_slot_mapping, | ||
| ) | ||
|
|
||
| slots = _current_slot_mapping() |
Sleep must drop the capture singleton's GPU/CPU buffers so wake does not treat them as overhead and allocate a second pool before replacing it. Add reset calls in ModelRunner cleanup and MemoryManagerMixin._release_kv_cache, plus a unit test.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical routing and compilation issues, plus multiple memory-accounting defects, block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
atom/model_engine/model_runner.py:1459
- The route buffer is allocated and charged to the PAGE pool on every TP worker, but
_store_routed_experts_stepand_attach_routed_expertsexplicitly skip everyrank != 0worker. Those workers therefore hold a full GPU buffer and CPU twin that are never read, multiplying the memory surcharge by TP size and reducing KV capacity unnecessarily. Either make initialization/pool pricing rank-0-only or consume the per-rank buffers in an explicit aggregation path.
geo = self._routed_experts_geometry()
if geo is not None:
from atom.model_ops.fused_moe.routed_experts_capturer import (
capture_bytes_per_kv_block,
)
atom/model_engine/model_runner.py:2006
- The pad row is subtracted from
available_for_kv, but it is not represented inpool_plan.total_reserved_bytes.allocate_kv_cache()then allocates that extra row, so the later expected-vs-actual comparison is always short bycapture_pad_row_bytes(*geo)(and can exceed the 3% threshold for small pools). Include the pad allocation in the same accounting used by the mismatch check rather than only reducing the budget.
# Capture buffer is part of the PAGE budget (per-slot int32 routes).
# Allocate it before the expected-vs-actual check so the measurement
# includes those bytes rather than warning on a false mismatch.
self._maybe_init_routed_experts_capturer(num_kvcache_blocks)
atom/model_engine/model_runner.py:2087
num_layersis derived from the last MoE layer rather than the model's layer count. Qwen3-Next-style configs can place MoE layers sparsely (decoder_sparse_step/mlp_only_layers), so a dense suffix makesrouted_expertsshorter than the documented[seq_len - 1, num_layers, top_k]contract. Size this dimension from the target HF config and leave dense-layer rows zero-filled.
self._routed_experts_geo = (
max(layer_ids) + 1,
max(int(m.top_k) for m in moes),
)
atom/model_engine/model_runner.py:1465
- The R3 surcharge is merged into the PAGE entry size, but the route tensor is allocated separately by
_maybe_init_routed_experts_capturerand is not part of the builder's paged pool. For builders withstate_transfer.copies(for example Kimi MLA/GDN),get_num_blocks()passes this inflated PAGE size asPagedStateCheckpointSpec.page_unit_bytes, while the checkpoint geometry measures only the actual KV/index regions and raises a page-unit mismatch. Keep the surcharge out of checkpoint page-unit geometry or explicitly reject page-backed state transfer when R3 is enabled.
page_pool(
capture_bytes_per_kv_block(self.block_size, num_layers, top_k)
)
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
…n instead of DP-attention
There was a problem hiding this comment.
🟡 Changes recommended
A moderate active issue remains because DP-attention batches can return incomplete per-sequence routes instead of failing closed for a normal Aiter/GDN topology.
12 open issues
Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… Reject DP attention before exporting incomplete routes · New Size capture geometry using the full decoder layer count Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
1 resolved since last review
Review effort: Lite
| if self.config.enable_dp_attention and builder_cls.supports_token_graph: | ||
| raise ValueError( | ||
| "enable_return_routed_experts does not support enable_dp_attention " | ||
| f"with {builder_cls.__name__}: its prefill token graphs make an " | ||
| "idle DP rank run a dummy prefill on physical block 0, whose " | ||
| "captures overwrite the routes a real request reads there" |
There was a problem hiding this comment.
🟡 Changes recommended
An active nit remains because the contract comment contradicts the implementation and tests about whether routed-expert capture supports DP attention, potentially misleading users about supported topologies.
13 open issues
Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Correct the routed-expert DP-attention contract comment · New Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
Review effort: Lite
valar-cop
left a comment
There was a problem hiding this comment.
LGTM
ci Run non-GPU unit tests failed — not sure yet (low): The Pre Checkin job failed but its log (.valar/ci/job_126820971975.log) is 0 bytes, so the cause cannot be classified; the same job passes on main and other PR runs show only cancellations.
next: Re-run the job (or fetch a non-empty log) before merging; if it reproduces, inspect the non-GPU unit test output. https://github.com/ROCm/ATOM/actions/runs/41900249814/job/126820971975
…ts, and update restriction text
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because the change has multiple hidden correctness issues spanning deferred route handling, mixed prefix-cache batches, and KV sleep/wake state cleanup.
13 open issues
Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Correct the routed-expert DP-attention contract comment Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
Review effort: Lite
valarLip
left a comment
There was a problem hiding this comment.
Reviewed head c9e710c75. The capture/export design has converged a lot since the last round, but there is one crash on a supported topology, and several earlier asks are still open (folded in below where they overlap, listed at the end otherwise). Memory and KV-capacity figures below are estimates from the stated formulas, not measurements; the one timing figure is from a CPU microbenchmark of this PR's own apply_routed_expert_patch.
Must change
- Pure middle chunks read the drafter's
slot_mapping(model_runner.py:3884). In the middle-chunk branch,propose_draft_token_ids(..., align_only=True)runs first, and only then_store_routed_experts_step(batch, stream=...)withslot_mapping=None, sostore_stepreadscurrent_slot_mapping()after the drafter replaced it (eagle_proposer.py:509setsattn_metadata.slot_mapping = var["slot_mapping"].gpu[:running_bs];dspark_proposer.py:905does the same). With DP>1 + MTP k>=2 (or DSpark) + a prompt long enough to produce a pure middle chunk + R3 -- none of which is refused --store_stepraisesthe step forwarded N tokens but its slot_mapping has running_bs slotsand the engine core dies; if the lengths ever fit, the rows are gathered from draft slots.postprocessalready snapshotsslots = current_slot_mapping()before propose; do the same here and pass it explicitly. No test covers the align path.
Worth addressing
- One capture point instead of five (
moe.py:5448). The hook is pasted afterselect_expertsatmoe.py:5448/5524,comm_fused_moe.py:370and twice inqwen4_exp.py. Both quant methods'select_expertsalready go through_select_logical(moe.py:506), which has the layer and returns logical ids beforeplace_experts. Capturing there covers FusedMoE and comm-fused at once, leaving only Qwen4Exp's in-kernel branch. Since the buffer is filled with -1 once and never cleared on slot reuse, a future forward path that misses the hook returns the previous occupant's routes rather than -1, with nothing to catch it. - The prefix-hit gather is unbudgeted and unbounded (
routed_experts_capturer.py:340).self.buffer[rows_at].to(int16)materializes an int32 gather plus an int16 copy sized to this step's tokens plus the whole cached prefix, and the profile run never allocates it (_store_routed_experts_stepreturns onis_dummy_run). Estimate,tokens x L x K x (4 + 2) B: DeepSeek-R1 (L=61, K=8) is 2.9 KB/token, ~290 MB for a 100k-token prefix hit, taken from HBM the planner already gave to KV. The int16 rows (~98 MB for 100k tokens) are also queued on the copy stream ahead of the token D2H, so the next step'scopy_donewait covers them. Bound it per step (chunkrouted_prefix_ranges) or read the prefix once at finish. - Capture buffer sizing (
routed_experts_capturer.py:30), also A4.CAPTURE_DTYPE = torch.int32while every reader casts to int16 and both the ids and -1 fit int16, so the PAGE surcharge is twice what is needed; the layer axis also charges dense layers, whose rows are always -1. Estimate, surchargeL x K x 4 Bper token against per-rank KV bytes per token: R1 fp8 MLA 1952 B vs 35,136 B, ~5.3% of KV blocks (int16: ~2.7%); Qwen3-235B TP8 fp8 (1 KV head/rank) 3008 B vs 24,064 B, ~11%. The reply to A4 deferred int16 as "a separate follow-up"; it is a one-constant change here and halves the gather as well. - Only rank 0 reads the routes (
model_runner.py:2455), also A4. Every TP rank still allocates the fulltorch.fullbuffer and does anindex_putper MoE layer per forward. Keeping the PAGE charge on all ranks keeps block counts identical, as the reply notes, but the buffer and the scatter are only needed on rank 0. Estimate,(num_slots + 1) x L x K x 4 B: ~1.9 GB per non-zero rank for ~1M slots on R1, unread. RequestOutput.routed_expertshas no reader (scheduler.py:4651).generate()readsSequence.routed_experts(llm_engine.py:1077); nothing inatom/reads theRequestOutputfield. Meanwhile the finalRequestOutputgoes out as("STREAM", ...)andfinished_seqsis pickled with__dict__including_routed_expert_buf(engine_core.py:575/581), so each finished request's routes cross processes twice (estimate: 32k tokens x 61 x 8 x 2 B ~ 31 MB per copy on R1). Please drop the field and its kwarg.- The drop warning fires on every normal finish (
scheduler.py:4153). With deferred output a request whose EOS was sampled in step N is already in batch N+1. Postprocess finishes it and removes it fromself.running; batch N+1's routes arrive with the next output andrunning_by_idhas no entry, so every finished request logs "not in the batch it was scheduled in" -- which is also not what happened. Skip ids that are already finished (or log at debug) and keep the warning for the case it was added for (earlier thread). - Finish truncates but never checks completeness (
scheduler.py:4775, also:3080). On abort/reject the previous step's patch can still be in flight;finish_routed_expertsfreezes the buffer and the late patch is dropped, so the returned array has fewer thanseq_len - 1rows without -1 fill and without an error, whileRequestOutputdocuments[seq_len - 1, ...]. Either assertrows == keepfor non-aborted finishes or document that aborted outputs are short. - Sleep leaves stale route state (
memory_manager.py:335).release_memoryrunstokenID_processor.clean()(dropsprev_batch) and_release_kv_cacheresets the capturer, but_deferred_routes/_held_routesare only cleared inexit. After wake the first deferred step attaches the pre-sleepPendingRoutesto an output whose tokens belong to a new batch. This is one more symptom of 10/11. - A second deferred pipeline beside the token one (
model_runner.py:3662). Routes get their ownafter_recvcallback threaded intoprepare_sampled_ids, a_deferred_routesmirror ofprev_batch, a_held_routeslist,held_first/take_heldordering rules, and a per-steprecord_stream+ event. Logprobs already ride the token D2H entry and itscopy_done, andoutput_transfer.D2HQueueexists for exactly this. Make routes another payload of the token entry and the ordering, clean() and sleep cases come for free. The non-deferred branch (rows.cpu()) is reachable only with PP or a P/D producer, both refused; please delete it. - Process-global singleton (
routed_experts_capturer.py:25), A7 still open._INSTANCE, the weakref owner, the runner's_routed_experts_capturerplus the per-stepget() is not capturercheck, and teardown written twice (model_runner.py:1210,memory_manager.py:335) -- and item 9 shows the copies already drifting. Binding each target FusedMoE abuffer[:, layer]view in the KV bind loop makes drafter exclusion and sleep/wake follow the KV cache's lifecycle. ATOM runs one runner per process, so the multi-owner machinery protects a configuration that isn't supported. _routed_experts_geometryis not a getter (model_runner.py:2368). It stampsmoe_capture_layer_idon modules, enforces refusals and caches viahasattr, and runs as a side effect of_sub_pool_specs/get_num_blocks. Please make it an explicit_bind_routed_experts_layers()called once after load. Itsenable_dp_attention and builder_cls.supports_token_graphbranch (:2395) can never fire: the onlysupports_token_graphbuilder (V4.1) is already refused one check earlier forsupplies_token_slots = False. Delete it, and the matching sentence in the description.- Mono refusal is in the wrong place (
deployment.py:147).common_refusalsis shared withatom/mono/dsa_indexer/dispatch.py, which routes no experts, so on GLM-5 with R3 on the DSA indexer mono turns off silently for no reason. Put the refusal on the MoE mono installers that route in-kernel. (The new docstring line is also well past the surrounding width.) - Per-request host work in the scheduler (
sequence.py:647)._apply_routed_expert_patchdoes one Python iteration plus numpy calls per request per step: ~1.05 µs/request/step in a CPU microbenchmark of this PR's code (R1 geometry), i.e. ~270 µs/step at bs=256 on the engine-core thread. The doubling buffer copies the whole history on each growth and the frozen view pins the doubled base (estimate: ~125 MB live / up to 250 MB capacity for a 128k-token R1 request). The final length is known at admission (prompt + max_tokens - 1), so allocate once._routed_experts_final+ read-only flag + the copy-while-running property mode can go too: both readers run after finish, andseq.is_finishedalready says that. - Docs (earlier, section 4).
docs/configuration_guide.md(Config and EngineArgs tables) still lacksenable_return_routed_experts/--enable-return-routed-experts;pool_entriesthere still says entries come only from the attention builders'sub_pool_specs();docs/scheduling_kv_cache_guide.mdlacksScheduledBatchOutput.routed_expertsandScheduledBatch.routed_prefix_ranges;gdn_attn.py:1689's docstring still says the runner takes the unit from the pagedentry_bytes. The config comment and CLI help list only the config-time refusals: dense models, builders without token slots (V4/V4.1) and "mono turns off" are only discovered after weight load.
Smaller
checkpoint_page_unit_bytes(builder)(module function, always resolves) andbuilder.checkpoint_page_unit_bytes()(optional override, may return None) share a name; rename the resolver (e.g.resolve_checkpoint_page_unit_bytes). It also adds an engine ->model_opsimport intopage_unit_checkpoint.py; this arithmetic sits more naturally next toplan_pools.- That change also fixes a startup
RuntimeErrorfor a copy-checkpoint backend without its own unit (Kimi MLA+GDN) plus an MHA draft that owns a pool. Worth saying so in the description. scheduler.py:4043: the preempt fix-up ofrouted_forwarded_endis the only R3 hook not gated onself._return_routed_experts.is_fused_moe_module's docstring attributes the wrapper to plugin mode, but the decorator is applied unconditionally (moe.py:3452).moe_decode_single_token(..., return_topk_ids=False) -> Tensor | tuple[...]: two callers, so always returning(out, topk_ids)gives one return type.PendingRoutesandRoutedExpertsPatchdiffer only in tensor-vs-ndarray and the event; with 10 one record type is enough andmerge_routed_patchescan become a list applied in order.
Still open from earlier rounds
Not covered above (A4 is in 4/5, A7 in 11, the docs ask in 15):
- A2: per-step trimming (
scheduler.py:4626). Kept because the gap check needs the live length. That check could compare a patch's start againstseq.num_tokens - 1, which postprocess already maintains, instead of keeping a second counter in step with it every step. - A3: have the scheduler send the computed prefix slots. The upload is pinned now, but the runner still rebuilds slots from
batch.block_tablesby batch position with a freshpin_memory()per prefix-hit step. --enable-return-routed-expertsis exposed on the server, where nothing readsrouted_experts(section 3). A server with the flag pays the capture and KV cost and returns nothing; either refuse it inopenai_serveror document it as offline-only.test_rollout_memory_manager.pyinstalls the global capturer without fixture teardown (section 4). If the test fails before_release_kv_cache, every later MoE test in the process captures.
| None, # nothing verified -> segment's last row | ||
| align_only=True, | ||
| ) | ||
| held = self._store_routed_experts_step( |
There was a problem hiding this comment.
[1] Must change. propose_draft_token_ids(align_only=True) just above has already replaced attn_metadata.slot_mapping (eagle_proposer.py:509, dspark_proposer.py:905), so store_step reads current_slot_mapping() and gets running_bs draft slots. DP>1 + MTP k>=2 / DSpark + a pure middle chunk raises forwarded N tokens but its slot_mapping has running_bs slots. Snapshot current_slot_mapping() before the propose and pass it, as postprocess does.
There was a problem hiding this comment.
Fixed. A snapshot alone wouldn't be enough here: the drafter's decode metadata is a view of the same forward_vars slot_mapping buffer, and eagle_proposer.py:888/892 writes it in place. A saved reference would read draft slots in its first running_bs entries, so the crash becomes silently wrong routes. postprocess is only safe because its gather is queued inside prepare_sampled_ids, ahead of the propose. So the middle-chunk branch now stores its routes before the aligning propose, and stream order covers the rest. Added test_output_less_forward_stores_routes_before_the_aligning_propose to test_model_runner_staging_fence.py, which checks that order in forward.
| topk_weights, topk_ids = self.quant_method.select_experts( | ||
| self, hidden_states, router_logits | ||
| ) | ||
| maybe_capture_routed_experts(self, topk_ids) |
There was a problem hiding this comment.
[2] Both quant methods' select_experts go through _select_logical (L506), which has the layer and returns logical ids before place_experts. One capture there replaces this, L5524 and comm_fused_moe.py:370. A path that misses a pasted hook returns the slot's previous occupant's routes, not -1.
There was a problem hiding this comment.
Done. Capture now lives in _select_logical, which every quant method's select_experts returns through.
| req_ids = np.concatenate([p_req, req_ids]) | ||
| starts = np.concatenate([p_starts, starts]) | ||
| lens = np.concatenate([p_lens, lens]) | ||
| rows = self.buffer[rows_at].to(dtype=torch.int16) |
There was a problem hiding this comment.
[3] This gather is sized to the step's tokens plus the whole cached prefix, int32 then an int16 copy, and the profile run never allocates it. Estimate tokens x L x K x 6 B: ~290 MB for a 100k-token prefix hit on R1, from HBM the planner already gave to KV. Bound it per step or read the prefix at finish.
There was a problem hiding this comment.
Agreed, the prefix-hit gather isn't bounded, and the profile run never sees it. It only happens on the first step after a prefix-cache hit, and the memory is freed once the D2H completes. Splitting routed_prefix_ranges across steps would trip the gap check, and reading at finish needs a new runner round-trip, so I'd like to do this as a follow-up rather than grow this PR.
| # Missed captures read as this, never as a valid expert id. | ||
| UNCAPTURED = -1 | ||
| # The GPU capture buffer's element type; the KV pool budget is sized from it. | ||
| CAPTURE_DTYPE = torch.int32 |
There was a problem hiding this comment.
[4] (also A4) int16 holds every id and -1; int32 doubles the PAGE surcharge (estimate: R1 fp8 ~5.3% of KV blocks vs ~2.7%, Qwen3-235B TP8 fp8 ~11%) and adds a cast kernel per gather. The layer axis also charges dense layers that are always -1.
There was a problem hiding this comment.
We kept int32 on purpose. topk_ids come out of routing as int32, so capture scatters them with no conversion. An int16 buffer adds a cast kernel per MoE layer per forward: on MI355X under a CUDA graph the performance gets worse. Switching the buffer to int16 while keeping today's index_put scatter makes capture slower: about 190 µs → 355 µs per forward across R1's 61 MoE layers. That's +165 µs, from one extra cast kernel per layer, and it adds to every forward, decode steps included. The memory saving is real (R1 about 5.3% to 2.7% of KV blocks) but only matters when serving is KV-bound. If we want both, the way to get them is a fused scatter that casts in-register, which we can do as a follow-up.
| """ | ||
| if not self._return_routed_experts: | ||
| return None | ||
| if getattr(batch, "is_dummy_run", False) or self.rank != 0: |
There was a problem hiding this comment.
[5] (also A4) Only rank 0 reads, but every rank allocates the buffer and scatters every MoE layer. Keep the PAGE charge on all ranks so block counts match, and create the capturer on rank 0 only; the hook is already a no-op without one.
There was a problem hiding this comment.
Done. The capturer is now created on TP rank 0 only, at startup and on wake, so the other ranks' MoE hook does nothing and they skip the buffer and the per-layer scatter. Every rank still charges the capture rows to its KV pool, so block counts match across ranks. store_step's separate rank check is gone, since "no capturer" already covers it. Added test_only_tp_rank0_allocates_the_capture_buffer.
| return | ||
| cap = max(buf.shape[0], 1) | ||
| while cap < need: | ||
| cap *= 2 |
There was a problem hiding this comment.
[14] The final length is known at admission (prompt + max_tokens - 1); allocate once instead of doubling and copying the history. _routed_experts_final and the copy-while-running property mode are only exercised after finish, where seq.is_finished already says so.
There was a problem hiding this comment.
Dropped _routed_experts_final and the copy-on-read mode. routed_experts is now a view of the live rows, and the late-patch guard uses seq.is_finished. finish_routed_experts still trims and freezes, so the pickle carries only the live rows.
I'd keep the doubling instead of allocating at admission. max_tokens is a cap, not the length: RL rollouts set it to the response budget, and most responses stop at EOS well short of it. At about 1 KB per row (61 layers × top-8 × int16), 256 concurrent requests with max_tokens=32k would reserve around 8 GB of host memory per engine, mostly for rows that are never written. Doubling copies each row at most about once more over the request's life, and the spare capacity is trimmed at finish.
| # [seq_len - 1, num_layers, top_k]. Scatter is CUDA-graph-safe (device | ||
| # write inside fused MoE). Requires DCP=PCP=PP=1, no plugin mode, and | ||
| # no KV transfer / offload / RapidServe P/D; prefix cache stays on. | ||
| enable_return_routed_experts: bool = False |
There was a problem hiding this comment.
[15] Not in docs/configuration_guide.md (Config / EngineArgs tables) or docs/scheduling_kv_cache_guide.md. This comment and the CLI help list only the config-time refusals; dense models, V4/V4.1 builders and "mono turns off" are found only after weight load.
| and leave_reason is None | ||
| and not seq.is_partial_prefill | ||
| ): | ||
| keep = max(int(num_tokens) - 1, 0) |
There was a problem hiding this comment.
A2 still open. The gap check could compare a patch's start against seq.num_tokens - 1, which postprocess already maintains, rather than truncating a second counter every step.
There was a problem hiding this comment.
Keeping it. Checking against num_tokens - 1 would stop the check from catching a lost patch. postprocess advances num_tokens from the sampled tokens whether or not that step's routes arrived, so the next patch always starts within num_tokens - 1 and passes. Its rows would land after a hole of np.empty rows: uninitialized garbage, not -1, which the trainer would replay as real expert ids.
| ) | ||
|
|
||
|
|
||
| def checkpoint_page_unit_bytes(builder: object) -> int: |
There was a problem hiding this comment.
nit: same name as the optional builder hook checkpoint_page_unit_bytes() it reads, with a different meaning (resolved vs override). A resolve_ prefix would separate them.
There was a problem hiding this comment.
Renamed to resolve_checkpoint_page_unit_bytes, so it no longer shares a name with the builder hook it reads. I also fixed the hook's docstring in gdn_attn.py, which still described the old fallback.
| # The marker counts rows planned, not delivered. Fall back to what was | ||
| # delivered, so a range whose patch is lost is asked for again on | ||
| # resume (read from the cache on a hit, else recomputed). | ||
| seq.routed_forwarded_end = min(seq.routed_forwarded_end, seq.routed_expert_rows) |
There was a problem hiding this comment.
nit: the only R3 hook not gated on self._return_routed_experts.
There was a problem hiding this comment.
Gated on self._return_routed_experts like the other hooks.
… longer running Under deferred output, a finished request's last forward may report after it left the batch, and a preempted request rewinds its routed_forwarded_end. These rows are expected to be dropped, so remove the warning and update the test to assert silent dropping while still applying patches for live requests.
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because the change contains a moderate decode-loop blocking risk and a critical lifecycle issue where stale deferred route state can survive KV capture reset.
12 open issues
Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
1 resolved since last review
Review effort: Lite
… hook - Replace the global RoutedExpertsCapturer singleton with a per-ModelRunner instance bound to target FusedMoE layers; only TP rank 0 exports routes. - Capture routed experts inside `_select_logical` so all quant methods and the comm-fused MoE path are covered by one hook. - Tie the capture buffer's allocation and release to the KV cache lifecycle. - Remove `routed_experts` from `RequestOutput`; validate that finished sequences have `num_tokens - 1` rows unless aborted. - Update tests for per-runner binding, rank-0 export, and unified capture.
…r layer views Bind each target FusedMoE module directly to its slice of the routed-experts buffer via ROUTES_VIEW_ATTR, instead of using a process-global RoutedExpertsCapturer singleton. The capture buffer is now allocated and released with the KV cache lifecycle, and the MoE hook writes through its bound view without global lookups.
…re initializer Drop the ValueError that blocked routed-experts capture under DP attention with prefill token graphs, and rename `_maybe_init_routed_experts_capturer` to `_bind_routed_experts_layers` to reflect its binding role. Remove the corresponding test.
There was a problem hiding this comment.
🔵 Needs a closer look
Approval should be deferred because the change spans correctness-critical CUDA-graph, async D2H, TP/DP, and MTP paths without available GPU workloads, CI results, or a runnable PR head to validate the advertised behavior.
12 open issues
Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
Review effort: Lite
…and checkpoint helper rename - Move the in-kernel routed-expert refusal from common_refusals into a per-model routed_experts_refusal check; models whose kernels route in-kernel turn off mono decode, while DeepSeek-V4/V4.1 is refused after weight load for attention backends without per-token slots. - Return a read-only view of live routed-expert rows instead of copying, drop the _routed_experts_final flag, and use is_finished to drop late patches. - Only rewind routed_forwarded_end on preemption when the feature is on. - Rename checkpoint_page_unit_bytes to resolve_checkpoint_page_unit_bytes. - Update docs and tests.
There was a problem hiding this comment.
🟡 Changes recommended
A critical active finding remains: DP attention with prefill token graphs can overwrite real slot-0 routing data with dummy execution routes, so the capture path is unsafe until this topology is rejected or handled.
13 open issues
Dummy prefill capture can overwrite cached routes in slot 0 · New Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
Review effort: Lite
| builder_cls = type(self.attn_metadata_builder) | ||
| if not builder_cls.supplies_token_slots: | ||
| raise ValueError( | ||
| "enable_return_routed_experts requires per-token KV slots in the " | ||
| f"attention metadata; {builder_cls.__name__} does not supply them, " | ||
| "so routes would never be written" | ||
| ) |
There was a problem hiding this comment.
🟡 Changes recommended
A moderate active finding shows the DeepSeek-V4 mono path raises an unrelated AttributeError instead of cleanly refusing the unsupported routed-expert topology, so the startup check still requires correction.
14 open issues
Dummy prefill capture can overwrite cached routes in slot 0 Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… V4 mono startup check crashes on missing class attribute · New Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
Review effort: Lite
| builder_cls = type(self.attn_metadata_builder) | ||
| if not builder_cls.supplies_token_slots: |
There was a problem hiding this comment.
🟡 Changes recommended
Critical unresolved issues remain in IQ2R route capture and memory-manager teardown, including a missing quantization path and a potential asynchronous write race, while the documentation also needs reconciliation.
17 open issues
Capture routed experts in the IQ2R MoE path · New Drain pending route copies before clearing held routes · New Dummy prefill capture can overwrite cached routes in slot 0 Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… V4 mono startup check crashes on missing class attribute Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Reconcile Qwen4-Exp R3 support docs with implementation · New Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
Review effort: Lite
| num_fused_shared_experts=num_fused_shared_experts, | ||
| fused_shared_experts_scoring_func=layer.shared_expert_scoring_func, | ||
| routed_scaling_factor=layer.routed_scaling_factor, | ||
| ) | ||
| maybe_capture_routed_experts(layer, topk_ids) |
| # Routes for the batch `clean()` just dropped; kept, the first | ||
| # deferred step after wake would attach them to a new batch's output. | ||
| self._held_routes = [] | ||
| self._deferred_routes = None |
| capturing = getattr(ex, ROUTES_VIEW_ATTR, None) is not None | ||
| out = moe_decode_single_token( | ||
| hidden_states, | ||
| None, | ||
| None, | ||
| *weights, | ||
| router_logits=logits, | ||
| top_k=ex.top_k, | ||
| return_topk_ids=capturing, | ||
| ) | ||
| if capturing: | ||
| out, topk_ids = out | ||
| maybe_capture_routed_experts(ex, topk_ids) |
…uler metric mocks
There was a problem hiding this comment.
🔵 Needs a closer look
Two critical active issues remain: IQ2R routing bypasses capture and can yield missing rows, while releasing pending routes without synchronizing async copies can cause buffer reuse races.
17 open issues
Drain pending route copies before clearing held routes Capture routed experts in the IQ2R MoE path Dummy prefill capture can overwrite cached routes in slot 0 Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… V4 mono startup check crashes on missing class attribute Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Reconcile Qwen4-Exp R3 support docs with implementation Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
Review effort: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
A critical active finding shows the capture hook is bypassed on the IQ2R quantization path, causing routed-expert capture to fail closed for those layers and requiring a code fix plus regression test.
17 open issues
Drain pending route copies before clearing held routes Capture routed experts in the IQ2R MoE path Dummy prefill capture can overwrite cached routes in slot 0 Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… V4 mono startup check crashes on missing class attribute Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Reconcile Qwen4-Exp R3 support docs with implementation Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
Review effort: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
An active moderate issue leaves IQ2R/GLM-5.3 replay data incomplete because its routing bypasses the capture hook, with no coverage test confirming the corrected path.
17 open issues
Drain pending route copies before clearing held routes Capture routed experts in the IQ2R MoE path Dummy prefill capture can overwrite cached routes in slot 0 Clear reused routing slots before forward execution Capture routes in DeepSeek V4.1 mono decode Exclude torch.nn.Module from fused MoE module detection Reject MoE decoders that bypass route capture hooks Disable route capture during draft model execution Handle backends without slot_mapping during route capture Initialize routed-expert capture before level-3 compilation Adding the capture surcharge to the sharedPAGED_CLASSchangesplan.entry_bytes["kv"], which is… V4 mono startup check crashes on missing class attribute Reject DP attention before exporting incomplete routes Size capture geometry using the full decoder layer count Reconcile Qwen4-Exp R3 support docs with implementation Test KV-cache refusal through runner entry points This capture-only slot lookup runs on every forward even whenenable_return_routed_expertsis…
Review effort: Lite



Motivation
This PR adds Rollout Router Replay (R3) support to the ATOM runtime. Inspired by
vllm-project/vllm#28284, it records the expert routing
decision for every token at every MoE layer during inference, so RL
post-training can replay the exact routing the rollout used.
vLLM exposes this as
enable_return_routed_experts→CompletionOutput.routed_experts(int16 [seq_len-1, num_layers, top_k]).ATOM
LLM.generate()did not, so Lumen-RL and similar trainers could nothard-assign ATOM routes on the actor forward.
Goals:
generate(), includingCUDA-graph decode and prefix-cache hits.
wrong, instead of returning bad routes.
Routes are returned as
routed_expertsin each request'sgenerate()outputdict, read from the finished
Sequence.RequestOutput(the per-step streamcallback) does not carry them: no in-repo consumer reads them there, and the
OpenAI server never returned them.
Technical Details
Flag (default
False):Config.enable_return_routed_experts/--enable-return-routed-experts.ModelRunnerandSchedulerread it onceinto
_return_routed_experts; when it is off, none of the paths below run.Capture (GPU, inside the forward)
atom/model_ops/fused_moe/routed_experts_capturer.pyowns one GPU buffer,int32 [num_slots + 1, num_layers, top_k], indexed by physical KV slot andinitialized to
-1(UNCAPTURED).allocate_kv_cachebinds each target FusedMoE its layer's view,buffer[:, layer](routed_experts_view), on TP rank 0 only. Sleep clears itwith the KV views and wake rebinds it, so it shares the KV cache's lifecycle;
there is no process-global capturer.
_select_logical, which every quant method'sselect_expertsreturnsthrough, calls
maybe_capture_routed_experts(layer, topk_ids). The writehappens on device, so CUDA-graph replay still records it.
slot_mapping == PAD_SLOT_ID == -1, which indexes that row, so padding neveroverwrites slot 0.
slot_mappingindexes the buffer directly; there is noseparate destination computation.
k < top_kpadding is-1, so an uncaptured cell is never mistaken forexpert 0.
cannot write into the target model's rows.
the pad row) before the expected-vs-actual KV check.
Export (runner → scheduler)
Per step, on rank 0:
store_stepgathersbuffer[slot_mapping[:total_tokens]]into int16 rows onthe compute stream. It has to run there, before the next forward rewrites
the
slot_mappingstaging buffer in place.async_copy_stream, with their own event, and kept on the batch asPendingRoutes.PendingRoutes.patch()waits on the event and attaches one
RoutedExpertsPatch(req_ids, starts, offsets, rows). Each batch is attachedat most once; a pure middle prefill chunk, which builds no output, holds its
routes until the next output that does.
Output is always deferred under R3. The only non-deferred configurations (PP,
and a P/D producer with recurrent state) are already refused, so there is one
export path.
The IPC payload is the step's new rows plus three small index arrays,
independent of context length.
Accumulation (scheduler)
Scheduler._apply_routed_experts_patchwrites each request's segment intoSequence's growable int16 buffer (doubling capacity) at its absolute startposition. A re-forwarded range overwrites the old rows. A patch that would
leave a gap raises instead of handing the trainer uninitialized rows. Rows
for a request that has left
runningare dropped: a finished request's lastin-flight step is past its
num_tokens - 1rows, and a preempted one asksfor them again on resume.
routed_forwarded_endcounts rows planned, not delivered, sopreempt rewinds it to
routed_expert_rows. A range whose patch was lost isre-requested on resume (from the cache on a hit, otherwise recomputed).
_plan_routed_prefixesrecords, at schedule time, anyprefill that starts past what the request has forwarded. The runner turns
that range into slots with
prefix_slots(the same slot formula the forwarduses) and exports those rows once, as an extra segment of the same copy.
finish_routed_experts(num_tokens - 1), which trims to the live rows andfreezes them read-only. Rejected speculative tokens are already excluded from
num_tokens. A non-aborted finish with fewer thannum_tokens - 1rowsraises. An aborted request may be short, because its last step's patch can
still be in flight; it returns the prefix it has. Later patches for a
finished request are ignored.
Sleep / wake
release_memory(tags=["kv_cache"])clears each MoE layer'srouted_experts_viewwith the KV views and drops the runner's capturer, so thebuffer is freed with the KV cache and wake's
get_num_blocks()/allocate_kv_cache()don't count it as overhead and allocate a second one.Wake recreates and rebinds it in
allocate_kv_cache. Sleep also drops thepending routes (
_held_routes/_deferred_routes) next totokenID_processor.clean(), so a stale step's rows can't attach to the firstbatch after wake.
Refusals
check_return_routed_experts(Config,Scheduler,RapidServeModelRunner)ModelRunnerFusedMoEin the modelsupplies_token_slots(e.g. DeepSeek-V4 / V4.1)routed_experts_refusal)FusedMoE; disabled under R3Downstream: Lumen-RL's Ray ATOM rollout sets this flag when
moe.r3.enabledand forwards
routed_expertsfromATOMRayServer.Test Plan
On MI355X (TP = 2):
Covers batch-order segments, prefix segments and slot-formula parity, graph pad
tail,
-1for uncaptured rows and a shortslot_mapping, attach-once across adeferred step and a middle chunk, the async copy snapshot before staging reuse,
the gap and short-finish errors, finish trim/freeze, the preempt rewind, the
disabled path, drafter MoE exclusion, every refusal (each mono model's included),
and sleep unbinding the views. The e2e (TP = 2) covers CUDA-graph decode with a
prefix hit, concurrent requests, chunked prefill followed by a prefix hit, and
MTP 1 / 3 verify, checking each against a prefill of the same tokens.
Test Result
1105f69f1: the unit tests above are 585 passed, and the e2e on2 × MI355X is 5 passed. The full
tests/suite has no failures that are notalso on the merge base with
main: every file that failed was rerun on bothtrees, and the sets match.
MiniMax-M3 TP 4, Qwen4-Exp bs 1, DeepSeek-V3 MLA prefix hit): routes have the
right shape, contain no
-1, and a prefix hit returns identical cached rows.Host cost per decode step (MI355X, ctx 4096)
(2.33 ms per step before the batch-order export.)
The per-layer capture views cost the same as the old global scatter (±0.5%;
62 layers × top-k 8 at 256 tokens: 194.3 vs 193.9 µs).
Serving, R3 off vs on (MI355X, Nov 4)
Qwen3-30B-A3B-Base, TP 1,
max_num_seqs256, random 1024 in / 1024 out,concurrency 256, 2560 requests. Runs alternate off/on with the compile cache
cleared before each start.
Run 1 (off, 20,177 tok/s) overlapped the unit-test suite on the host and is
left out. Throughput and TPOT are within noise; mean TTFT is about 3% higher
with R3 on. An earlier round on the previous revision measured 2.6% lower
throughput with R3 on.
Lumen-RL 1-step DAPO smoke (KL)
Image
zhangdanyangamd/lumen-rl:dapo-gfx950-rocm7.2.3-260910, 8× gfx950, seed10086, Qwen3-30B-A3B-Base, Megatron native (TP = 1, PP = 1, EP = 8, a2a),
ATOM Ray rollout (TP = 1, CUDA graphs,
sleep_level=2), 64 sequences, 256-tokenresponses,
moe.r3.enabled+replay_mode=hard_assignment.Same-weight
r3_verifybefore the optimizer step:Native ATOM R3 cuts the large-mismatch tail ~2×, matching vLLM. The residual is
train/infer kernel mismatch, not missing routes.
Generate performance
Qwen3-30B-A3B serving,
max_num_seqs=256(measured before the A3 fixes,which only remove host syncs):
MiniMax-M3-MXFP8, TP=4, 8192 in / 1024 out: −0.8% tok/s at concurrency 16,
−1.4% at concurrency 64; TTFT unchanged.
Submission Checklist