Skip to content

[None][test] Add coverage for MiniMaxM3SparseForConditionalGeneration - #22172

Open
StanleySun639 wants to merge 2 commits into
NVIDIA:mainfrom
StanleySun639:gap-fixer/job-109-draft-182-check-153
Open

StanleySun639 wants to merge 2 commits into
NVIDIA:mainfrom
StanleySun639:gap-fixer/job-109-draft-182-check-153

Conversation

@StanleySun639

@StanleySun639 StanleySun639 commented Nov 12, 2026 •

Copy link
Copy Markdown
Collaborator

Dev Engineer Review

  • The change touches only test code and one test list. Source files under the model, attention, and registry code are unchanged.
  • The diff adds 127 lines and removes none, across 2 files. The test file adds 126 lines. The test-list file adds 1 line.
  • The new test uses internal attributes: metadata.kv_cache_params.use_cache, metadata.kv_cache_manager, model.mm_token_ids, and model.model.layers[0].self_attn.attn. A refactor of these internals can break the test without a behavior regression.
  • The test imports TrtllmAttention and TrtllmAttentionMetadata from the TRT-LLM attention backend. A rename or move of these classes breaks the test at import time.
  • The decoder-bypass check computes model.lm_head(model.model.norm(embeddings)). The assertion only shows that outputs differ. It does not show how the decoder contributes.
  • The first-token sensitivity check compares only the final-token logits (logits[-1]). Other positions are not checked.
  • The test uses max_num_requests=1, one context request (num_contexts=1, seq_lens=[3]), and KV caching disabled. Multi-request batches and the KV-cache-enabled path are not exercised.
  • Tolerances are atol=1e-2, rtol=1e-2. The test sets weights with a fixed seed, but these tolerances may need adjustment on other GPUs or kernels.

QA Engineer Review

Test files and test-list files changed:

  • tests/unittest/_torch/models/test_minimax_m3.py: adds test_registered_conditional_generation_text_forward (+126/-0). The test is marked gpu, skipped without CUDA, and decorated with torch.inference_mode().
  • tests/integration/test_lists/test-db/l0_b200.yml: adds unittest/_torch/models/test_minimax_m3.py::test_registered_conditional_generation_text_forward to the B200 pre-merge list (+1).

Behavior covered by the new test:

  • Architecture MiniMaxM3SparseForConditionalGeneration resolves to MiniMaxM3VLForConditionalGeneration.
  • The wrapper builds with no vision tower (vision_tower is None) and one decoder layer.
  • The attention module is TrtllmAttention.
  • mm_token_ids equals [62, 63].
  • Context logits have shape (3, vocab_size), dtype float32, and are finite on CUDA.
  • Token-ID input and explicit-embedding input produce matching logits.
  • Wrapper logits differ from a decoder-bypassed baseline.
  • Changing the first prompt token changes the final-token logits.

Not covered:

  • Multi-request batches.
  • The KV-cache-enabled path.
  • Logit changes at non-final positions.

Test pass status: not available from the supplied evidence. The PR description reports Jenkins run #137 as SUCCESS. That report is not verified here.

Coverage verdict: sufficient for the stated text-only forward scope. Follow-up for batched and KV-cache paths is optional.

Per-File QA Perspective

  • tests/unittest/_torch/models/test_minimax_m3.py: New CUDA-gated test for text-only registration and forward behavior. Verify that the gpu marker selects it on B200 runners.
  • tests/integration/test_lists/test-db/l0_b200.yml: Adds the new test ID to the B200 pre-merge list. The test ID matches the function name. No other entries changed.

Description

Adds unit-test coverage for existing supported model architectures.

  • tests/unittest/_torch/models/test_minimax_m3.py::test_registered_conditional_generation_text_forward verifies: The exact architecture registration resolves to MiniMaxM3VLForConditionalGeneration, and construction produces that wrapper with one real decoder layer, no vision tower, and the configured in-range multimodal token IDs. The constructed dense attention uses native TrtllmAttention, and prepared context metadata disables KV caching. Real wrapper forward returns finite CUDA float32 context logits with the configured token and vocabulary dimensions. Token-ID and explicit-embedding wrapper forwards agree numerically; the embedding call supplies no token IDs, so falling back to token lookup cannot satisfy the comparison. Wrapper logits differ from a separately computed decoder-bypassed embedding/norm/LM-head baseline. Changing only the first prompt token produces finite logits and changes the final token's logits despite leaving its own token ID unchanged.

Test Coverage

  • Test case: unittest/_torch/models/test_minimax_m3.py::test_registered_conditional_generation_text_forward
  • Jenkins log: Jenkins #137 — SUCCESS

Gap Fixer Validation

  • Patch review: Passed with no unresolved must-fix findings.
  • Analyzed source snapshot: 461580bcd5ad
  • Gap Fixer run: Job #109

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

  • PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

  • Test cases are provided for new code paths (see test instructions)

  • If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.

  • Any new dependencies have been scanned for license and vulnerabilities

  • CODEOWNERS updated if ownership changes

  • Documentation updated as needed

  • Update tava architecture diagram if there is a significant design change in PR.

  • The reviewers assigned automatically/manually are appropriate for the PR.

  • Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

Signed-off-by: Stanley Sun <stsun@nvidia.com>
@coderabbitai

coderabbitai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The change adds a CUDA-gated test for registered MiniMax M3 text-only conditional generation. The test checks model construction, attention backend selection, token IDs, and forward outputs. The B200 pre-merge PyTorch test list now includes this test.

Changes

MiniMax M3 text-only forward test

Layer / File(s) Summary
Registered model construction and forward validation
tests/unittest/_torch/models/test_minimax_m3.py, tests/integration/test_lists/test-db/l0_b200.yml
The test checks that registration resolves to the VL wrapper and that it builds without a vision configuration. It verifies the attention backend, multimodal token IDs, finite CUDA logits, equivalence of token-ID and embedding inputs, and the effects of bypassing the decoder or changing an input token. The B200 test list includes the test.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: Minimal · up to 4a5c8

This change only adds test coverage for an existing model and registers it in the B200 pre-merge list. Production behavior is unchanged, and no merge-blocking risk remains.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the main change: new test coverage for the MiniMax-M3 conditional generation model. It follows the [None][test] template format. The title names the class MiniMaxM3SparseForConditional…
Description check Passed The description covers the purpose of the new test and what it verifies. It lists the test case, a Jenkins log reference, and a Gap Fixer validation section. It keeps the PR checklist. It does not fil…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: new test coverage for the MiniMax-M3 conditional generation model. It follows the [None][test] template format. The title names the class MiniMaxM3SparseForConditionalGeneration, while the test file targets the registered MiniMaxM3VLForConditionalGeneration wrapper. The title is still related to the changeset, so it passes.

Full details: Description check

Explanation

The description covers the purpose of the new test and what it verifies. It lists the test case, a Jenkins log reference, and a Gap Fixer validation section. It keeps the PR checklist. It does not fill in the Test Coverage section fully, since it lists one test case without a short statement of why it is needed. The Description section also does not explain the issue and solution in short form, as the template asks. These gaps are minor, so the description is mostly complete and passes.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 13b996f and 4a5c82a.

Files selected for processing: 2

  • tests/integration/test_lists/test-db/l0_b200.yml
  • tests/unittest/_torch/models/test_minimax_m3.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review.

Keep the conditional-generation registry import alongside upstream quantization helpers, preserving the original forward test and B200 test-list entry.

Signed-off-by: Stanley Sun <stsun@nvidia.com>
@StanleySun639

Copy link
Copy Markdown
Collaborator Author

/bot run

@StanleySun639
StanleySun639 enabled auto-merge (squash) November 12, 2026 08:14
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #84409 [ run ] triggered by Bot. Commit: 4a5c82a Link to invocation

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants