[None][test] Adjust perf test cases to avoid OOM and remove outdated test cases - #17303
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe performance configuration replaces legacy model mappings and specialized cases with current variants. It updates remote-code handling and reorganizes workload coverage across GPU-specific conditions. ChangesPerformance configuration and test matrix
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
The list moves look right and I verified the cleanup is safe: none of the 13 aliases dropped from Two things that aren't OOM changes and shouldn't ride along silently: 1. The sampler label fix turns non-greedy sampling ON for a live case. 2. Non-blocking: Holding for now on (1) and (2); everything else looks good to me. |
…tions in performance tests - Removed unused model paths from _model_paths.py. - Updated sampler_options_config.py to reflect changes in model labels. - Cleaned up pytorch_model_config.py by removing obsolete DeepSeek R1 model configurations. These changes streamline the model configuration files and ensure they only contain relevant entries. Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
…3.5 and deepseek models, including reordering and adding new tests for improved performance benchmarking. Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
|
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. |
|
/bot run |
|
PR_Github #64032 [ run ] triggered by Bot. Commit: |
… paths and test cases - Cleaned up _model_paths.py by removing obsolete model entries. - Updated pytorch_model_config.py to eliminate references to deprecated models. - Refined llm_perf_core.yml by removing outdated test cases for improved clarity and performance benchmarking. These changes enhance the maintainability of the performance testing framework. Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
|
The head moved a long way since my last pass, and this one is a much bigger change than "avoid OOM and remove outdated test cases". Counting it mechanically at
Nine model families lose all perf coverage, and their aliases come out of
I checked every list under DeepSeek-V3.2, MiniMax-M2.5 and Kimi-K2-Thinking aren't outdated, so I don't think the title covers them. Two readings and the diff doesn't tell me which:
Which one? The OOM-driven part I have no problem with — the condition re-split (new Also: thanks for dropping the |
|
PR_Github #64032 [ run ] completed with state
|
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Deletions check out — I grepped tests/ for every removed model key and every removed pytorch_model_config pattern (including the deepseek_v3_lite chunked-prefill block) and found no dangling references, and the surviving 'deepseek_r1' pattern still substring-matches deepseek_r1_0528_* so those defaults are unchanged. Condition renumbering is consistent, no duplicated case across overlapping blocks.
The two re-gating moves are each half-applied; details inline. Both are cheap to fix and worth fixing now, since the whole point of the PR is that the gating be trustworthy.
|
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. |
|
/bot skip --comment "only test list modify" |
|
PR_Github #64182 [ skip ] triggered by Bot. Commit: |
|
PR_Github #64182 [ skip ] completed with state |
|
Re-counted at
The 70B aliases are still in This is now approved with CI skipped as "only test list modify", so it can land any moment, and my earlier question is still open. Could you either split the coverage removals out, or say per family why each is obsolete/replaced? The OOM half — the new |
Description
8192,1024case to B300 while retaining the shorter shared B200/B300 case.Test Coverage
python3 scripts/check_test_list.py --validatePR Checklist
Dev Engineer Review
qwen3_0.6breduction from 256 to 64 requests needs a timeout-related rationale because it creates a new test ID without a baseline.starcoder2_3b,qwen_14b_chat, andqwen2_7b_instructremain inallowed_configs.py. Remove them or document their purpose.QA Engineer Review
tests/integration/test_lists/qa/llm_perf_core.yml.