[None][fix] BREAKING Block reuse policy rename and add more tests - #17277
Conversation
70e4794 to
33e2798
Compare
|
/bot run |
WalkthroughThe PR renames ChangesBlock reuse policy field rename
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 |
|
PR_Github #63923 [ run ] triggered by Bot. Commit: |
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Rename verified complete — I grepped for block_reuse_config.block_reuse_policy across .py/.md/.json/.yaml; nothing stale remains, and the manifest reorder is just alphabetical-by-path from regeneration.
One process point worth a sentence in the PR description: BlockReuseConfig inherits StrictBaseModel (extra = "forbid"), so any user or checked-in YAML still saying block_reuse_policy: now fails with a bare pydantic "extra inputs are not permitted" — no hint that the field was renamed. That's defensible for a status="prototype" field with the api-breaking label, and the immediately adjacent mamba_state_cache_interval deprecated-alias precedent argues the other way, so it's a deliberate call either way — just make it explicit rather than implicit.
Separately: docs/source/features/kvcache.md discusses the per_conversation policy (line 91) but never shows its config path, so after this PR the only doc naming the field is the generated telemetry table. The YAML snippet from the PR description would be a useful two-line addition there.
|
PR_Github #63923 [ run ] completed with state
|
Signed-off-by: Jiagan Cheng <jiaganc@nvidia.com>
33e2798 to
bf1225d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run |
|
PR_Github #64026 [ run ] triggered by Bot. Commit: |
|
PR_Github #64026 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64086 [ run ] triggered by Bot. Commit: |
|
PR_Github #64086 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64256 [ run ] triggered by Bot. Commit: |
|
PR_Github #64256 [ run ] completed with state |
Dev Engineer Review
BlockReuseConfig.block_reuse_policytopolicyacross API validation, KV-cache managers, Mamba consumers, telemetry, documentation, tests, and the golden manifest.max_num_turnsmanifest coverage.QA Engineer Review
Modified test code includes:
The modified tests are unit tests and are not listed in
tests/integration/test_lists/for CI or manual QA coverage.Verdict: sufficient.
Description
Follow-up to #16883. That PR documented the nested configuration path as
kv_cache_config.block_reuse_config.policy, but the merged implementation retained theredundant
block_reuse_policyfield name insideBlockReuseConfig.This change:
BlockReuseConfig.block_reuse_policytoBlockReuseConfig.policy;llm_args_golden_manifest.jsonfrom the corrected schema; andThe user-facing configuration is now the shape declared by #16883:
This remains an intentional breaking change: configurations that use
kv_cache_config.block_reuse_config.block_reuse_policyare rejected rather than accepted as adeprecated alias. Validation now reports that the field was renamed to
kv_cache_config.block_reuse_config.policyinstead of returning only the generic Pydanticextra-field error.
Test Coverage
upstream/mainat91fb4433c5;git range-diffconfirmed all six replayed patches are unchanged.git diff --check upstream/main...HEADpassed.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.