[None][chore] ReuseScope refactor (cp #14140) - #14353
Conversation
After NVIDIA#14140 (cherry-picked in NVIDIA#14353) RootBlock no longer exposes ``lora_task_id`` / ``cache_salt_id`` directly — both fields are folded into a ``ReuseScope`` NamedTuple attached as ``root.reuse_scope``. ``KVCacheEventManager._root_attrs_from_root_block`` was still reading the legacy attribute names via ``getattr(..., None)``, so every emitted event silently received ``(None, None)`` and the V1-compat hash collapsed to the same value for any LoRA/salt request. Downstream Dynamo routing depends on those hashes, so this regression would materially degrade prefix cache hit rate for any request carrying a LoRA task id or a cache salt. Fix ``_root_attrs_from_root_block`` to prefer ``root.reuse_scope`` and fall back to the legacy attributes (keeps any in-flight pre-refactor RootBlock instances working). Also: - Update the now-stale ``test_v2_root_key_distinguishes_lora_from_cache_salt_id`` to use the new single-arg ``RootBlock.make_key(ReuseScope(...))`` API. - Extend ``_FakeRootBlock`` with an optional ``reuse_scope`` kwarg so tests can mimic both the pre- and post-refactor RootBlock shape. - Add a regression test ``test_v2_kv_cache_event_manager_v1_hash_reads_root_reuse_scope`` that asserts (a) a ReuseScope-shaped root and a legacy root with the same lora/salt produce identical V1 event hashes, (b) different scopes still produce different hashes (no silent collapse), and (c) an empty ReuseScope matches a legacy root with no salt/lora. Addresses reviewer feedback on NVIDIA#14351 (peihu-nv). Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com>
After NVIDIA#14140 (cherry-picked in NVIDIA#14353) RootBlock no longer exposes ``lora_task_id`` / ``cache_salt_id`` directly — both fields are folded into a ``ReuseScope`` NamedTuple attached as ``root.reuse_scope``. ``KVCacheEventManager._root_attrs_from_root_block`` was still reading the legacy attribute names via ``getattr(..., None)``, so every emitted event silently received ``(None, None)`` and the V1-compat hash collapsed to the same value for any LoRA/salt request. Downstream Dynamo routing depends on those hashes, so this regression would materially degrade prefix cache hit rate for any request carrying a LoRA task id or a cache salt. Fix ``_root_attrs_from_root_block`` to prefer ``root.reuse_scope`` and fall back to the legacy attributes (keeps any in-flight pre-refactor RootBlock instances working). Also: - Update the now-stale ``test_v2_root_key_distinguishes_lora_from_cache_salt_id`` to use the new single-arg ``RootBlock.make_key(ReuseScope(...))`` API. - Extend ``_FakeRootBlock`` with an optional ``reuse_scope`` kwarg so tests can mimic both the pre- and post-refactor RootBlock shape. - Add a regression test ``test_v2_kv_cache_event_manager_v1_hash_reads_root_reuse_scope`` that asserts (a) a ReuseScope-shaped root and a legacy root with the same lora/salt produce identical V1 event hashes, (b) different scopes still produce different hashes (no silent collapse), and (c) an empty ReuseScope matches a legacy root with no salt/lora. Addresses reviewer feedback on NVIDIA#14351 (peihu-nv). Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com>
c5bc871 to
a4c6725
Compare
After NVIDIA#14140 (cherry-picked in NVIDIA#14353) RootBlock no longer exposes ``lora_task_id`` / ``cache_salt_id`` directly — both fields are folded into a ``ReuseScope`` NamedTuple attached as ``root.reuse_scope``. ``KVCacheEventManager._root_attrs_from_root_block`` was still reading the legacy attribute names via ``getattr(..., None)``, so every emitted event silently received ``(None, None)`` and the V1-compat hash collapsed to the same value for any LoRA/salt request. Downstream Dynamo routing depends on those hashes, so this regression would materially degrade prefix cache hit rate for any request carrying a LoRA task id or a cache salt. Fix ``_root_attrs_from_root_block`` to prefer ``root.reuse_scope`` and fall back to the legacy attributes (keeps any in-flight pre-refactor RootBlock instances working). Also: - Update the now-stale ``test_v2_root_key_distinguishes_lora_from_cache_salt_id`` to use the new single-arg ``RootBlock.make_key(ReuseScope(...))`` API. - Extend ``_FakeRootBlock`` with an optional ``reuse_scope`` kwarg so tests can mimic both the pre- and post-refactor RootBlock shape. - Add a regression test ``test_v2_kv_cache_event_manager_v1_hash_reads_root_reuse_scope`` that asserts (a) a ReuseScope-shaped root and a legacy root with the same lora/salt produce identical V1 event hashes, (b) different scopes still produce different hashes (no silent collapse), and (c) an empty ReuseScope matches a legacy root with no salt/lora. Addresses reviewer feedback on NVIDIA#14351 (peihu-nv). Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com>
a4c6725 to
29401a9
Compare
After NVIDIA#14140 (cherry-picked in NVIDIA#14353) RootBlock no longer exposes ``lora_task_id`` / ``cache_salt_id`` directly — both fields are folded into a ``ReuseScope`` NamedTuple attached as ``root.reuse_scope``. ``KVCacheEventManager._root_attrs_from_root_block`` was still reading the legacy attribute names via ``getattr(..., None)``, so every emitted event silently received ``(None, None)`` and the V1-compat hash collapsed to the same value for any LoRA/salt request. Downstream Dynamo routing depends on those hashes, so this regression would materially degrade prefix cache hit rate for any request carrying a LoRA task id or a cache salt. Fix ``_root_attrs_from_root_block`` to prefer ``root.reuse_scope`` and fall back to the legacy attributes (keeps any in-flight pre-refactor RootBlock instances working). Also: - Update the now-stale ``test_v2_root_key_distinguishes_lora_from_cache_salt_id`` to use the new single-arg ``RootBlock.make_key(ReuseScope(...))`` API. - Extend ``_FakeRootBlock`` with an optional ``reuse_scope`` kwarg so tests can mimic both the pre- and post-refactor RootBlock shape. - Add a regression test ``test_v2_kv_cache_event_manager_v1_hash_reads_root_reuse_scope`` that asserts (a) a ReuseScope-shaped root and a legacy root with the same lora/salt produce identical V1 event hashes, (b) different scopes still produce different hashes (no silent collapse), and (c) an empty ReuseScope matches a legacy root with no salt/lora. Addresses reviewer feedback on NVIDIA#14351 (peihu-nv). Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com>
4c1b768 to
dc137ab
Compare
After NVIDIA#14140 (cherry-picked in NVIDIA#14353) RootBlock no longer exposes ``lora_task_id`` / ``cache_salt_id`` directly — both fields are folded into a ``ReuseScope`` NamedTuple attached as ``root.reuse_scope``. ``KVCacheEventManager._root_attrs_from_root_block`` was still reading the legacy attribute names via ``getattr(..., None)``, so every emitted event silently received ``(None, None)`` and the V1-compat hash collapsed to the same value for any LoRA/salt request. Downstream Dynamo routing depends on those hashes, so this regression would materially degrade prefix cache hit rate for any request carrying a LoRA task id or a cache salt. Fix ``_root_attrs_from_root_block`` to prefer ``root.reuse_scope`` and fall back to the legacy attributes (keeps any in-flight pre-refactor RootBlock instances working). Also: - Update the now-stale ``test_v2_root_key_distinguishes_lora_from_cache_salt_id`` to use the new single-arg ``RootBlock.make_key(ReuseScope(...))`` API. - Extend ``_FakeRootBlock`` with an optional ``reuse_scope`` kwarg so tests can mimic both the pre- and post-refactor RootBlock shape. - Add a regression test ``test_v2_kv_cache_event_manager_v1_hash_reads_root_reuse_scope`` that asserts (a) a ReuseScope-shaped root and a legacy root with the same lora/salt produce identical V1 event hashes, (b) different scopes still produce different hashes (no silent collapse), and (c) an empty ReuseScope matches a legacy root with no salt/lora. Addresses reviewer feedback on NVIDIA#14351 (peihu-nv). Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com>
dc137ab to
8003b8a
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #49555 [ run ] triggered by Bot. Commit: |
|
PR_Github #49555 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #49644 [ run ] triggered by Bot. Commit: |
|
PR_Github #49644 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #49705 [ run ] triggered by Bot. Commit: |
|
PR_Github #49705 [ run ] completed with state |
…4140) Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
After the ReuseScope refactor cherry-picked into this stack, ``RootBlock.make_key`` takes a single ``ReuseScope`` instead of the legacy ``(lora_task_id, cache_salt_id)`` pair, and ``sequence_to_blockchain_keys`` likewise drops the trailing ``cache_salt_id`` argument in favour of a ``ReuseScope`` positional. Two callers on the base feat/deepseek_v4 branch still used the legacy shapes and would trip the seven disaggregated/test_router.py tests that exercise the V2 hash path: - tensorrt_llm/serve/router.py — two call sites passed ``(None, cache_salt_id)`` to V2RootBlock.make_key - tests/unittest/disaggregated/test_router.py — four call sites passed raw ``None`` or a trailing ``cache_salt_id`` to sequence_to_blockchain_keys Migrate every call to ``ReuseScope(salt=cache_salt_id)`` (or ``ReuseScope()`` for the unsalted path) so the V2 prefix-hash flow matches what the radix tree now expects. Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com>
After NVIDIA#14140 (cherry-picked in NVIDIA#14353) RootBlock no longer exposes ``lora_task_id`` / ``cache_salt_id`` directly — both fields are folded into a ``ReuseScope`` NamedTuple attached as ``root.reuse_scope``. ``KVCacheEventManager._root_attrs_from_root_block`` was still reading the legacy attribute names via ``getattr(..., None)``, so every emitted event silently received ``(None, None)`` and the V1-compat hash collapsed to the same value for any LoRA/salt request. Downstream Dynamo routing depends on those hashes, so this regression would materially degrade prefix cache hit rate for any request carrying a LoRA task id or a cache salt. Fix ``_root_attrs_from_root_block`` to prefer ``root.reuse_scope`` and fall back to the legacy attributes (keeps any in-flight pre-refactor RootBlock instances working). Also: - Update the now-stale ``test_v2_root_key_distinguishes_lora_from_cache_salt_id`` to use the new single-arg ``RootBlock.make_key(ReuseScope(...))`` API. - Extend ``_FakeRootBlock`` with an optional ``reuse_scope`` kwarg so tests can mimic both the pre- and post-refactor RootBlock shape. - Add a regression test ``test_v2_kv_cache_event_manager_v1_hash_reads_root_reuse_scope`` that asserts (a) a ReuseScope-shaped root and a legacy root with the same lora/salt produce identical V1 event hashes, (b) different scopes still produce different hashes (no silent collapse), and (c) an empty ReuseScope matches a legacy root with no salt/lora. Addresses reviewer feedback on NVIDIA#14351 (peihu-nv). Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com>
63732b3 to
4bacb3c
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #49720 [ run ] triggered by Bot. Commit: |
peihu-nv
left a comment
There was a problem hiding this comment.
LGTM.
Minor follow-up later: a couple tests not in CI still use stale APIs (cache_salt_id= / make_key(None)).
|
PR_Github #49720 [ run ] completed with state |
Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com> Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com> Co-authored-by: Yao Yao <lowsfer@users.noreply.github.com>
Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com> Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com> Co-authored-by: Yao Yao <lowsfer@users.noreply.github.com>
Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com> Signed-off-by: lancelly <108499334+lancelly@users.noreply.github.com> Co-authored-by: Yao Yao <lowsfer@users.noreply.github.com>
Summary
Stacked on top of #14351. Cherry-picks #14140 onto
feat/deepseek_v4and fixes two regressions that surface once that refactor lands on the DSv4 branch.[None][chore] Refactor salting support for KVCacheManagerV2 #14140 (cherry-picked) — Refactor
cache_salt_id/lora_task_idinto aReuseScopeNamedTuple. Eliminates positional(int | None, int | None)parameters across the radix tree API. Conflict resolution against the DSv4 branch:_kv_cache_manager.py: collapse the publiccreate_kv_cache/ private_create_kv_cachesplit into a single entry point ([None][chore] Refactor salting support for KVCacheManagerV2 #14140's shape), but keep the DSv4-onlyexpected_prompt_lengthparameter._kv_cache.py: dropcache_salt_idfrom__init__(now folded intoreuse_scope), keepexpected_prompt_length.resource_manager.py: switch caller fromimpl._create_kv_cache(lora_task_id, ..., cache_salt_id=, expected_prompt_length=)toimpl.create_kv_cache(ReuseScope(lora_id=..., salt=...), input_tokens, id=request_id, expected_prompt_length=...). Preserves the DSv4IndexMapperfree-slot guard.Re-migrate router callers from
TreeTaskId→ReuseScope. PR [None][chore] Replace cache_salt_id impl with main's TreeTaskId (cp #13793) #14351'seaddc9ac22migrated callers to aTreeTaskIdseed (_KVCache._make_tree_task_id(None, cache_salt_id)); [None][chore] Refactor salting support for KVCacheManagerV2 #14140 supersedes that API, so two sites intensorrt_llm/serve/router.pyand four sites intests/unittest/disaggregated/test_router.pyneed to be re-migrated toReuseScope(salt=cache_salt_id)(orReuseScope()for the unsalted path). Drops the now-unused_KVCacheimport.Event-hash V1 compat fix (addresses @peihu-nv's review feedback on [None][chore] Replace cache_salt_id impl with main's TreeTaskId (cp #13793) #14351). After [None][chore] Refactor salting support for KVCacheManagerV2 #14140,
RootBlockno longer exposeslora_task_id/cache_salt_iddirectly — both fields live onroot.reuse_scope.KVCacheEventManager._root_attrs_from_root_blockwas still reading them viagetattr(root, "lora_task_id", None)/getattr(root, "cache_salt_id", None), so every emitted V1-compat event silently received(None, None)and the hash collapsed to the same value for any LoRA/salt request. Downstream Dynamo prefix-cache routing depends on those hashes, so this regression would materially degrade hit rate. Fix readsroot.reuse_scopefirst and falls back to the legacy attributes (keeps any in-flight pre-refactorRootBlockinstances working). Includes a regression test (test_v2_kv_cache_event_manager_v1_hash_reads_root_reuse_scope) covering: (a) ReuseScope-shaped vs legacy root produce identical V1 hashes for the same lora/salt, (b) different scopes still produce different hashes (no silent collapse), (c) emptyReuseScopematches a legacy root with no salt/lora.