[MoE] Expose zero-copy MegaMoE workspace output view - #4341
Merged
Merged
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe PR adds backend capability declarations for workspace-backed outputs, exposes ChangesWorkspace-backed output views
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change preserves the existing copied-output behavior by default and adds an opt-in workspace-backed view with backend capability gating; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Caller
participant MoEEpMegaLayer
participant MegaKernelBackend
participant Workspace
Caller->>MoEEpMegaLayer: forward(return_workspace_view=True)
MoEEpMegaLayer->>MegaKernelBackend: check supports_output_view
MoEEpMegaLayer->>Workspace: omit owned output allocation
Workspace-->>MoEEpMegaLayer: return workspace-backed output view
MoEEpMegaLayer-->>Caller: return output view
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mhoqueanik
reviewed
Aug 4, 2026
mhoqueanik
left a comment
mhoqueanik
left a comment
Collaborator
There was a problem hiding this comment.
overall looks like a good suggestion. Can you briefly run the microbench from here to make sure the PR doesn't break the upstream paths?
Contributor
Author
@mhoqueanik Yes — I ran the Both upstream MegaMoE paths completed successfully:
I also reran the focused CUDA-graph, output-view, and 4-GPU multi-rank tests. The detailed results are included in the PR description. This is not the original 8-GPU reference configuration, but I did not observe any failure or regression in the upstream paths. |
foraxe
marked this pull request as ready for review
August 6, 2026 01:58
foraxe
requested review from
Anerudhan,
Aneureka,
IwakuraRein,
aleozlx,
feih-nv,
jiahanc,
nv-yunzheq,
samuellees and
yzh119
as code owners
August 6, 2026 01:58
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/moe_ep/test_mega_cuda_graph.py (1)
247-272: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd MXFP8 coverage for the public output-view API.
The feature contract enables
return_workspace_view=Truefor NVFP4 and MXFP8. This test initializes only"nvfp4". Add an MXFP8 case that checks support, shape, workspace aliasing, repeatability, and equality with the copied output.Confidence: High.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/moe_ep/test_mega_cuda_graph.py` around lines 247 - 272, The test test_mega_layer_forward_output_view_public_api currently covers only NVFP4; extend it to run the same assertions for MXFP8 as well. Parameterize or otherwise repeat the case for both layer types, verifying supports_output_view, output shape, workspace aliasing for non-empty batches, repeatability, and equality with the copied output while preserving setup and cleanup for each case.flashinfer/moe_ep/modes/mega_layer.py (1)
191-196: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument the first-call precondition for
knobs="auto".If
return_workspace_view=Trueis the first forward, both supported CuTeDSL backends rejectcompute(output=None)while autotuning is pending. State thatwarmup()or one owned-output forward must run first, or reject this combination withMoEEpConfigErrorbefore staging.Confidence: high.
As per coding guidelines, keep documentation synchronized with code changes, including documented error handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@flashinfer/moe_ep/modes/mega_layer.py` around lines 191 - 196, Update the MegaMoE forward API documentation near the return_workspace_view description to state that knobs="auto" requires warmup() or one prior owned-output forward before return_workspace_view=True; alternatively, add an early MoEEpConfigError validation for this combination before staging. Keep the documented behavior synchronized with the implemented precondition and backend constraints.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@flashinfer/moe_ep/modes/mega_layer.py`:
- Around line 191-196: Update the MegaMoE forward API documentation near the
return_workspace_view description to state that knobs="auto" requires warmup()
or one prior owned-output forward before return_workspace_view=True;
alternatively, add an early MoEEpConfigError validation for this combination
before staging. Keep the documented behavior synchronized with the implemented
precondition and backend constraints.
In `@tests/moe_ep/test_mega_cuda_graph.py`:
- Around line 247-272: The test test_mega_layer_forward_output_view_public_api
currently covers only NVFP4; extend it to run the same assertions for MXFP8 as
well. Parameterize or otherwise repeat the case for both layer types, verifying
supports_output_view, output shape, workspace aliasing for non-empty batches,
repeatability, and equality with the copied output while preserving setup and
cleanup for each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f806aec2-1d7e-440d-8c80-75399bd24dc5
📒 Files selected for processing (6)
flashinfer/moe_ep/backends/mega/kernel/mxfp8_cutedsl/backend.pyflashinfer/moe_ep/backends/mega/kernel/nvfp4_cutedsl/backend.pyflashinfer/moe_ep/core/kernel/base.pyflashinfer/moe_ep/modes/mega_layer.pytests/moe_ep/test_mega_cuda_graph.pytests/moe_ep/test_moe_ep_nvfp4_cutedsl_mega_multirank.py
5 tasks
Collaborator
|
Hi @foraxe, there is a minor conflict (it was introduced after the push style sm90 megamoe). Could you resolve it? |
foraxe
force-pushed
the
codex/zero-copy-output
branch
from
August 16, 2026 07:38
adbcaab to
6f1d4f5
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
Author
|
Hi @mhoqueanik Thanks for pointing this out; I rebased onto the latest main, resolved the SM90 MegaMoE conflict, and verified the relevant tests pass. |
Collaborator
|
Hi @foraxe — while preparing the moe_ep merge queue we ran your PR merged with current main through the full moe_ep suite and hit one unit failure:
Making the owned-output allocation conditional moved the The fix keeps your conditional and just restores the ordering — we validated exactly this patch (unit + Blackwell mega multirank green on 8×B200): --- a/flashinfer/moe_ep/modes/mega_layer.py
+++ b/flashinfer/moe_ep/modes/mega_layer.py
@@ -220,12 +220,9 @@ class MoEEpMegaLayer(nn.Module):
workspace = self._ensure_workspace()
- self._kernel.stage_inputs(
- t,
- workspace,
- quantize_input=quantize_input,
- )
-
+ # Owned-output allocation must stay ahead of the staging round (see
+ # test_mega_layer_allocates_output_before_staging_round): allocator
+ # work between stage and compute can sync the device mid-round.
y = None
if not return_workspace_view:
y = torch.empty(
@@ -234,6 +231,13 @@ class MoEEpMegaLayer(nn.Module):
dtype=torch.bfloat16,
device=t.hidden_states.device,
)
+
+ self._kernel.stage_inputs(
+ t,
+ workspace,
+ quantize_input=quantize_input,
+ )
+
return self._kernel.compute(
workspace,
self._transformed,No rebase needed — the branch merges cleanly with main; just this one commit, then a fresh |
Contributor
Author
|
@flashinfer-bot run |
Contributor
Author
|
Thanks @mhoqueanik, I've applied the fix in 982cb6a by moving output allocation before stage_inputs to satisfy the allocate-before-staging contract; |
Collaborator
|
@flashinfer-bot run |
mhoqueanik
approved these changes
Aug 17, 2026
Collaborator
|
/bot run tests/moe_ep |
Collaborator
Collaborator
|
[FAILED] Pipeline #63146462 — 27/30 executed test jobs passed Compared with nightly #63077496. Unit Tests
✅ Pass · 🟡 Old failure · ❌ New failure · ⏱ Test timeout · Multi-GPU and Multi-Node Tests — 6/6 passed
Failure detailsPR-related regressions
New relative to nightly (attribution uncertain)
Timeouts, infrastructure, or incomplete jobs
|
Contributor
Author
|
We reran the unit tests in local env. (on GB200, Driver Version: 580.105.08, CUDA Version: 13.1) Ran with: Local results:
So in this local env, those DSLRuntimeError failures do not reproduce. They look environment/branch-variant specific to the CI stack (B300/GB200 + CUDA 13.0), not currently reproducible on this local setup. We also tested in sgl-project/sglang#31470 (comment) The new failures in bot pipeline seems like a broad MoE cutlass/DSL compiler regression on CUDA 13.0 + B300/GB200 (same DSLRuntimeError across many unrelated tests), not a narrow PR regression from this output-view/output-order fix. |
Collaborator
|
@foraxe Agreed! I am looking into this |
4 tasks
jefby
pushed a commit
to jefby/flashinfer
that referenced
this pull request
Aug 19, 2026
) ## Summary This draft exposes a backward-compatible zero-copy output path for the FlashInfer MegaMoE layer. The new `return_workspace_view=False` argument preserves the existing owned output behavior by default. When enabled and supported by the selected MegaMoE backend, `forward` returns the layer's workspace output view instead of copying the result into a new tensor. Capability gating keeps unsupported backends on the existing path. Backend support is expressed as the `supports_output_view` capability property on the kernel contract. The patch also keeps the workspace-view contract explicit across the NVFP4 and MXFP8 CuTeDSL MegaMoE backends and updates the relevant CUDA-graph and multi-rank tests. ## Before and after ```text Before (default behavior): MoEEpTensors | v MegaMoE forward(...) | v workspace output --copy--> caller-owned output tensor After (opt-in view): MoEEpTensors | v MegaMoE forward(..., return_workspace_view=True) | v workspace output view ------> caller uses workspace-backed tensor ``` The default remains the copied, caller-owned output. The new path makes the workspace view available only when the backend advertises the capability. ## Motivation The SGLang MegaMoE integration currently needs the computed output to remain in the FlashInfer workspace. Returning that view removes a large per-layer output materialization while retaining the old API behavior for callers that need an owned tensor. ## Validation - `tests/moe_ep/test_mega_cuda_graph.py`: 8 passed. - The targeted output-view test passes: 1 passed, 7 deselected. - Four-GPU NVFP4 multi-rank output-view coverage passes with `NVSHMEM_DISABLE_CUDA_VMM=1`. - The corresponding SGLang adapter test passes 3 cases when paired with this API. The upstream `moe_ep_benchmark` harness was also run from the `vllm_repro_8_gpu_v2` branch with the FlashInfer MegaMoE section on four GPUs (`GPUS=4`, `DEVS=0,1,2,3`). This is a GB200 smoke/regression run, not an 8-GPU reference result: | backend | tokens/rank | hidden/intermediate | experts/top-k | p50 latency | throughput | | --- | ---: | ---: | ---: | ---: | ---: | | NVFP4 CuTeDSL | 32 | 7168 / 2048 | 256 / 8 | 358.4 us | 357174.7 | | MXFP8 CuTeDSL | 32 | 7168 / 2048 | 256 / 8 | 619.9 us | 206472.9 | The harness reported accuracy-loss fields of 23.201 percent for NVFP4 and 6.371 percent for MXFP8 against its BF16 dense reference. These are retained as harness output and are not used as a model accuracy claim here. This is the FlashInfer side of the paired SGLang integration. The SGLang draft depends on this API and is posted separately. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an option to return workspace-backed output views, reducing unnecessary output copying when supported. * Added capability detection so unsupported configurations provide a clear error while preserving existing default behavior. * Enabled output views for supported FP8 and FP4 workflows. * **Tests** * Added coverage for output-view shapes, workspace aliasing, repeatability, zero-token inputs, and multi-size execution. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
11 tasks
aleozlx
added a commit
that referenced
this pull request
Sep 5, 2026
…es (#4956) <!-- .github/pull_request_template.md --> ## 📌 Description `.github/workflows/ci-bot-commands.yml` decides whether a PR comment is a bot command with unanchored substring matches: ``` # BOT below stands for the literal bot handle, elided so this PR does not trigger itself. if: github.event.issue.pull_request && contains(github.event.comment.body, 'BOT') ... elif echo "$COMMENT_BODY" | grep -qi "BOT run"; then ``` Neither is anchored, so the phrase matches **anywhere** in a comment body — inside inline code spans, fenced blocks, markdown tables, and quoted reply history. *Writing about* a command runs it. Each accidental fire re-applies the `run-ci` label, which emits a `labeled` event, which under `concurrency: cancel-in-progress` **cancels the in-flight GPU run and starts a new one**. A run is ~4.5 hours, so each accident is expensive. The known workaround is to write the handle with a zero-width entity (`@flashinfer​-bot`) — a hack no contributor should need to know. ### The fix A command counts only when it **starts a line that is not inside a fenced code block.** Implemented entirely inside the `Parse command` step, in two stages: 1. `awk` drops fenced code blocks (both ``` and `~~~`, including indented fences). 2. `grep -iEm1 '^[[:space:]]*@flashinfer​-bot[[:space:]]+(run|rerun|stop)([[:space:]]|$)'` takes the first surviving line that *begins* with the handle. Leading whitespace is allowed; anything else to the left — `>` for a quoted reply, `|` for a table cell, a backtick for an inline span, or prose — is not. The four existing classifiers then run against that single extracted line, gaining `^` anchors and a trailing word boundary. Order (`rerun failed` before `rerun`) is unchanged. **Why not the job-level `if:`** — GitHub Actions expressions have no regex (only `contains`/`startsWith`/`endsWith`), so the job guard cannot be anchored. It is left as-is and re-commented as a cheap pre-filter. This is harmless: a prose comment now spawns a job that resolves `command=unknown` and takes no action, since every handler step is gated on `steps.parse.outputs.command`. No bot-author guard is included. It would not have prevented any of these accidents — they came from **humans writing documentation**, not from the bot. It is worth adding separately as complementary hardening, but anchoring is the actual fix. ## 🔍 Related Issues No tracking issue. The behaviour was found while working on #4880, where four documentation comments each cancelled and restarted an in-flight ~4.5 hour GPU run — but the problem is repo-wide and predates it (see the replay below, spanning 2025-10-18 → 2026-09-04 across five PRs). This change is independent of #4880: `git grep` confirms lines 26 and 96-102 of `ci-bot-commands.yml` are the only consumers of `comment.body` on `main`. ## 🚀 Pull Request Checklist Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete. ### ✅ Pre-commit Checks - [x] I have installed `pre-commit` by running `pip install pre-commit` (or used your preferred method). - [x] I have installed the hooks with `pre-commit install`. - [x] I have run the hooks manually with `pre-commit run --all-files` and fixed any reported issues. > If you are unsure about how to set up `pre-commit`, see [the pre-commit documentation](https://pre-commit.com/). ## 🧪 Tests - [x] Tests have been added or updated as needed. — see the replay, corpus and verification below. - [x] All tests are passing (`unittest`, etc.). **This file cannot be tested by CI.** `issue_comment` workflows always load from the default branch, so zero CI runs on this PR execute the changed file; it takes effect only once merged. All verification below was therefore done out-of-band. ### Repo-wide replay I replayed **every** issue comment in this repository's history through both matchers — 19,930 comments, of which 725 are handle-bearing PR comments spanning 2025-10-18 → 2026-09-04. | | old | new | |---|---|---| | fires | 689 | 666 | | suppressed (old fired, new does not) | — | **31** | | newly honored (old ignored, new fires) | — | **8** | | reclassified to a different command | — | **0** | **All 31 suppressions are accidental. Zero legitimate commands are lost.** By how the phrase was embedded: 21 inline code span, 4 bare in a prose sentence, 2 table cell, 2 fenced block, 2 blockquote. 8 of the 31 were by users authorized to trigger CI (`aleozlx` ×4, `Anerudhan`, `mhoqueanik`, `qsang-nv`, `yongwww`) across 5 PRs (#4880, #4795, #4341, #3471, #2529) over 7 months — these are the ones that actually consumed GPU CI, and every one is documentation prose. **This is a repo-wide problem, not a #4880 artifact.** The other 23 were by users with only `read` permission, so the bot replied "unauthorized" and no CI ever started; the only loss there is a feedback reaction. The 4 "bare in prose" cases are the most arguable, e.g. *"Could a maintainer please approve the external CI for this PR? @flashinfer-bot run"* (#4435). I checked all three authors (`foraxe`, `DocJlm`, `Archie-wang`): each has only `read` and is not in `ci-users`, so none of these started CI under the old code either. **The change also fixes a latent bug in the other direction.** `@flashinfer-bot` + two spaces + `run` matched *nothing* under the old literal-substring rule. It was silently ignored 8 times by 4 authorized maintainers (`yzh119` ×3, `yongwww` ×3, `jiahanc`, `kahyunnam`); 7 of the 8 carry zero reactions, confirming the handler never fired. Those now work. ### Corpus 33 hand-built cases + the 11 real #4880 comments. Verified three ways: against an independent Python model of the pipeline, by executing the shipped step under `bash -e`, and live in a sandbox repo. **MUST TRIGGER — all preserved** | case | body | old | new | |---|---|---|---| | bare run | `@flashinfer​-bot run` | run | run | | with path | `@flashinfer​-bot run tests/gemm/test_x.py` | run | run | | multiple paths | `@flashinfer​-bot run tests/a.py tests/b.py` | run | run | | leading spaces | `␣␣␣@flashinfer​-bot run` | run | run | | leading tab | `⇥@flashinfer​-bot rerun failed` | rerun-failed | rerun-failed | | mixed case | `@FlashInfer​-Bot RUN` | run | run | | mixed case rerun | `@FLASHINFER​-BOT ReRun` | rerun | rerun | | first line of multi-line | `@flashinfer​-bot run\n\nKicking off CI.` | run | run | | later line of multi-line | `Rebased.\n\n@flashinfer​-bot run` | run | run | | middle line | `Fixed lint.\n@flashinfer​-bot run tests/utils/\nThanks!` | run | run | | rerun | `@flashinfer​-bot rerun` | rerun | rerun | | rerun failed | `@flashinfer​-bot rerun failed` | rerun-failed | rerun-failed | | stop | `@flashinfer​-bot stop` | stop | stop | | trailing prose | `@flashinfer​-bot run please` | run | run | | after a fenced block | a log in a backtick fence, then `@flashinfer​-bot run` below it | run | run | | CRLF line endings | `Rebased.\r\n@flashinfer​-bot run\r\n` | run | run | | trailing whitespace | `@flashinfer​-bot run␣␣␣` | run | run | | after a bullet list | list then `@flashinfer​-bot rerun failed` | rerun-failed | rerun-failed | | **double space** | `@flashinfer​-bot␣␣run` | **unknown** | **run** | **MUST NOT TRIGGER — all now suppressed** | case | body | old | new | |---|---|---|---| | inline code span, in prose | ``The command is `@flashinfer​-bot run` -- type it on its own line.`` | run | **unknown** | | inline code span at line start | code span first on the line, then prose | run | **unknown** | | fenced block | backtick fence listing the commands | stop | **unknown** | | fenced block with language | backtick fence tagged `bash` | run | **unknown** | | tilde fence | `~~~` block | rerun-failed | **unknown** | | blockquote | `> @flashinfer​-bot run` | run | **unknown** | | nested blockquote | `> > @flashinfer​-bot rerun` | rerun | **unknown** | | prose, mid-sentence | `I will ask a maintainer to @flashinfer​-bot run this once...` | run | **unknown** | | table cell | `\| `@flashinfer​-bot run` \| full suite \|` | stop | **unknown** | | cc mention only | `cc @flashinfer​-bot -- could you take a look?` | unknown | unknown | | bullet + inline span | `- **`fix(ci): bind COMMENT_BODY in the @flashinfer​-bot run handler`**` | run | **unknown** | | heading | ``### How `@flashinfer​-bot run` works`` | run | **unknown** | | indented fence in a numbered list | `1.` then an indented backtick fence | run | **unknown** | | prose, sentence start | `Someone should @flashinfer​-bot run the suite again;` | run | **unknown** | | quoted reply history | `> On Tue, alex wrote:\n> @flashinfer​-bot run tests/g...` | run | **unknown** | | the 4 real #4880 documentation comments | (verbatim from the API) | run ×4 | **unknown ×4** | | the 7 real #4880 genuine commands | (verbatim from the API) | run ×7 | run ×7 | ### How it was verified - **Offline**: an independent Python model of the pipeline agrees with the shipped shell step on all 44 corpus cases and on all 725 real handle-bearing comments — 0 divergences. - **Under the real shell**: the `Parse command` step extracted verbatim from the committed file, run as `bash -e` with `COMMENT_BODY` in the environment. All cases exit `rc=0` and always write a `command=` output, including empty, whitespace-only, and non-matching bodies. - **GNU toolchain**: the runner image is not macOS, so the corpus was also run on `ubuntu-24.04` (`GNU grep 3.11`, `GNU Awk 5.2.1`) — **44/44 PASS, 0 FAIL**. - **Live**: 12 headline cases posted as real PR comments in a sandbox repo running a byte-identical copy of the step, driven by a real `issue_comment` event — 6 fired, 6 did not, **0 mismatches**, matching predictions exactly. ## 🔬 Experimental Track <!-- Not an experimental-track PR; section left as the template provides it. --> <!-- Only for PRs submitted under the experimental policy (CONTRIBUTING.md → "Experimental APIs and Backends"). Leave this section untouched for normal PRs. --> - [ ] This PR is **experimental**: it adds or changes code under `flashinfer/experimental/` and/or an `@flashinfer_experimental_api`. Tracking issue: # - [ ] The tracking issue names an owner, the reason for the experimental path, and a graduation plan with a target release. - [ ] Core changes are limited to a thin entry point (signature, shared validation, feature-gate check, backend selection, handoff). - [ ] Tests live in `tests/experimental/` and were validated on the intended hardware; a runnable example is included. - [ ] Nothing is registered in `flashinfer/aot.py`, and no experimental backend is reachable from `backend="auto"` without `FLASHINFER_ALLOW_EXPERIMENTAL_AUTO_BACKENDS=1`. (Calling an `@flashinfer_experimental_api` or naming a backend explicitly is itself the opt-in and needs no environment variable.) - [ ] **Test scope declared below.** The experimental CI lane runs exactly these targets, so keep them as narrow as the change allows. <!-- Required for experimental PRs. Replace the commented lines below with your targets. Do not delete the fence or change its `experimental-tests` tag — the experimental-track watcher reads it verbatim to decide which targets to ask CI for. --> ```experimental-tests # One target per line: a directory or a file. (A pytest ::selector is not # supported -- the sharding runner cannot consume one.) Must be under # tests/experimental/ and must exist. Delete these comment lines and add yours, e.g. # # tests/experimental/test_my_backend.py # tests/experimental/my_backend/ # # Declaring the whole tree (tests/experimental/) is allowed but means every # experimental PR pays for every other feature's tests, in every matrix cell. ``` ## Reviewer Notes ### Reviewability is the safety property `issue_comment` workflows always load from the **default branch**, so this file is executed by zero CI runs on this PR and cannot be tested by any PR. Nothing here validates it before it lands on `main`. That is why the change is confined to one file with a small, obvious diff, and why the verification above was done out-of-band in a sandbox repo instead. ### Residual gaps **Still trigger, arguably should not.** The fence handling is a simple toggle, so a fence nested inside another fence flips it back off, and these leak (verified live): ```` ``` BOT run ``` ```` The same applies to a `~~~` outer fence containing a ``` inner fence. This is ordinary CommonMark nesting and is exactly what a comment documenting *this change* would type. Fixing it properly means tracking the opening fence's character and length, which costs the diff its obviousness; it occurs **zero** times in 725 real comments. Also still triggering: 4-space-indented code blocks, and HTML constructs (`<details>`, `<pre>`, `<!-- -->`), since none of these is a fence. A line that *begins* with the handle and continues into prose — `@flashinfer-bot run is the command you want.` — still fires. This is inherent and unfixable: `run <paths>` is documented, so the two forms are textually identical. **No longer trigger, arguably should.** All fail closed (no CI started, never a false trigger): - A command below an **unterminated** fence is swallowed. A line that merely *starts* with a triple-backtick while actually being a one-line inline code span in GFM (a command wrapped in triple backticks on its own line) also flips fence parity, dropping a genuine command later in the same comment. - Anything to the left of the handle on the line: a bullet (`- `), `1. `, bold (`**`), a non-breaking space, or prose. - `@flashinfer​-bot run-ci` and `@flashinfer​-bot running ...` now resolve to `unknown` (previously `run`), because of the added word boundary. Intentional tightening. - Precedence is now positional rather than by-keyword: a `stop` line above a `rerun failed` line yields `stop`, where the old code yielded `rerun-failed`. Only observable in a comment containing two different commands; occurs zero times in 725 real comments. **Feedback loss.** Comments that no longer parse get no reaction at all, since the "Unauthorized user" step is gated on `command != 'unknown'`. A mis-shaped command is now silent for authorized and unauthorized commenters alike. **Latent, not currently reachable.** The step is correct today because the runner shell is `bash -e 0` *without* `pipefail`, so the `CMD=$(... | grep ... | sed ...)` assignment takes `sed`'s status 0 even when `grep` matches nothing. If anyone later adds `shell: bash` to this step or a `defaults.run.shell: bash` to the workflow, `pipefail` turns on and the assignment returns 1 under `set -e` — a red X on every prose comment mentioning the handle. Fail-safe (never a false trigger), but worth knowing; a trailing `|| true` would immunize it. ### Scope Touches only `.github/workflows/ci-bot-commands.yml` (+25/-5). Confirmed with `git grep` that lines 26 and 96-102 are the only consumers of `comment.body` on `main`, so this is independent of #4880. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved command detection to avoid interpreting regular prose, quoted replies, inline code, tables, and fenced code blocks as commands. - Preserved content inside code fences when opening and closing delimiters do not match. - Improved handling when no valid command is found, preventing unnecessary processing failures. - **Documentation** - Clarified workflow filtering behavior for more transparent command processing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 task
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
4 participants
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.
Summary
This draft exposes a backward-compatible zero-copy output path for the
FlashInfer MegaMoE layer.
The new
return_workspace_view=Falseargument preserves the existing ownedoutput behavior by default. When enabled and supported by the selected
MegaMoE backend,
forwardreturns the layer's workspace output view insteadof copying the result into a new tensor. Capability gating keeps unsupported
backends on the existing path. Backend support is expressed as the
supports_output_viewcapability property on the kernel contract.The patch also keeps the workspace-view contract explicit across the NVFP4 and
MXFP8 CuTeDSL MegaMoE backends and updates the relevant CUDA-graph and
multi-rank tests.
Before and after
The default remains the copied, caller-owned output. The new path makes the
workspace view available only when the backend advertises the capability.
Motivation
The SGLang MegaMoE integration currently needs the computed output to remain
in the FlashInfer workspace. Returning that view removes a large per-layer
output materialization while retaining the old API behavior for callers that
need an owned tensor.
Validation
tests/moe_ep/test_mega_cuda_graph.py: 8 passed.NVSHMEM_DISABLE_CUDA_VMM=1.API.
The upstream
moe_ep_benchmarkharness was also run from thevllm_repro_8_gpu_v2branch with the FlashInfer MegaMoE section on four GPUs(
GPUS=4,DEVS=0,1,2,3). This is a GB200 smoke/regression run, not an8-GPU reference result:
The harness reported accuracy-loss fields of 23.201 percent for NVFP4 and
6.371 percent for MXFP8 against its BF16 dense reference. These are retained
as harness output and are not used as a model accuracy claim here.
This is the FlashInfer side of the paired SGLang integration. The SGLang
draft depends on this API and is posted separately.
Summary by CodeRabbit
New Features
Tests