-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[https://nvbugs/5437405][fix] qwen3 235b eagle3 ci #7000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds a new 4-GPU NVFP4 MOE test method for Qwen3-235B-A22B, updates FP8 NVLM parameterization to enable attention_dp/cuda_graph/overlap_scheduler for one case, renames the test reference to a 4-GPU variant across QA/test-db lists, removes a SKIP waiver for the old test, and inserts the new test into CI lists. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Test as test_nvfp4_4gpus
participant Config as Model Configurator
participant Model as nvfp4_moe_only
participant Eval as Evaluator
Runner->>Test: invoke with params (tp, pp, ep, attention_dp, cuda_graph, overlap_scheduler, moe_backend, eagle3)
Test->>Config: build model config (moe_backend, eagle3 flag)
Config->>Model: instantiate nvfp4_moe_only
alt eagle3 enabled
Test->>Model: route to Eagle3 decoding path
else eagle3 disabled
Test->>Model: use standard decoding path
end
Model->>Eval: run evaluations (MMLU, GSM8K)
Eval->>Runner: return results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
/bot --help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand.
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
/bot run --only-multi-gpu-test |
PR_Github #15617 [ run ] triggered by Bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
tests/integration/defs/accuracy/test_llm_api_pytorch.py
(1 hunks)tests/integration/test_lists/qa/llm_function_full.txt
(1 hunks)tests/integration/test_lists/qa/llm_function_sanity.txt
(1 hunks)tests/integration/test_lists/test-db/l0_gb200_multi_nodes.yml
(1 hunks)tests/integration/test_lists/waives.txt
(0 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.py
: Python code must target Python 3.8+
Python indentation: 4 spaces, no tabs
Maintain module namespace in imports (from package.subpackage import foo; then use foo.SomeClass())
Python file names use snake_case
Python class names use PascalCase
Python functions/methods and local variables use snake_case; variables starting with a number get k_ prefix (e.g., k_99th_percentile)
Global variables use G_ prefixed UPPER_SNAKE_CASE (e.g., G_MY_GLOBAL)
Constants use UPPER_SNAKE_CASE in Python
Avoid shadowing variables from outer scopes in Python
Initialize all externally visible members of a Python class in init
Prefer docstrings for interfaces used outside a file; comments for local code
Use Google-style docstrings for classes and functions (Sphinx-parsable)
Document attributes/variables inline with short docstrings
Avoid reflection when simple alternatives exist (e.g., prefer explicit parameters over dict(**locals()))
In try/except, catch the narrowest exceptions possible
For duck-typing with try/except, keep try body minimal and put logic in else
Files:
tests/integration/defs/accuracy/test_llm_api_pytorch.py
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
tests/integration/defs/accuracy/test_llm_api_pytorch.py
🧠 Learnings (2)
📚 Learning: 2025-07-28T17:06:08.621Z
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Applied to files:
tests/integration/test_lists/qa/llm_function_full.txt
tests/integration/defs/accuracy/test_llm_api_pytorch.py
📚 Learning: 2025-08-13T11:07:11.772Z
Learnt from: Funatiq
PR: NVIDIA/TensorRT-LLM#6754
File: tests/integration/test_lists/test-db/l0_a30.yml:41-47
Timestamp: 2025-08-13T11:07:11.772Z
Learning: In TensorRT-LLM test configuration files like tests/integration/test_lists/test-db/l0_a30.yml, TIMEOUT values are specified in minutes, not seconds.
Applied to files:
tests/integration/test_lists/test-db/l0_gb200_multi_nodes.yml
🧬 Code Graph Analysis (1)
tests/integration/defs/accuracy/test_llm_api_pytorch.py (8)
tensorrt_llm/_torch/distributed/communicator.py (2)
tp_size
(46-47)pp_size
(42-43)tensorrt_llm/llmapi/llm_args.py (4)
CudaGraphConfig
(106-163)MoeConfig
(166-194)KvCacheConfig
(929-1024)EagleDecodingConfig
(418-444)tests/integration/defs/conftest.py (1)
llm_models_root
(77-83)tensorrt_llm/llmapi/llm.py (1)
LLM
(1079-1095)tensorrt_llm/_torch/llm.py (1)
LLM
(4-9)tests/integration/defs/accuracy/accuracy_core.py (3)
MMLU
(276-290)evaluate
(147-206)evaluate
(687-697)tensorrt_llm/evaluate/mmlu.py (1)
MMLU
(51-339)tensorrt_llm/evaluate/interface.py (1)
evaluate
(81-110)
🪛 YAMLlint (1.37.1)
tests/integration/test_lists/test-db/l0_gb200_multi_nodes.yml
[error] 22-22: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (3)
tests/integration/defs/accuracy/test_llm_api_pytorch.py (1)
2416-2457
: Config choices look solid for 4-GPU TRTLLM + Eagle3.
- Disabling block reuse when Eagle3 is enabled is correct.
- Keeping cuda graph off and overlap scheduler disabled for this scenario is aligned with prior stability choices.
- MMLU and GSM8K coverage is sufficient for CI.
tests/integration/test_lists/qa/llm_function_sanity.txt (1)
102-102
: Wiring of the new 4-GPU Eagle3 path looks correct.Entry matches the new param id and test method name. Good to see this scoped in the sanity list.
tests/integration/test_lists/qa/llm_function_full.txt (1)
566-566
: Full list updated to include the 4-GPU Eagle3 variant.The entry aligns with the new test and id. If the intent is to run both TRTLLM MoE without Eagle3 and with Eagle3, keeping both entries (lines 565 and 566) is fine.
PR_Github #15617 [ run ] completed with state |
/bot run --only-multi-gpu-test |
PR_Github #15621 [ run ] triggered by Bot |
PR_Github #15621 [ run ] completed with state |
/bot run --only-multi-gpu-test |
PR_Github #15664 [ run ] triggered by Bot |
PR_Github #15664 [ run ] completed with state |
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Post-Merge-6" |
c6b9df6
to
903edb1
Compare
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Post-Merge-6" |
PR_Github #15808 [ run ] triggered by Bot |
PR_Github #15808 [ run ] completed with state |
/bot run --stage-list "GB200-8_GPUs-2_Nodes-PyTorch-Post-Merge-7" |
PR_Github #15847 [ run ] triggered by Bot |
PR_Github #15847 [ run ] completed with state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration/test_lists/test-db/l0_gb200.yml (1)
72-72
: Optional: Quote the test string to future-proof YAML parsing.Colons and brackets are valid in plain scalars, but quoting avoids accidental parse issues if formatting changes. This is optional given existing style in this file.
- - accuracy/test_llm_api_pytorch.py::TestQwen3_235B_A22B::test_nvfp4_4gpus[latency_moe_trtllm_eagle3] TIMEOUT (90) + - "accuracy/test_llm_api_pytorch.py::TestQwen3_235B_A22B::test_nvfp4_4gpus[latency_moe_trtllm_eagle3] TIMEOUT (90)"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/integration/test_lists/test-db/l0_gb200.yml
(1 hunks)tests/integration/test_lists/test-db/l0_gb200_multi_nodes.yml
(0 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/test-db/l0_gb200_multi_nodes.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T11:07:11.772Z
Learnt from: Funatiq
PR: NVIDIA/TensorRT-LLM#6754
File: tests/integration/test_lists/test-db/l0_a30.yml:41-47
Timestamp: 2025-08-13T11:07:11.772Z
Learning: In TensorRT-LLM test configuration files like tests/integration/test_lists/test-db/l0_a30.yml, TIMEOUT values are specified in minutes, not seconds.
Applied to files:
tests/integration/test_lists/test-db/l0_gb200.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (3)
tests/integration/test_lists/test-db/l0_gb200.yml (3)
72-72
: Placement check: AI summary says pre_merge, code adds post_merge. Confirm intent.The new Qwen3-235B A22B NVFP4 4-GPU Eagle3 test is added under the post_merge block, while the AI summary mentions pre_merge. If pre_merge coverage is desired, replicate/move this entry into the pre_merge section above; if it’s intentionally post_merge-only, ignore the summary.
Do you want me to open a follow-up PR to place this test in pre_merge as well (or adjust the summary)?
72-72
: TIMEOUT formatting and value are correct
- TIMEOUT is interpreted in minutes.
- 90 minutes matches other single-node heavy tests (e.g., l0_rtx_pro_6000.yml: TIMEOUT (90)).
- Syntax (
TIMEOUT (90)
) aligns with repo conventions—no changes needed.
72-72
: Selector validation complete — no updates needed.
- The class
TestQwen3_235B_A22B
and its methodtest_nvfp4_4gpus
are defined intests/integration/defs/accuracy/test_llm_api_pytorch.py
(lines 2381 and 2467).- The parameter value
"latency_moe_trtllm_eagle3"
appears in the same file (line 2464).- No conflicting or legacy selectors remain in any
test-db
orqa
lists.
Signed-off-by: bhsueh <[email protected]>
Signed-off-by: bhsueh <[email protected]>
Signed-off-by: bhsueh <[email protected]>
Signed-off-by: bhsueh <[email protected]>
7ad218c
to
d69b2ad
Compare
/bot skip --comment "only fix CI test of post merge multi-gpu" |
PR_Github #15980 [ skip ] triggered by Bot |
PR_Github #15980 [ skip ] completed with state |
Signed-off-by: bhsueh <[email protected]>
/bot skip --comment "only unwaive and adjust the CIs of multi-gpu post merge" |
PR_Github #15993 [ skip ] triggered by Bot |
PR_Github #15993 [ skip ] completed with state |
Signed-off-by: bhsueh <[email protected]> Signed-off-by: Yuxin <[email protected]>
Summary by CodeRabbit