[TRTLLM-15078][test] Remove all DeepSeek-R1-Distill-* tests - #17351
[TRTLLM-15078][test] Remove all DeepSeek-R1-Distill-* tests#17351xinhe-nv wants to merge 8 commits into
Conversation
Remove test functions, parametrize entries, model path registrations, accuracy references, perf configs, and test list entries for all DeepSeek-R1-Distill-* model variants across unittest and integration tests. Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change removes DeepSeek-R1 accuracy, performance, quickstart, scaffolding, waiver, and end-to-end test coverage. Scaffolding tests now use ChangesDeepSeek test coverage retirement
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/bot run |
|
PR_Github #64276 [ run ] triggered by Bot. Commit: |
|
PR_Github #64276 [ run ] completed with state
|
|
/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. Details
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 --disable-fail-fast |
|
PR_Github #64331 [ run ] triggered by Bot. Commit: |
|
PR_Github #64331 [ run ] completed with state
|
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
One real breakage (inline). Otherwise the sweep looks complete — I grepped tests/ for remaining Distill references and only unrelated Cosmos3 hits came back.
Worth confirming the two pytorch_model_config.py and _model_paths.py deletions are load-bearing only for the removed perf ids; a stale entry there is harmless, but a missing one that another label still pattern-matches is not. enable_attention_dp: False was listed under a comment about preventing hangs — make sure no other model label was relying on that pattern block (it matched only deepseek_r1_distill_llama_70b, so it looks safe).
Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
…f entries - Restore create_trtllm_worker helper and add trtllm_model_path fixture pointing at gpt_oss/gpt-oss-20b; rename deepseek_distill_7b_path across test_worker.py and test_scaffolding.py so directory collection no longer fails - Drop empty isort guard block in test_worker.py - Remove deepseek_r1_distill_qwen_32b rows from base_perf_pytorch.csv Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
…lm_model_path Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
|
@coderabbitai review the test_worker.py |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
file="tests/unittest/scaffolding/test_worker.py"
echo "== File outline =="
ast-grep outline "$file" --items all || true
echo
echo "== PR diff for requested file =="
git diff --find-renames origin/main...HEAD -- "$file" || git diff --find-renames HEAD^...HEAD -- "$file"
echo
echo "== Current file =="
cat -n "$file"
echo
echo "== Model fixture and worker call sites =="
rg -n -C 4 --glob '*.py' \
'(def trtllm_model_path\b|def model_name\b|TRTLLMWorker|test_trtllm_worker_generation\b|deepseek_distill_7b_path\b|gpt-oss-20b)' \
tests/unittest/scaffoldingLength of output: 14275 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Candidate resolver files =="
fd -t f 'test_llm\.py|llm_data\.py|_model_paths\.py' tests | sort
echo
echo "== Resolver definitions and gpt-oss path references =="
rg -n -C 8 --glob '*.py' \
'(def get_model_path\b|def llm_models_root\b|gpt_oss/gpt-oss-20b|gpt-oss-20b|init_with_new_llm\s*\()' \
tests tensorrt_llm
echo
echo "== Requested-file history at its parent commit =="
git show HEAD^:tests/unittest/scaffolding/test_worker.py | cat -nLength of output: 45726
The provided CI status does not include test-level failure output. I cannot link the L0 failure to this file from the available information. You are interacting with an AI system. |
|
/bot run --disable-fail-fast |
|
PR_Github #64506 [ run ] triggered by Bot. Commit: |
| if model_name.startswith("Qwen3"): | ||
| args.extend(["--reasoning_parser", "qwen3"]) | ||
| else: | ||
| args.extend(["--reasoning_parser", "deepseek-r1"]) |
There was a problem hiding this comment.
With both DeepSeek params gone, theelse: --reasoning_parser deepseek-r1branch is now unreachable, and the deepseek-r1 parser loses all server-level coverage (unit coverage in test_reasoning_parser.py remains).
| _MEM_FRACTION_95 = 0.95 | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("model_name,model_path", [ |
There was a problem hiding this comment.
This one is an RCCA regression test for nvbugs/5238105 (n>1 must not produce empty completions) — the model is incidental and any small model would exercise the same property. It's already waived on 6 platforms, so the immediate loss is limited, but could we repoint it to e.g. TinyLlama/Qwen3-0.6B rather than delete it, or note on the bug that this regression guard is going away?
There was a problem hiding this comment.
according to the background, Qwen3-0.6B is an acceptable model.
4 participants
Remove test functions, parametrize entries, model path registrations, accuracy references, perf configs, and test list entries for all DeepSeek-R1-Distill-* model variants across unittest and integration tests.
Dev Engineer Review
trtllm_model_pathwithgpt_oss/gpt-oss-20b.QA Engineer Review
Removed test code includes:
TestDeepSeekR1DistillLlama70B.test_auto_dtype_tp2test_qwen_e2e_cpprunner_large_new_tokenstest_ptp_scaffoldingdeepseek_distill_7b_pathUpdated scaffolding helpers and tests now use
trtllm_model_path. The retained TRT-LLM worker generation test remains covered by the unit test module.Removed integration test-list entries from:
tests/integration/test_lists/qa/llm_function_core.txttests/integration/test_lists/qa/llm_spark_func.ymltests/integration/test_lists/qa/llm_spark_perf.ymltests/integration/test_lists/test-db/l0_l40s.ymltests/integration/test_lists/waives.txtThe removed integration tests had corresponding QA or CI test-list entries. The unit tests had no integration test-list entries.
Verdict: needs follow-up
CI remains failing, and no CBTS coverage data is available.
Description
The background is https://docs.google.com/spreadsheets/d/17qLRr5oQESqn1rT9LrIUnpjvhH8wcC59273YEPT9k44/edit?gid=177162358#gid=177162358
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.