Repository navigation
Conversation
Contributor
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #77598 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #77598 [ run ] completed with state |
moraxu
marked this pull request as draft
October 14, 2026 22:29
moraxu
force-pushed
the
user/mguzek/nvbug6800150-kv-budget-split-waives
branch
from
October 14, 2026 22:30
b7c7cff to
905d545
Compare
…fix their stale mocks The budget split tests build KvCacheCreator with object.__new__ and stub only the attributes the cost path read when each test was written. Later changes to that path started reading _cache_transceiver_config, _max_beam_width, _llm_args.sleep_config, _is_kv_cache_manager_v2, mapping.has_cp_helix() and the draft config's sparse_attention_config, so the tests failed on the CPU stages and were waived under nvbugs/6800223. Stub the missing attributes with the values KvCacheCreator.__init__ sets for these configurations, and remove the 13 waive lines. Signed-off-by: Michal Guzek <mguzek@nvidia.com>
moraxu
force-pushed
the
user/mguzek/nvbug6800150-kv-budget-split-waives
branch
from
October 14, 2026 22:35
905d545 to
30d90c7
Compare
moraxu
marked this pull request as ready for review
October 14, 2026 22:37
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #77921 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #77921 [ run ] completed with state |
Collaborator
|
[by Claude Code] @yizhang-nv Could you review this PR? Thanks! |
Collaborator
Author
|
Already fixed on main by #20399, closing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dev Engineer Review
The change removes 13 waivers for KV budget split tests and adds explicit
KvCacheCreatormock attributes. The new values match the test configurations. No production code or assertions changed. The scope is limited to test setup and waiver removal.QA Engineer Review
test_kv_cache_budget_split.pyandtests/integration/test_lists/waives.txtchanged. The waiver file removes entries forTestExternalDrafterKvDtypeand 12TestSplitGpuBudgetForDraftcases. The test file adds stubs for cache manager version, context-parallel helix state, sleep configuration, beam width, transceiver configuration, and sparse attention configuration. The reported focused run passed 60 tests; broader CI results are reported in the PR objectives, not independently verified here. Coverage verdict: sufficient for the reported test scope.Per-File QA Perspective
tests/unittest/_torch/executor/kv_cache/test_kv_cache_budget_split.py: Covers KV cache budget splitting and external-drafter dtype behavior. The supplied context does not establish whether this file appears intest-db/orqa/.tests/integration/test_lists/waives.txt: Removes 13 waivers for the KV budget split tests, enabling those tests to run. No other test-list changes are reported.Description
The tests in
tests/unittest/_torch/executor/kv_cache/test_kv_cache_budget_split.pybuildKvCacheCreatorwithobject.__new__and stub only the attributes the cost path read when each test was written. Later changes that made the cost path read another attribute broke some of them. The tests are markedcpu_only, so they run only on the CPU stages. That is where they failed, and where they were waived under nvbugs/6800223, and they kept breaking while waived. Each row below was verified by running the test file at the listed commit and at its parent:_cache_transceiver_configin_get_draft_cache_costtest_target_cost_uses_derived_layer_type_windows(nvbugs/6800150, fixed in #19028)TestExternalDrafterKvDtypewithout that stub, while all ofunittest/_torch/executorwas waivedTestExternalDrafterKvDtype(2 tests)mapping.has_cp_helix()in_get_draft_cache_cost. A bareMockanswers it truthily, so the slope gets multiplied byMock.cp_sizetest_native_full_target_cost_preserves_draft_capacity(3),test_one_model_draft_cost_uses_derived_kv_config(2)_is_kv_cache_manager_v2in_bound_unsplit_v2_draft_by_tokenstest_returns_none_when_draft_kv_zero_llm_args.sleep_configin_validate_or_fallback_kv_cache_manager_v2#19183 and #17495 are only on main. On release/1.3 the failures come from #19040 and #19273.
This PR stubs each missing attribute with the value
KvCacheCreator.__init__sets for the configuration each test models:has_cp_helix()returnsFalsefor the non-CP mappings._llm_args.sleep_configisNone,_max_beam_widthis1and_cache_transceiver_configisNone._is_kv_cache_manager_v2matches the stubbed manager class.sparse_attention_config = None.release/1.3 does not read
_llm_args.sleep_configor_is_kv_cache_manager_v2on these paths yet. Those two stubs are kept:KvCacheCreator.__init__sets both attributes on this branch too, and they keep the change correct for main, where both reads exist.It then removes the 13 waive lines filed under nvbugs/6800223. No assertions change, and no production code changes. Of the 20 waived test items, 7 fail on release/1.3 and 13 already pass (on main, 8 fail and 12 pass).
nvbugs/6800150 and nvbugs/6800223 track the same CI failure. The waives were filed under 6800223.
Test Coverage
test_kv_cache_budget_split.pywith-m cpu_onlyand no waives file: 60 passed. On release/1.3 without this change: 7 failed, 53 passed.unittest/_torch/executorrun as the CPU stage runs it (-m cpu_only --test-prefix=unittest --waives-file=tests/integration/test_lists/waives.txt, CUDA hidden), in release/1.3's CI devel image (PyTorch 26.09) with a release/1.3 CI wheel built on that image: 1614 passed, 15 skipped. Compared with release/1.3 without this change, the only difference is the 20 unwaived items, which move from skipped to passed. The 15 skips aretest_pcg_fx_fallback_policy_is_model_specific, waived under nvbugs/6831653.CPU-Generic-x86-1andCPU-Generic-arm-1, through theunittest/_torch/executorentry inl0_cpu.yml.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.