Repository navigation
[None][test] Guard lazy engine initialization and GPU-free imports - #21231
Conversation
Signed-off-by: Ruodi Lu <ruodil@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Check PDL configuration behavior tests/unittest/_torch/executor/test_model_engine_initialization.py, tests/unittest/others/test_import_gpu_free.py |
CPU-only tests check PDL configuration across model engine initializations, including retry after failure. The GPU-free import test verifies that importing model_engine does not call the PDL setter. |
Check CUDA context state after imports tests/unittest/others/test_import_cuda_context.py, tests/integration/test_lists/test-db/l0_a10.yml, tests/integration/test_lists/test-db/l0_h100.yml |
A subprocess test checks CUDA context state after importing tensorrt_llm and LLM. The A10 and H100 pre-merge test lists include this test. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Other
Suggested reviewers: litaotju
Merge Risk: 
565bd
-
The added tests cover deferred PDL setup and GPU-free imports as intended. A matching runtime build or CI run is still needed, but no merge-blocking issue is established.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title follows the repository template with the [None][test] prefix and describes two changes present in the changeset: guarding lazy engine initialization (the DeepGEMM PDL configuration tests and… Description check The description covers the purpose (splitting initialization-module test coverage out of #21095), the scope (tests and CI-list changes only), and a detailed Test Coverage section that reports local ru…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: Title check
-
Explanation
The title follows the repository template with the [None][test] prefix and describes two changes present in the changeset: guarding lazy engine initialization (the DeepGEMM PDL configuration tests and retry behavior in model engine initialization) and GPU-free imports (the import tests for CUDA context and tensorrt_llm). It is specific enough for a teammate scanning history, though it does not mention that the PR contains only tests and CI-list changes.
Full details: Description check
-
Explanation
The description covers the purpose (splitting initialization-module test coverage out of
#21095), the scope (tests and CI-list changes only), and a detailed Test Coverage section that reports local runs, rebased checks, and the blocked runtime verification. It has a Description section and a PR Checklist section. It does not include the template's@coderabbitaisummary line or the template's own guidance, and it does not state the checklist items individually. The Description section is brief, but the purpose and scope are clear, so the description is mostly complete.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. 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 4 functions across 3 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
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 |
Signed-off-by: Ruodi Lu <200874449+ruodil@users.noreply.github.com>
|
/bot run |
|
PR_Github #81674 [ run ] triggered by Bot. Commit: |
|
PR_Github #81674 [ run ] completed with state |
JunyiXu-nv
left a comment
JunyiXu-nv
left a comment
There was a problem hiding this comment.
LGTM from the runtime side. For the rebase: main renamed tests/unittest/_torch/executor to pyexecutor (#20669), so test_model_engine_initialization.py needs to move to tests/unittest/_torch/pyexecutor/.
|
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. |
Description
Guard deferred DeepGEMM PDL configuration, successful one-time setup, retry after failure, and public imports that leave CUDA contexts uninitialized. Keep the visible-GPU import check in the A10/H100 pre-merge lists.
Split the initialization module coverage out of #21095 so it can be reviewed independently. This PR contains only its tests and any associated CI-list changes.
Test Coverage
main(b7b797a3aada): Python AST, Ruff lint/format and whitespace checks passed; repository test-list validation passed.mainand lackstrtllm::k3_mnnvl_fusion_allreduce. A matching build or remote CI is still required.PR Checklist
Dev Engineer Review
The changes add tests for deferred DeepGEMM PDL setup, retry after setup failure, and imports that do not initialize CUDA contexts. They also add the CUDA-context test to the A10 and H100 pre-merge lists. No source behavior changes are reported. Current review findings and runtime results for the rebased head are unavailable.
QA Engineer Review
The test changes cover PDL configuration, GPU-free imports, and CUDA context state in subprocesses. The CUDA-context test appears in both the A10 and H100 test databases. CI or manual-QA list coverage for the other changed test modules is not established. Coverage verdict: needs follow-up. The author reports static checks passed, but runtime verification of the rebased head remains blocked pending a compatible build or remote CI.
Per-File QA Perspective
tests/integration/test_lists/test-db/l0_a10.yml: Addsunittest/others/test_import_cuda_context.pyto the A10 pre-merge list.tests/integration/test_lists/test-db/l0_h100.yml: Addsunittest/others/test_import_cuda_context.pyto the H100 pre-merge list.tests/unittest/_torch/executor/test_model_engine_initialization.py: Tests initialization state, one-time PDL setup, and retry after configuration failure. List membership is not established by the available evidence.tests/unittest/others/test_import_cuda_context.py: Tests whether imports leave visible-device CUDA contexts inactive. It is listed for A10 and H100.tests/unittest/others/test_import_gpu_free.py: Extends GPU-free import checks to cover deferred PDL configuration. List membership is not established by the available evidence.