Repository navigation
[https://nvbugs/6899682][fix] Isolate disaggregated client outputs per test - #20139
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughClient output files for disaggregated tests are written to and verified in the test work directory. The shared output cleanup helper and its calls in three runners are removed. ChangesDisaggregated test output
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation 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. Title check The title follows the required ticket and type format and clearly describes the main change: isolating disaggregated client outputs per test. Description check The description explains the problem, root cause, fix, test coverage, and checklist status. It provides specific reproduction and validation results relevant to the change.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
/bot run |
|
PR_Github #77416 [ run ] triggered by Bot. Commit: |
|
PR_Github #77416 [ run ] completed with state
|
…r test run_client_tests() let disagg_client.py write output.json, output_streaming.json and the chat outputs into the pytest cwd, then read them back from there. QA runs its test splits as concurrent one-node jobs from one checkout on shared storage, so another test can overwrite these files between the client run and the check. In release/1.3 QA build 1937, the DeepSeek-V3-Lite helix test overwrote the output.json of test_disaggregated_mixed[TinyLlama-1.1B-Chat-v1.0]. That test then failed, although its own responses were correct. run_client_tests() now takes a required client_output_dir, and writes and checks every client output there. run_disaggregated_test() passes its per-test work_dir. Remove cleanup_output_files(). It deleted these files from the shared cwd when the aiperf, mixed-stress and cancel tests started, and none of those tests read them. Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
5a0739e to
abd40a2
Compare
|
/bot run |
|
PR_Github #77740 [ run ] triggered by Bot. Commit: |
|
PR_Github #77740 [ run ] completed with state
|
|
/bot run |
|
PR_Github #77979 [ run ] triggered by Bot. Commit: |
|
PR_Github #77979 [ run ] completed with state
|
|
/bot run |
|
PR_Github #78010 [ run ] triggered by Bot. Commit: |
|
PR_Github #78010 [ run ] completed with state |
5 participants
Description
Problem. On release/1.3,
test_disaggregated_mixed[TinyLlama-1.1B-Chat-v1.0]failed on B300 in QALLM_FUNCTION_CLUSTER_TESTbuild 1937 (nvbugs/6899682). The error wasExpected string 'The capital of Germany is Berlin' not found in output.json. The test's own client log shows correct answers in all 8 client runs. Theoutput.jsonit checked holds completions from a different model.Root cause.
run_client_tests()letdisagg_client.pywriteoutput.json,output_streaming.jsonand the chat outputs into the pytest cwd, and read them back from there. QA runs its test splits concurrently on many nodes, all from one checkout on shared storage, so they share that cwd. In build 1937,test_disaggregated_deepseek_v3_lite_bf16_tllm_gen_helixran its client phase on another node at the same time. One completion in the failingoutput.jsonis identical, character for character, to one the helix client logged.Only the TinyLlama test fails, because the DeepSeek check accepts the TinyLlama answers. A rerun on a single node cannot reproduce the failure, because no other test writes to its cwd.
Fix.
run_client_tests()takes a requiredclient_output_dir, and writes and checks every client output there.run_disaggregated_test()passes itswork_dir, a freshtempfile.mkdtemp()directory per test.cleanup_output_files(). It deletedoutput.jsonandoutput_streaming.jsonfrom the shared cwd, which could break another test's check with aFileNotFoundError. None of its callers read those files.Same change as #20140 on main.
Test Coverage
Two B300 nodes ran pytest from one cwd on shared storage, as QA does. They used the release/1.3
LLM_DOCKER_IMAGE(…-19345) and the release/1.3 L0_PostMerge build 27 wheel (14772f4). The GPU runs used an earlier revision of this PR that behaves the same as the final code; only the parameter name, the docstring and local variables differ.test_disaggregated_mixedwhile a helper rewritesoutput*.jsonin the cwd every 0.1 s with the DeepSeek output from build 1937test_disaggregated_mixed; node B runs the DeepSeek-V3-Lite helix test 3 times; same cwdoutput.jsonheld the real DeepSeek output. All 3 helix runs passed.With the fix,
test_disaggregated_overlap_transceiver_runtime_python(checksoutput_chat.json),test_disaggregated_chat_completion_tool_callsandtest_disaggregated_diff_max_tokensalso pass on B300.In the previous CI run on this PR (L0_MergeRequest_PR 177), all single-GPU disaggregated tests passed on A10 and H100. pre-commit is clean.
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
Dev Engineer Review
cleanup_output_files(). It shows formatting and docstring edits, plus a logger-call style change. The implementation change therefore cannot be confirmed from the available diff.QA Engineer Review
tests/integration/defs/disaggregated/test_disaggregated.py. No test functions or IDs are visibly added or removed in the supplied diff.UNSTABLErelease pipeline; GPU health-check coverage was not run.Per-File QA Perspective
tests/integration/defs/disaggregated/test_disaggregated.py: Intended behavior changes would isolate client output files per test and remove shared-directory cleanup, but the supplied diff does not show those changes. Existing disaggregated tests appear in CI and manual-QA lists; no new isolation test is visible.