Repository navigation
[TRTLLM-17556][test] Run every harmony parsing unit test in the CPU stage - #21411
JunyiXu-nv merged 1 commit into
Conversation
…tage tests/unittest/llmapi/apps/test_harmony_parsing.py has 55 test functions (56 collected items), but the CPU test list registered only its TestStripIncompleteMessagesReporting class, so 49 of them (tool calls on the analysis channel, reasoning_content, delta merging, usage info and stream options) never ran in CI. Replace the class-level node id in l0_cpu.yml with a file-level entry so the whole file runs in the CPU stages. The whole file passes locally against current main: 56 passed, 0 skipped, 0 failed, with no test skipped as "harmony_adapter not importable". Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Register the full test file tests/integration/test_lists/test-db/l0_cpu.yml |
The test list entry now selects all tests in test_harmony_parsing.py instead of selecting only TestStripIncompleteMessagesReporting. |
Estimated code review effort: 1 (Trivial) | ~5 minutes
Suggested reviewers: litaotju
Merge Risk: 
30c7e
-
No merge-blocking issue was identified in this test-registration change.
Pre-merge checks: 
-
Check name Status Explanation Description check The description includes the required Description, Test Coverage, and PR Checklist sections. It clearly explains the issue, the file-level test-list change, expected coverage, validation commands, res… Title check The title clearly identifies the ticket, test-related change, and primary objective: running all harmony parsing unit tests in the CPU stage. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description includes the required Description, Test Coverage, and PR Checklist sections. It clearly explains the issue, the file-level test-list change, expected coverage, validation commands, results, and the absence of production behavior changes.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
|
PR_Github #81910 [ run ] triggered by Bot. Commit: |
|
PR_Github #81910 [ run ] completed with state |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
This adds the whole file with no other changes. One optional follow-up: your description says a break in a later guarded import skips all 56 tests while the stage stays green (test_harmony_parsing.py:49-55). The upcoming serve refactor renames exactly those symbols, so that silent skip is the most likely way this coverage gets lost again. Consider either dropping the try/except (these are in-tree imports, so a failure should break collection) or narrowing it to the one optional dependency it is meant to guard. Then CI fails on a rename instead of skipping the tests.
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
Dev Engineer Review
The change replaces the class-level entry in
tests/integration/test_lists/test-db/l0_cpu.ymlwith a file-level entry forunittest/llmapi/apps/test_harmony_parsing.py. This registers the test file without changing production code or test code. Review finding counts are unavailable.QA Engineer Review
Only the CPU test-list file changed. The entry now selects the entire test file instead of only
TestStripIncompleteMessagesReporting. The objectives report 56 tests collected and passing locally; CPU-stage CI confirmation is not available here. Coverage verdict: needs follow-up until CI confirms the tests run in the intended stages.Per-File QA Perspective
tests/integration/test_lists/test-db/l0_cpu.yml: The file-level entry addstest_harmony_parsing.pyto the CPU test list. Verify that the intended CPU stages run the complete file.Description
tests/unittest/llmapi/apps/test_harmony_parsing.pyhas 55 test functions (56 collected items), but the CPU test list registered only itsTestStripIncompleteMessagesReportingclass (6 functions, 7 items), so 49 functions had never run in CI: streaming and non-streaming tool calls on the analysis channel, non-functions recipients,reasoning_content, stream metadata, remaining tokens on done, delta merging, cached tokens in usage info, andstream_options. This PR replaces the class-level node id inl0_cpu.ymlwith a file-level entry, so the whole file runs in the CPU stages (CPU-Generic-x86-1/2,CPU-Generic-arm-1/2).With this change, harmony unit plus end-to-end coverage in CI goes from 38 of 87 test functions (this file's 6 registered functions (7 items), the 23 channel-validation tests, and the 9 gpt-oss end-to-end tests in
l0_h100.yml) to 87 of 87.All 49 newly registered tests pass locally against current
mainwith no test change needed (run and results under Test Coverage). Note for reviewers of future harmony renames — the file'stry/except ImportErrorguard has two failure modes: a break in the first guarded import statement (theharmony_adapternames) fails collection loudly, because a module-level type annotation still referencesHarmonyStreamState; a break in any later guarded import (theopenai_protocol,openai_serverorpostprocess_handlersnames) skips all 56 tests with "harmony_adapter not importable" and the CPU stage stays green. So a green stage alone does not prove the tests ran — run with-rsand check for that skip reason, or count the collected items in the stage log. That is also why this registration comes first: later serve-refactor changes rename exactly the symbols this file imports, and CI should fail loudly, not skip silently, when one of them breaks.This is preparation for the trtllm-serve refactor (epic TRTLLM-17056): the newly running tests pin exactly the behaviors that the planned change routing gpt-oss through the standard OpenAI chat handler will touch.
Coordination with open PRs touching this file (no action needed whichever merges first):
OpenAIServerimport) and shrinkstest_none_tokenizer_num_postprocess_workerswithout removing it, so the counts above (55 functions, 56 items) hold before and after it merges. At this PR's base the list has 11 names, all defined.l0_cpu.ymlis in a different block of the file.Behavior
No behavior change. Production code is not touched; no test file is modified. The only effect is that 49 existing unit tests start running in the CPU pre-merge stages.
CI bookkeeping
tests/integration/test_lists/test-db/l0_cpu.ymlline 260 — class-level node id replaced by the file-level entryunittest/llmapi/apps/test_harmony_parsing.py(block stays alphabetical).Test Coverage
No new test; the PR's effect is that 49 existing functions start running in CI. Everything below was run on this branch (base
mainat a9be700) withtensorrt_llm1.4.0rc3 andopenai_harmonyinstalled,LLM_MODELS_ROOTpointing at a root containingdatasets/tiktoken_vocab/o200k_base.tiktoken:LLM_MODELS_ROOT=$HOME/llm-models-ci pytest -m cpu_only -rs tests/unittest/llmapi/apps/test_harmony_parsing.py→ 56 passed, 0 skipped, 0 failed; no "harmony_adapter not importable" skip, no other skip line.test_harmony_channel_validation.py→ 79 passed (56 + 23), 0 skipped; no "not importable" and no "not available in this version" skip lines.pytest -m cpu_only --collect-only -q tests/unittest/llmapi/apps/test_harmony_parsing.py→ 56 tests collected (55 functions; one is parametrized into 2 items).HarmonyAdapter,HarmonyStreamState,_create_response_message,_create_usage_info,get_harmony_adapter,handle_streaming_response,StreamOptions,_logit_bias_to_embedding_bias,OpenAIServer,ChatCompletionPostprocArgs,chat_harmony_streaming_post_processor) is defined in its module onmain(git show origin/main:tensorrt_llm/serve/<module>.py | grep -c -E "^(async )?(def|class) <name>\b"→ 1 for each), so the module is not being skipped today.python scripts/check_test_list.py --validate→ OK, 2087 unique test entries validated.python scripts/check_test_list.py --l0→ passes.python scripts/test_to_stage_mapping.py --tests "unittest/llmapi/apps/test_harmony_parsing.py"→CPU-Generic-arm-1,CPU-Generic-arm-2,CPU-Generic-x86-1,CPU-Generic-x86-2.grep -c "pytest.mark.cpu_only" tests/unittest/llmapi/apps/test_harmony_parsing.py→ 1. The stage passes a file-level entry's path to pytest directly, so what keeps the entry honest is the realcpu_onlymark under-m cpu_only(an unmarked module would have every test deselected) together with pytest exiting with code 5 when nothing is selected, which fails the stage loudly.pytest -m cpu_only tests/unittest/llmapi/apps tests/unittest/disaggregated -q→ 3676 passed, 4 failed, 1 skipped, 2 xfailed. The 4 failures are local-environment artifacts, not related to this change (which touches no Python):test_render_standalone.py::TestCpuOnlyStartup::test_the_launcher_loads_a_cuda_stub_only_when_the_driver_is_missingshells out to thetrtllm-renderconsole script and passes once the script is onPATH; the threetest_cluster_storage.py::TestEtcdClusterStorage::test_watch*tests are timing-sensitive and pass or fail non-deterministically on the shared box used for the run (each passed in at least one re-run).pytest tests/unittest/api_stability -q→ 64 passed.To confirm in this PR's CI: the CPU stage logs show
test_harmony_parsing.pywith 56 items run and passed (not a smaller count, which would mean a silent collection gap).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.🤖 Generated with Claude Code