[TRTLLM-13696][test] Part2.2: Migrate CPU only tests - generic, models, disagg - #17290
Conversation
Signed-off-by: Yuan Tong <13075180+tongyuantongyu@users.noreply.github.com>
Signed-off-by: Yuan Tong <13075180+tongyuantongyu@users.noreply.github.com>
Signed-off-by: Yuan Tong <13075180+tongyuantongyu@users.noreply.github.com>
|
/bot run --stage-list "CPU-Generic-x86-1, CPU-Generic-arm-1" --disable-fail-fast |
|
PR_Github #63969 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #63969 [ run ] completed with state |
|
PR_Github #64004 [ run ] triggered by Bot. Commit: |
WalkthroughThe pull request marks unit tests as CPU-only, updates hardware-specific integration test lists, and adjusts selected tests for optional imports, profiler assertions, MPI mapping, and runtime configuration. ChangesCPU test scheduling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unittest/bindings/test_executor_bindings.py (1)
719-719: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a boolean assertion.
config_copy.fast_logitsis already the value under test.== Truetriggers Ruff E712 and obscures the intended check.Proposed fix
- assert config_copy.fast_logits == True + assert config_copy.fast_logitsAs per coding guidelines, keep the boolean assertion PEP 8 compliant. The static-analysis report flags this exact comparison.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/bindings/test_executor_bindings.py` at line 719, Update the assertion in the test around config_copy.fast_logits to assert the boolean value directly instead of comparing it with True, preserving the existing expected truthy behavior and satisfying Ruff E712.Sources: Coding guidelines, Linters/SAST tools
tests/unittest/others/test_mapping.py (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd return annotations to the changed functions.
Both changed functions omit return annotations. Apply the repository annotation rule at both sites.
tests/unittest/others/test_mapping.py#L25-L29: add a generator annotation such as-> Iterator[None]to_force_mpi_topology_mapping.tests/unittest/others/test_pretrained_config.py#L24-L26: add-> Nonetotest_pretrained_config_parses_runtime_defaults_correctly.As per coding guidelines,
**/*.pyrequires: “Annotate every function.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/others/test_mapping.py` around lines 25 - 29, Annotate both changed functions: add a generator return annotation such as Iterator[None] to _force_mpi_topology_mapping in tests/unittest/others/test_mapping.py (lines 25-29), and add -> None to test_pretrained_config_parses_runtime_defaults_correctly in tests/unittest/others/test_pretrained_config.py (lines 24-26).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unittest/bindings/test_executor_bindings.py`:
- Line 719: Update the assertion in the test around config_copy.fast_logits to
assert the boolean value directly instead of comparing it with True, preserving
the existing expected truthy behavior and satisfying Ruff E712.
In `@tests/unittest/others/test_mapping.py`:
- Around line 25-29: Annotate both changed functions: add a generator return
annotation such as Iterator[None] to _force_mpi_topology_mapping in
tests/unittest/others/test_mapping.py (lines 25-29), and add -> None to
test_pretrained_config_parses_runtime_defaults_correctly in
tests/unittest/others/test_pretrained_config.py (lines 24-26).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9e09e699-6238-4d7c-a539-cdf9cf25e298
📒 Files selected for processing (84)
tests/integration/test_lists/test-db/l0_a10.ymltests/integration/test_lists/test-db/l0_b200.ymltests/integration/test_lists/test-db/l0_cpu.ymltests/integration/test_lists/test-db/l0_h100.ymltests/integration/test_lists/test-db/l0_l40s.ymltests/unittest/_torch/executor/test_benchmark_disagg.pytests/unittest/_torch/memory/test_gms_backend.pytests/unittest/_torch/modeling/test_gemma4_multimodal.pytests/unittest/_torch/modeling/test_modeling_afmoe.pytests/unittest/_torch/modeling/test_modeling_llava_next.pytests/unittest/_torch/modeling/test_modeling_nemotron_nano_v2_vl.pytests/unittest/_torch/modeling/test_modeling_parakeet.pytests/unittest/_torch/modeling/test_modeling_qwen3vl.pytests/unittest/_torch/modeling/test_modeling_qwen3vl_preprocess.pytests/unittest/_torch/modeling/test_modeling_step3p7.pytests/unittest/_torch/modeling/test_nemotron_nano_preprocessing.pytests/unittest/_torch/models/checkpoints/hf/test_weight_loader.pytests/unittest/_torch/models/checkpoints/laguna/__init__.pytests/unittest/_torch/models/checkpoints/laguna/test_laguna_weight_mapper.pytests/unittest/_torch/models/checkpoints/mistral/__init__.pytests/unittest/_torch/models/checkpoints/mistral/test_config_loader.pytests/unittest/_torch/models/checkpoints/mistral/test_mistral_weight_mapper.pytests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.pytests/unittest/_torch/models/test_nemotron_h_puzzle.pytests/unittest/_torch/multimodal/test_external_embedding.pytests/unittest/_torch/multimodal/test_multimodal_mixin.pytests/unittest/_torch/multimodal/test_multimodal_runtime.pytests/unittest/_torch/multimodal/test_qwen3vl_disagg_prompt.pytests/unittest/_torch/multimodal/test_share_multiparams.pytests/unittest/_torch/test_connector.pytests/unittest/_torch/test_custom_config_registration.pytests/unittest/_torch/test_hf_quant_config.pytests/unittest/_torch/test_model_config.pytests/unittest/bindings/test_bindings_ut.pytests/unittest/bindings/test_executor_bindings.pytests/unittest/bindings/test_transfer_agent_bindings.pytests/unittest/disaggregated/region/test_aux.pytests/unittest/disaggregated/region/test_block.pytests/unittest/disaggregated/region/test_page.pytests/unittest/disaggregated/region/test_region.pytests/unittest/disaggregated/test_agent.pytests/unittest/disaggregated/test_agent_multi_backends.pytests/unittest/disaggregated/test_cache_reuse_adapter.pytests/unittest/disaggregated/test_cluster_storage.pytests/unittest/disaggregated/test_disagg_cluster_manager_worker.pytests/unittest/disaggregated/test_disagg_openai_client.pytests/unittest/disaggregated/test_disagg_utils.pytests/unittest/disaggregated/test_disaggregated_params.pytests/unittest/disaggregated/test_extractor.pytests/unittest/disaggregated/test_kv_transfer.pytests/unittest/disaggregated/test_messenger.pytests/unittest/disaggregated/test_openai_disagg_server.pytests/unittest/disaggregated/test_openai_disagg_service.pytests/unittest/disaggregated/test_peer.pytests/unittest/disaggregated/test_perf_logger.pytests/unittest/disaggregated/test_rank_info.pytests/unittest/disaggregated/test_remoteDictionary.pytests/unittest/disaggregated/test_request_id.pytests/unittest/disaggregated/test_router.pytests/unittest/llmapi/test_disagg_telemetry_launcher.pytests/unittest/metrics/test_collector.pytests/unittest/models/test_modeling_utils.pytests/unittest/models/test_quant_config_utils.pytests/unittest/others/test_bench.pytests/unittest/others/test_convert_utils.pytests/unittest/others/test_exception.pytests/unittest/others/test_export.pytests/unittest/others/test_lm_eval.pytests/unittest/others/test_lora_module_count.pytests/unittest/others/test_mapping.pytests/unittest/others/test_multimodal_registry.pytests/unittest/others/test_pretrained_config.pytests/unittest/others/test_time_breakdown.pytests/unittest/others/test_triton_kernels_vendoring.pytests/unittest/quantization/test_mode.pytests/unittest/scripts/test_check_model_registry.pytests/unittest/scripts/test_fetch_cache.pytests/unittest/scripts/test_legacy_utils.pytests/unittest/tools/test_config_database_sync.pytests/unittest/tools/test_host_profiler.pytests/unittest/tools/test_prepare_dataset.pytests/unittest/tools/test_test_to_stage_mapping.pytests/unittest/utils/test_logger.pytests/unittest/utils/test_prebuilt_whl_cpp_extensions.py
💤 Files with no reviewable changes (4)
- tests/integration/test_lists/test-db/l0_l40s.yml
- tests/integration/test_lists/test-db/l0_b200.yml
- tests/integration/test_lists/test-db/l0_h100.yml
- tests/integration/test_lists/test-db/l0_a10.yml
Signed-off-by: Yuan Tong <13075180+tongyuantongyu@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #64016 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unittest/tools/test_test_to_stage_mapping.py (1)
328-329: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not skip tests with no stage mapping.
When
tests_to_stages([test_name])returns no stages,continuebypasses the consistency checks. A missing or broken test-to-stage mapping can therefore pass this test. Assert thatstagesis non-empty and report the affected test and backend.Proposed fix
stages = stage_query.tests_to_stages([test_name]) if not stages: - continue # Skip tests that don't map to any stages + assert stages, ( + f"Test '{test_name}' with backend '{backend}' " + "does not map to any stage" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/tools/test_test_to_stage_mapping.py` around lines 328 - 329, Replace the `if not stages: continue` behavior in the test-to-stage consistency loop with an assertion that `stages` is non-empty, including the affected `test_name` and backend in the failure message. Keep the subsequent consistency checks running for mapped tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unittest/tools/test_test_to_stage_mapping.py`:
- Around line 328-329: Replace the `if not stages: continue` behavior in the
test-to-stage consistency loop with an assertion that `stages` is non-empty,
including the affected `test_name` and backend in the failure message. Keep the
subsequent consistency checks running for mapped tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 83a2705d-26d9-42c5-bb46-1ede38288121
📒 Files selected for processing (2)
tests/unittest/disaggregated/test_disagg_cluster_manager_worker.pytests/unittest/tools/test_test_to_stage_mapping.py
💤 Files with no reviewable changes (1)
- tests/unittest/disaggregated/test_disagg_cluster_manager_worker.py
|
PR_Github #64004 [ run ] completed with state |
|
PR_Github #64016 [ run ] completed with state |
Dev Engineer Review
pytest.mark.cpu_onlyat module, class, and function scope.QA Engineer Review
Test-list changes
l0_cpu.yml: Adds memory, modeling, checkpoint, multimodal, configuration, binding, disaggregated, metrics, quantization, script, tool, and logger coverage.l0_a10.yml: Removes migrated tests and updates retained modeling coverage.l0_b200.yml: Removesunittest/metrics/test_collector.py.l0_h100.yml: Removes migrated memory and disaggregated tests while retaining selected disaggregated tests.l0_l40s.yml: Removestest_llava_next_expand_prompt_token_ids_for_mm.Test-code changes
test_prepare_multimodal_inputs_accepts_tensor_encoder_outputby adding separate CPU and CUDA wrappers.test_kv_transfer.py.l0_cpu.ymldirectory or file entries.Verdict: sufficient.
Description
Move generic, models, disagg test cases don't need GPU to CPU stage.
Test Coverage
These tests pass on CPU.
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.