Repository navigation
[TRTLLM-17557][test] Run the SMG single-frontend guard test in the CPU stage - #21413
Conversation
…U stage test_grpc_optional.py::test_smg_rejects_multiple_frontends_before_engine_start is marked pytest.mark.cpu_only, but the file is registered only in l0_a10.yml, whose stage deselects cpu_only tests, so this test runs nowhere in CI. Register its node id in l0_cpu.yml, after the file's five other cpu_only node ids that NVIDIA#19760 already registered there. No GPU and no model files are needed; it passes locally: 2 passed. 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 |
|---|---|
Add test to CPU integration list tests/integration/test_lists/test-db/l0_cpu.yml |
The list includes the test for rejecting multiple frontends before engine start. |
Estimated code review effort: 1 (Trivial) | ~3 minutes
Change: Other
Suggested reviewers: litaotju
Merge Risk: 
2c017
-
This registration is mergeable; it enables the intended CPU test coverage without changing product behavior.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the ticket, test change, and CPU-stage execution. It accurately summarizes the primary change. Description check The description follows the required template and explains the issue, solution, test coverage, CI bookkeeping, and checklist status. It provides sufficient technical detail for review. 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: 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 #81911 [ run ] triggered by Bot. Commit: |
|
PR_Github #81911 [ 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.
The node id matches test_smg_rejects_multiple_frontends_before_engine_start at tests/unittest/grpc/test_grpc_optional.py:359, which is marked cpu_only. That marker keeps it out of the A10 stage, so adding it here is the only way it runs in CI.
One fix to the description: the "Test Coverage" section ends mid-sentence at "Local run on". Please finish it with the environment and the pytest result (2 params passing). Without that, nothing shows the test passes in a CPU-only container before it becomes a gating L0 test.
|
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
l0_cpu.yml. No product code or test code changes.test_smg_server_startup_failure_cleans_up, as intended. No unintended scope changes are evident.QA Engineer Review
l0_cpu.ymladdsunittest/grpc/test_grpc_optional.py::test_smg_rejects_multiple_frontends_before_engine_start.Per-File QA Perspective
tests/integration/test_lists/test-db/l0_cpu.yml: Adds the test ID to the CPU CI list. This changes CI test selection; it does not change test behavior or manual-QA lists.Description
tests/unittest/grpc/test_grpc_optional.py::test_smg_rejects_multiple_frontends_before_engine_startis markedpytest.mark.cpu_only, but the file is registered only inl0_a10.yml, whose stage runs unit tests with-m "not cpu_only"— so this test currently runs nowhere in CI. #19760 registered the file's five othercpu_onlytests as node ids inl0_cpu.yml, but not this one.The test runs
trtllm-serve <model dir> --grpcwith--num_serve_frontends 4(once as a CLI flag, once through a--configYAML, 2 items) and expects a non-zero exit with "Multiple gRPC frontends require --grpc-protocol openengine" from the guard in_serve_llm, before any engine starts. No GPU and no model files are needed: it writes a minimalconfig.jsonintotmp_path, and--gpus_per_node 1keepsget_llm_argsfrom callingdevice_count().This PR adds the test's node id to
l0_cpu.ymlright after the file's last registered node id (test_smg_server_startup_failure_cleans_up), keeping the node ids in the order of the tests in the file. A node id, not a file entry, follows the pattern #19760 established for this file'scpu_onlytests.Coordination with open PRs touching the same lines or file:
l0_cpu.yml, so whichever PR merges second hits a textual conflict there (in either order; the merge is not auto-clean). Resolution: keep both lines, ordered as the tests appear in the file —...::test_openengine_pygrpc_native_is_checked_before_engine_startfirst, then this test's line.l0_cpu.yml; no conflict.Upcoming trtllm-serve refactor PRs under epic TRTLLM-17056 split
tensorrt_llm/commands/serve.pyinto a CLI package, retargeting thefrom tensorrt_llm.commands.serve import serveimport inside this test and the launcher code the test exercises; registering the test first means CI checks those updates instead of merging edits to a test that never runs.No product or test code changes — one line in one test list.
Behavior
No behavior change. The
l0_a10.ymlfile entry is untouched: the A10 stage keeps running the file's non-cpu_onlytests and keeps deselecting this one.CI bookkeeping
tests/integration/test_lists/test-db/l0_cpu.ymlgainsunittest/grpc/test_grpc_optional.py::test_smg_rejects_multiple_frontends_before_engine_startafter...::test_smg_server_startup_failure_cleans_up, beforeunittest/inputs.legacy-files.txt: no changes needed.Test Coverage
Local run on a GPU-less dev container, exactly as the CPU stage runs the entry. The Python code under test is this branch via a
PYTHONPATHoverlay —python3 -c "import tensorrt_llm; print(tensorrt_llm.__file__)"printed this checkout's path — over compiled extensions from a source build ofmainat5d86481f60(TensorRT-LLM 1.4.0rc3); the test spawns thetrtllm-serveCLI from this branch's Python.pytest -m cpu_only -rs "tests/unittest/grpc/test_grpc_optional.py::test_smg_rejects_multiple_frontends_before_engine_start" -q→2 passed, 13 warnings in 4.22s.pytest -m cpu_only -rs tests/unittest/grpc/test_grpc_optional.py -q→13 passed, 5 deselected, 24 warnings in 76.52s;-rsreported no skips. The 5 deselected items are the file's non-cpu_onlytests, which stay on the A10 stage.python3 scripts/test_to_stage_mapping.py --tests "unittest/grpc/test_grpc_optional.py::test_smg_rejects_multiple_frontends_before_engine_start"→CPU-Generic-arm-1,CPU-Generic-arm-2,CPU-Generic-x86-1,CPU-Generic-x86-2.python3 scripts/check_test_list.py --validate→OK: 2089 unique test entries validated.(one more node-id entry than before this PR);python3 scripts/check_test_list.py --l0passes.grep -c "pytest.mark.cpu_only" tests/unittest/grpc/test_grpc_optional.py→ 6. What makes this entry run (and fail loudly if it stops running): the test carries thecpu_onlymarker, so the stage's-m cpu_onlyselects it, and pytest exits with code 5 when an entry collects nothing, which fails the stage. (The literal-text grep matters for directory entries:tests/unittest/conftest.py::pytest_ignore_collectdrops marker-less files during directory traversal, but a node id passed directly on the command line, as this entry is, is collected regardless.)git diff --stat→ one file, one line added.pre-commit run --files tests/integration/test_lists/test-db/l0_cpu.yml→ all hooks pass, twice (the test-list AST validation hook checks the node id resolves to a real test).After CI: the CPU-Generic stage logs should show
test_unittests_v2[unittest/grpc/test_grpc_optional.py::test_smg_rejects_multiple_frontends_before_engine_start]with 2 items, both passed.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.
🤖 Generated with Claude Code