Repository navigation
Replace old use of routing_record with routing_snapshot_ in onnxruntime/contrib_ops/cuda/moe/moe_quantization.cc. - #32932
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The packed INT path consumes a routing snapshot without first capturing one, causing tracked inference to fail.
Review effort: Balanced
Findings: 1
What changed in this PR
Fixes a CUDA QMoE build error caused by a stale routing_record reference.
Changes:
- Replaces the obsolete routing call with
routing_snapshot_->Consume().
| File | Description |
|---|---|
onnxruntime/contrib_ops/cuda/moe/moe_quantization.cc |
Updates packed INT GEMV routing snapshot handling. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Capture packed INT2 routing results before consuming the expert tracking snapshot and add a packed GEMV counting regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mirounga
self-requested a review
September 29, 2026 22:44
mirounga
approved these changes
Sep 29, 2026
This was referenced Oct 1, 2026
David Fan (jiafatom)
added a commit
that referenced
this pull request
Oct 1, 2026
…4a2) (#33045) ## Summary PR4a2 follows up the five non-blocking comments from the final review of #32963 at `d3d60acf26`. Based on main after #32963 was squash-merged as `a4d55e2b6f`. No production kernel math, alignment contract, or runtime eligibility changes. ## Review Follow-ups - **Coverage** ([comment](#32963 (comment))): allow compute capability major 8 in the direct-kernel fixture instead of exactly 8.0. Test parameters continue to select the SM80 implementation. Legacy W4/W8 alignment regressions remain enabled, removing their fixture-level skip on A10/SM86. - **Plugin build footprint** ([comment](#32963 (comment))): restrict dense/W4/W8 baselines and disabled benchmarks to legacy tests; add source-specific plugin/adapter definitions. Reduce plugin internal-test contrib implementation sources from 16 to 4, retaining the adaptor needed for INT2 unpacking. Plugin retains INT2 FP16/BF16 numerical, metadata, valid-offset and misalignment coverage. No measured CI wall-time improvement is claimed. - **Diagnostic options** ([comment](#32963 (comment))): remove unconditional duplicate suppressions. CUDA 13+ inherits 970/2189 from shared options. CUDA <13 retains target-local suppressions with an explanatory comment because the internal INT2 TU exposes third-party Abseil/Protobuf header diagnostics under warning-as-error. Removing them reproduced a CUDA 12.8 compile failure; the explicit version guard fixes it. - **Rebase** ([comment](#32963 (comment))): #32932 is already in the main base. This PR does not replay that fix and has no `moe_quantization.cc` diff. - **Headers** ([comment](#32963 (comment))): add the requested Microsoft copyright/MIT license headers to the helper header, implementation CU and test TU. ## Validation All compilation, formatting, CMake checks and GPU tests ran inside Docker `jiafa-dev` on an A100-SXM4-80GB. | Check | Result | | --- | --- | | CUDA 12.8 plugin standalone harness | 16 passed, zero skipped | | CUDA 12.8 legacy standalone harness | 23 enabled passed, zero skipped; 3 benchmarks disabled | | CUDA 13.0 plugin standalone harness | 16 passed, zero skipped | | Fresh legacy/plugin test TU compilation with CUDA 12.8 and 13.0 headers | Passed | | Fresh four plugin implementation CUs with CUDA 12.8 and 13.0, warning-as-error | Passed | | Fresh legacy INT2 implementation CU, CUDA 12.8 | Passed | | Scoped execution of actual CMake source/definition and option blocks | Passed; four contrib sources enabled / zero disabled, exactly one suppression per diagnostic per toolkit | | Symbol audit | Plugin baseline dependencies/benchmarks absent; legacy coverage retained | | Production body comparison | Unchanged apart from requested headers | | clang-format dry run and git diff --check | Passed | ### Limitations - Focused standalone validation, not a full top-level CMake configure/build or provider integration run. Existing Ninja recipes were adapted to compile current sources and new options into fresh objects; actual changed CMake blocks were extracted and executed with capture functions. Full target integration remains for CI. - Plugin harnesses freshly compile the test and all four contrib implementation CUs, reusing unchanged core/static libraries and CUDA support objects. Legacy freshly compiles the test and INT2 CU, reusing unchanged baseline kernel objects and standalone plugin support shims. - Only SM80 hardware was available. SM86/SM89 execution, Windows, full regression and sanitizer runs were not exercised for this follow-up. CUDA 13 legacy test compilation passed; legacy runtime execution used CUDA 12.8. - Exact local commands and compiler/test logs are recorded under `build/pr4a2-review-validation/` on the validation machine; these ignored scratch artifacts are not committed.
Gopalakrishnan Nallasamy (GopalakrishnanN)
pushed a commit
that referenced
this pull request
Oct 1, 2026
…untime/contrib_ops/cuda/moe/moe_quantization.cc. (#32932) ### Description <!-- Describe your changes. --> Replace old use of `routing_record` with `routing_snapshot_` in onnxruntime/contrib_ops/cuda/moe/moe_quantization.cc. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> Fix build error about undeclared identifier `routing_record`. Likely an unlucky merge artifact from two recent PRs #32761 and #32738. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Gopalakrishnan Nallasamy (GopalakrishnanN)
pushed a commit
to GopalakrishnanN/onnxruntime
that referenced
this pull request
Oct 1, 2026
Bring in the shared CUDA routing fix from microsoft#32932 and the updated Android minimal size baseline from microsoft#32780 and microsoft#32805. Preserve the existing model-load validation changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Gopalakrishnan Nallasamy (GopalakrishnanN)
added a commit
that referenced
this pull request
Oct 5, 2026
### Description <!-- Describe your changes. --> Promotes the experimental EPContext named-buffer callback transport to the stable ONNX Runtime 1.31 C and C++ APIs. - Adds stable read/write callback registration and immutable EP-facing snapshots of the callback and application-state pointers. - Implements review option 3: removes `max_data_size` and the proposed read-options type, constructor, getter, setter, and release APIs. Registration and retrieval carry only callback/state pointers. - Leaves allocation limits to the application callback and data validation/deserialization limits to the EP. ORT does not invoke the callback or inspect its result. The sample helper can enforce an EP-selected limit without putting that policy into the stable ABI. - Adds append-only `GetEpContextDataCallbackSupport` READ/WRITE advertisement, gated on plugin API version 31. Unsupported participating EPs are rejected before `Compile()`, including the ORT-format read path. Registered callbacks cannot silently fall back to filesystem I/O. - Removes the experimental EPContext lookup entries and C++ wrapper; experimental APIs have no stability guarantee. - Updates the C++ wrappers, provider bridge, example EP, tests, and design document. - Retains the early support query for providers that can produce external context data without `Compile()`, with VitisAI's direct-assignment lifecycle rationale documented. - Includes the upstream GPU build fix from #32932 and the previously approved 2 KiB Android size-budget adjustment. Provider/binding adoption follows in QNN (#32266), OpenVINO (#32267), C# (#32270), and Python (#32271). Those follow-ups need to use the finalized callback/state-only signatures. ### Motivation and Context <!-- - Why is this change required? What problem does it solve? - If it fixes an open issue, please link to the issue here. --> This graduates the transport introduced experimentally by #28624 and refined by #29294 and #31195. Applications can keep external EPContext data in encrypted or application-managed storage without silently falling back to plaintext filesystem I/O. Following the payload-policy review, the stable contract is limited to transport, ownership, and support negotiation. Centralized result-size validation would not itself prevent allocation inside the callback; passing a limit to the callback would still depend on callback/EP enforcement. Removing the policy avoids permanent API surface for a guarantee ORT does not provide. ### Testing Windows VS18 builds: - `cmake --build build\WindowsEpContextStable\RelWithDebInfo --config RelWithDebInfo --parallel 8 --target onnxruntime_shared_lib_test onnxruntime_autoep_test onnxruntime_test_all` — passed. - `cmake --build build\WindowsEpContextMinimal\MinSizeRel --config MinSizeRel --parallel 4 --target onnxruntime_shared_lib_test` — passed; minimal configuration disables exceptions, RTTI, and ML ops. Tests run from `build\WindowsEpContextStable\RelWithDebInfo\RelWithDebInfo`: - `.\onnxruntime_shared_lib_test.exe --gtest_filter=EpContextDataApiTest.*` — 14 passed, including immutable snapshots, cloning/lifetime, unchanged error outputs, and callback clearing. - `.\onnxruntime_autoep_test.exe --gtest_filter=*EpContext*` — 25 passed, 2 skipped for unavailable Windows symlink privileges. Covers strict support negotiation, callback/file paths, and EP-selected helper size-limit boundaries. - `.\onnxruntime_test_all.exe --gtest_filter=InternalTestingEP.*EpContext*:InternalTestingEP.Nhwc*` — 19 passed. Tests run from `build\WindowsEpContextMinimal\MinSizeRel\MinSizeRel`: - `.\onnxruntime_shared_lib_test.exe --gtest_filter=EpContextDataApiTest.*` — 2 passed. Formatting and `git diff --check` passed. No references to the removed read-options API or runtime size fields remain in tracked files. Not validated locally: GPU/Android builds, VitisAI/VAIP hardware behavior, other hardware EP integrations, or the full suite. The VitisAI lifecycle discussion remains open for reviewer confirmation. --------- Co-authored-by: Gopalakrishnan Nallasamy <gopalakrishnan.nallasamy@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Edward Chen <18449977+edgchen1@users.noreply.github.com>
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.
Description
Replace old use of
routing_recordwithrouting_snapshot_in onnxruntime/contrib_ops/cuda/moe/moe_quantization.cc.Motivation and Context
Fix build error about undeclared identifier
routing_record. Likely an unlucky merge artifact from two recent PRs #32761 and #32738.