Repository navigation
[https://nvbugs/6981910][fix] Unwaive test_forward_error_survival on CPU - #21000
trtllm-agent wants to merge 1 commit into
Conversation
The four _validate_request tests in tests/unittest/executor/test_forward_error_survival.py failed with "AttributeError: 'types.SimpleNamespace' object has no attribute 'kv_cache_retention_config'": PyExecutor._validate_request reads request.kv_cache_retention_config and self._is_kv_manager_v2, and the test's _request() and _validating_executor() fakes did not set them. NVIDIA#20883 added both attributes to the fakes and is on main, but the CPU waiver for the whole file added by NVIDIA#20895 is still in waives.txt, so CI keeps skipping the file. This removes that waiver. Verified in the CI x86 image on a CPU-only node with main's tests and Python sources at dd75334 over the post-merge wheel from b96ba43: the file passes 21/21 in each of 3 runs; with NVIDIA#20883's two lines reverted, the same four _validate_request tests fail with the AttributeError above. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Remove CPU test waiver tests/integration/test_lists/waives.txt |
The skip-list entry for full:CPU/unittest/executor/test_forward_error_survival.py was removed. |
Priority:
Estimated code review effort: 1 (Trivial) | ~2 minutes
Change: Other
Possibly related issues
- NVIDIA/cuda-python#2077: This issue tracks tests that do not run in CI; removing the waiver allows this test to run in CPU CI.
Possibly related PRs
- NVIDIA/TensorRT-LLM#20895: This PR added the same waiver that this change removes.
- NVIDIA/TensorRT-LLM#20883: This PR added fake attributes cited as fixed before removing the waiver.
Suggested reviewers: xinhe-nv
Merge Risk: 
Minimal · up to 9bbb2
-
The forward-error survival test will run in CPU CI instead of being skipped. No issue requiring a pre-merge fix is identified.
Pre-merge checks: 
5
-
Full details: Description check
-
Explanation
The description explains the root cause, the waiver removal, the separation from PR
#18843, and detailed test coverage. The PR checklist is not included, but the required technical information is otherwise complete.
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files.
-
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.
|
Closing: main no longer waives this test, so there is nothing left to unwaive. |
Description
tests/unittest/executor/test_forward_error_survival.pyis waived for the CPU stages on main (full:CPU/...intests/integration/test_lists/waives.txt, added by #20895).Root cause:
PyExecutor._validate_requestreadsrequest.kv_cache_retention_configandself._is_kv_manager_v2. The test's_request()and_validating_executor()fakes did not set these attributes, so four_validate_requesttests failed withAttributeError: 'types.SimpleNamespace' object has no attribute 'kv_cache_retention_config'. #20883 adds both attributes to the fakes and is already on main. The waiver was not removed, so CI still skips the whole file.Fix: remove the waiver line.
Open PR #18843 (a Mamba cache manager refactor that changes 63 files and currently has merge conflicts with main) also removes this line, in one of its commits. This PR lands the removal on its own, without the refactor.
Test Coverage
Run in the CI x86 image on a CPU-only node with
pytest -m cpu_only, the way the CPU stages run it. The tests and Python sources come from main at dd75334, installed over the post-merge wheel built from b96ba43 (the compiled extensions are that build's). The test file,py_executor.pyandwaives.txthave not changed between dd75334 and this PR's base.unittest/executor/test_forward_error_survival.py: 21 passed in each of 3 runs.test_validate_request_rejects_via_model_hook,test_validate_request_accepts_when_model_hook_accepts,test_validate_request_skips_hook_for_text_only_requestsandtest_validate_request_accepts_models_without_the_hook, each with theAttributeErrorabove.unittest/executorentries ofl0_cpu.yml: 330 passed, 5 skipped. One failed:test_rpc.py::TestRpcCorrectness::test_incremental_task_asynctimed out, and it is already waived inwaives.txtunder https://nvbugs/5741476.test_base_worker.py: 30 passed.Fixes https://nvbugs/6981910
Dev Engineer Review
The change removes the CPU waiver for
test_forward_error_survival.py. The change is limited to one entry in the waiver list.QA Engineer Review
The change modifies only
tests/integration/test_lists/waives.txtand removes the waiver forfull:CPU/unittest/executor/test_forward_error_survival.py. The test file appears intests/integration/test_lists/test-db/l0_cpu.yml. The supplied PR objectives report 21 tests passed in each of three CPU-only runs. Coverage verdict: sufficient.Per-File QA Perspective
tests/integration/test_lists/waives.txt: Removing the waiver allows the test file to run without being skipped by this entry. The test file is listed in the CIl0_cpu.ymllist.