[#14173][tests] move autodeploy accuracy tests to post merge and use model registry - #14352
Conversation
Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
…figs Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
8ed8683 to
f4bdb5a
Compare
📝 WalkthroughWalkthroughAutoDeploy accuracy tests are refactored to pull model-specific ChangesAutoDeploy Test Registry Integration and Curation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_lists/test-db/l0_h100.yml (1)
507-507:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconsider pre-merge vs. post-merge assignment for GLM4Flash
enable_chunked_prefillparameter.The code's
get_default_kwargsmethod includes an explicit comment: "Keep the original non-chunked variant behavior even when registry defaults enable chunked prefill." This indicates thatenable_chunked_prefill=False(non-chunked) is the original/baseline behavior. However, the test-db currently assigns:
- Pre-merge:
test_auto_dtype[trtllm-True](chunked enabled)- Post-merge:
test_auto_dtype[trtllm-False](non-chunked)Since False is the original behavior that the code explicitly emphasizes preserving, it should likely be the primary pre-merge variant, not the secondary post-merge variant. Verify whether the current assignment aligns with the intended primary vs. secondary test strategy.
🤖 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/integration/test_lists/test-db/l0_h100.yml` at line 507, The test-db currently assigns the GLM4Flash enable_chunked_prefill variants such that the chunked (True) case is pre-merge and the original non-chunked (False) case is post-merge; update the assignment so the baseline behavior (enable_chunked_prefill=False) is the primary pre-merge variant and enable_chunked_prefill=True becomes the secondary post-merge variant; locate the mapping that produces the test names test_auto_dtype[trtllm-True] and test_auto_dtype[trtllm-False] and swap their pre-merge/post-merge roles (also ensure this aligns with get_default_kwargs which documents preserving the non-chunked default).
🧹 Nitpick comments (1)
tests/integration/test_lists/test-db/l0_h100.yml (1)
505-542: Verify QA test list coverage for reorganized AutoDeploy tests.This PR reorganizes AutoDeploy accuracy tests between pre-merge and post-merge stages. Per coding guidelines, when integration tests under
tests/integration/defs/are materially altered, entries may be needed intests/integration/test_lists/qa/.Please confirm:
- Are the AutoDeploy accuracy tests already covered in
tests/integration/test_lists/qa/llm_function_core.txtor similar QA lists?- Do the QA lists need updates to ensure the primary variants (now in pre-merge) are captured for scheduled regression runs?
- Should any of the post-merge-only variants be added to QA coverage?
As per coding guidelines, integration test changes should be evaluated for QA list inclusion to ensure scheduled regression coverage.
🤖 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/integration/test_lists/test-db/l0_h100.yml` around lines 505 - 542, The AutoDeploy accuracy tests were moved between pre-merge and post-merge in the test list and you need to ensure QA regression lists include the primary variants; search current QA lists (e.g., llm_function_core.txt) for the AutoDeploy test IDs moved (examples: TestLlama3_1_8B::test_auto_dtype, TestLlama3_1_8B_Instruct_Eagle3::test_eagle3_one_model, TestNemotronV2::test_auto_dtype, TestGLM4Flash::test_auto_dtype, TestNemotronNanoV3::test_accuracy) to confirm coverage, then add any missing primary pre-merge variants to the appropriate QA list(s) so scheduled runs catch them and consider adding high-risk post-merge-only variants if they lack QA coverage; finally commit the QA list updates and run the integration list grep/validation used in CI to verify the new entries are picked up.
🤖 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.
Outside diff comments:
In `@tests/integration/test_lists/test-db/l0_h100.yml`:
- Line 507: The test-db currently assigns the GLM4Flash enable_chunked_prefill
variants such that the chunked (True) case is pre-merge and the original
non-chunked (False) case is post-merge; update the assignment so the baseline
behavior (enable_chunked_prefill=False) is the primary pre-merge variant and
enable_chunked_prefill=True becomes the secondary post-merge variant; locate the
mapping that produces the test names test_auto_dtype[trtllm-True] and
test_auto_dtype[trtllm-False] and swap their pre-merge/post-merge roles (also
ensure this aligns with get_default_kwargs which documents preserving the
non-chunked default).
---
Nitpick comments:
In `@tests/integration/test_lists/test-db/l0_h100.yml`:
- Around line 505-542: The AutoDeploy accuracy tests were moved between
pre-merge and post-merge in the test list and you need to ensure QA regression
lists include the primary variants; search current QA lists (e.g.,
llm_function_core.txt) for the AutoDeploy test IDs moved (examples:
TestLlama3_1_8B::test_auto_dtype,
TestLlama3_1_8B_Instruct_Eagle3::test_eagle3_one_model,
TestNemotronV2::test_auto_dtype, TestGLM4Flash::test_auto_dtype,
TestNemotronNanoV3::test_accuracy) to confirm coverage, then add any missing
primary pre-merge variants to the appropriate QA list(s) so scheduled runs catch
them and consider adding high-risk post-merge-only variants if they lack QA
coverage; finally commit the QA list updates and run the integration list
grep/validation used in CI to verify the new entries are picked up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c75a516c-db73-4eec-b7fa-f1414a1ba0c6
📒 Files selected for processing (5)
tests/integration/defs/accuracy/test_llm_api_autodeploy.pytests/integration/test_lists/test-db/l0_b200.ymltests/integration/test_lists/test-db/l0_dgx_b200.ymltests/integration/test_lists/test-db/l0_dgx_h100.ymltests/integration/test_lists/test-db/l0_h100.yml
Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1" --disable-fail-fast |
|
PR_Github #49409 [ run ] triggered by Bot. Commit: |
nvchenghaoz
left a comment
There was a problem hiding this comment.
I think it is good to move some tests as it can reduce the overall pre_merge time. Possibly reduce the churn in the pre_merge..
One concern, should we force the AutoDeploy dev to run all AD related post-merge tests to make sure that all accuracy tests are covered in the PR? As the failures in the post-merge will be waived..
|
PR_Github #49409 [ run ] completed with state
|
Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1" --disable-fail-fast |
|
PR_Github #49660 [ run ] triggered by Bot. Commit: |
|
PR_Github #49660 [ run ] completed with state
|
Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1" --disable-fail-fast |
|
PR_Github #50096 [ run ] triggered by Bot. Commit: |
|
PR_Github #50096 [ run ] completed with state
|
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1" --disable-fail-fast |
|
PR_Github #50111 [ run ] triggered by Bot. Commit: |
|
PR_Github #50111 [ run ] completed with state |
…d use model registry (NVIDIA#14352) Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
…d use model registry (NVIDIA#14352) Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
Summary
This PR reduces pre-merge AutoDeploy GPU time by moving variant selection to CI test lists, while standardizing relevant AutoDeploy accuracy tests to use model registry configs with explicit overrides only where needed.
What changed
1. pre-merge cost reduction
2. Registry adoption in test_llm_api_autodeploy.py
Updated selected tests to load config via model registry (yaml_extra) and then override as needed:
3. Preserved original test intent where registry defaults could change behavior
Added explicit pins to avoid unintentional behavior drift:
4. Removed redundant overrides now covered by registry
Dropped duplicated settings where registry already provides equivalent values, while keeping non-redundant behavioral overrides.
Removed as redundant (same as registry/default):
Kept intentional non-redundant overrides:
Summary by CodeRabbit
Description
Test Coverage
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-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin 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.