[kv_offload] Establish tier-owned KV event handling - #46544
Conversation
When a block is cascaded from the primary (CPU) tier to a secondary offloading tier under TieringOffloadingSpec, append a placeholder BlockStored presence event (block hash + tier medium; no self-describing token payload; no secondary BlockRemoved). take_events() now yields primary-tier (CPU) events before secondary-tier presence events. The events are gated on enable_kv_cache_events and are inert otherwise. Because TieringOffloadingSpec rejects self_describing_kv_events, the secondary keys have no metadata snapshot, so the existing OffloadingEventsTracker renders them as hash-only placeholder BlockStored events (token_ids=[], block_size=0, medium=<tier_type>). Step 1 of the multi-tier KV-event contract (vllm-project#38260). Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Define medium-name constants (MEDIUM_CPU/FS/OBJ) in kv_events.py next to
MEDIUM_GPU. CPULoadStoreSpec.medium() returns MEDIUM_CPU (behavior-identical).
Secondary tiers expose a `medium` property (mirroring CPUOffloadingManager.medium):
each built-in tier sets the EVENT_MEDIUM class attribute to its stable wire name
(FileSystemTierManager -> MEDIUM_FS, ObjectStoreSecondaryTierManager -> MEDIUM_OBJ),
and the base falls back to the registered tier_type for custom tiers. Secondary
offloading events now carry that wire medium instead of the arbitrary registered
tier-type string. Config/registry tier types are unchanged ("fs"/"obj").
Per the multi-tier KV-event discussion in vllm-project#38260.
Signed-off-by: Change72 <changg@nvidia.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the stable-wire-medium-names change: GPULoadStoreSpec.medium() returned the literal "GPU" while the sibling CPULoadStoreSpec was migrated to the MEDIUM_CPU constant. Return MEDIUM_GPU instead so all offload-spec mediums reference the kv_events.py constants. Behavior-identical (MEDIUM_GPU == "GPU"). Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @Change72 for proposing this! I think we need to cleanup first before we add secondary tier events. My proposal:
WDYT? |
Per review feedback (vllm-project#38260 discussion), introduce a medium() -> str | None accessor on both OffloadingManager and SecondaryTierManager as the source of the wire medium for Stored KV events: - OffloadingManager.medium() defaults to None; CPUOffloadingManager returns MEDIUM_CPU. SecondaryTierManager.medium() defaults to None. - Secondary tiers are opt-in: fs/obj accept enable_kv_events (default False) and return MEDIUM_FS/MEDIUM_OBJ only when enabled, else None. A tier whose medium() is None emits no secondary BlockStored; custom tiers are silent unless they override medium(). (Replaces the EVENT_MEDIUM class attribute.) - Managers still emit their own Stored/Removed events, now gated on medium(); the CPU wire output and event ordering (eviction Removed before the new Stored) are unchanged. No secondary BlockRemoved. LoadStoreSpec.medium() is kept: it is the worker transfer-dispatch key ((src.medium(), dst.medium()) in worker.py), independent of KV events, so it cannot be removed. Moving Stored-event emission out of the managers into the OffloadingConnectorScheduler is left as a follow-up, to keep this change behavior-preserving on the merged CPU path. Config/registry tier types are unchanged (fs/obj). Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This pull request has merge conflicts that must be resolved before it can be |
- The medium() docstrings/comments said the scheduler emits Stored events; in the current design the managers emit them gated on medium(). Reword to describe the manager's event path using this medium (no scheduler-emit claim). - Add lightweight tests that the real FileSystemTierManager / ObjectStoreSecondaryTierManager report MEDIUM_FS / MEDIUM_OBJ only when enable_kv_events is set (else None), exercising medium() via __new__ to avoid their heavy __init__ (the obj test skips if NIXL is unavailable). Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the conflict in vllm/v1/kv_offload/tiering/obj/manager.py by keeping both this PR's medium() opt-in accessor and main's LookupResult-based lookup(). Update the secondary-event tests to call on_new_request() before prepare_store() (main now tracks per-request state via _req_state, populated in on_new_request). Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The medium applies to both Stored and Removed events (the CPU eviction Removed event also uses medium()), so describe it as the managers KV-event medium rather than Stored-only. Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous commit accidentally reverted mains base.py changes (a stale pre-merge copy was applied, dropping LookupResult and other merged content). Restore the merged base.py and apply the intended docstring clarification: medium() applies to both Stored and Removed events (the CPU eviction Removed event also uses medium()), so it documents the managers KV-event medium rather than Stored-only. Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per the reviewer cleanup (orozery): clean up KV-event responsibility before adding secondary-tier events. - The connector (OffloadingConnectorScheduler) now emits the GPU->primary Stored KV event on complete_store, using OffloadingManager.medium(); complete_store returns the stored keys for this. - CPUOffloadingManager reports only eviction Removed events. - take_events drains the manager events (eviction Removed) before the queued Stored events, preserving the Removed-before-Stored wire order. - Remove LoadStoreSpec.medium() (+ the GPU/CPU overrides). It is no longer the worker transfer-dispatch key: since vllm-project#45053 each OffloadingWorker serves a single medium and uses submit_store/submit_load instead of (src_medium, dst_medium) routing, so it is redundant with OffloadingManager.medium(). - Drop this PR secondary-tier Stored emission (and the per-tier enable_kv_events plumbing); opt-in secondary Stored and promotion events are deferred to a follow-up. SecondaryTierManager.medium() stays (default None). A side effect: tier promotions (secondary->primary) no longer emit a phantom primary Stored event, since the connector only emits for GPU->primary store jobs. Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Gate record_store (the prepare_store metadata snapshot) on the same condition as the Stored emission: KV events enabled AND manager.medium() is not None. Previously, with self-describing events on but medium=None, metadata was captured but never consumed by a Stored or Removed event, so a custom manager could accumulate it indefinitely. - Drop MEDIUM_FS / MEDIUM_OBJ (no production callers now; they belong with the secondary-tier events follow-up) and restore TieringOffloadingManager.take_events() to its original ordering. - Make LoadStoreSpec a plain marker class; it no longer has abstract methods. - Document the engine-drain invariant in take_events. Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ned keys - complete_store path: read manager.medium() once instead of via the _emit_stored_events property and again for the event, so a custom manager whose medium() is not perfectly stable stays consistent. - Strengthen the connector Stored test: complete_store returns a sentinel key disjoint from the input store keys, and the test asserts the published BlockStored is built from that returned key (not the input keys). Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @orozery. Reworked around medium(), cleanup-first as you suggested. Point by point:
4/5/7 (opt-in secondary-tier Stored + promotion events): deferred to a follow-up PR — this one is the cleanup. I dropped the secondary emission, the per-tier enable_kv_events plumbing, and the FS/OBJ medium constants so they Single-tier CPU wire output is unchanged (primary Stored medium=CPU, eviction Removed before it). Happy to open the follow-up once this lands — does the two-PR split work for you? |
Replace the _emit_stored_events bool property with a _stored_event_medium property that returns the medium (or None). prepare_store metadata capture checks "is not None" and the complete_store emission reuses the returned medium, so both paths go through one helper (evaluated once each) and the docstring matches the implementation. Signed-off-by: Change72 <changg@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Chang Guo <changg@nvidia.com>
|
@Change72 Thanks for the quick adaptation to my cleanup suggestion.
Given all of that, I think it's better if we revert the changes I suggested. BTW we can still take down |
|
@orozery Thanks for revisiting this. I agree tier-owned events are cleaner, especially for distinguishing actual stores from no-ops and supporting multiple child media. My understanding is:
One scope question: should this PR also implement the opt-in secondary-tier |
Actual secondary tier event implementations should be on a follow-up. |
Keep Stored and Removed event production with the tier that owns the storage state. Restore CPU manager event ownership, remove connector-side synthesis and manager medium APIs, and make the tiering manager aggregate primary and secondary event streams only. Secondary-tier event implementations remain a follow-up; LoadStoreSpec.medium() stays removed as orthogonal cleanup. Signed-off-by: Change72 <changg@nvidia.com>
Keep the pre-existing enable_events constructor argument, internal event queue, and drain behavior. The ownership cleanup only adds secondary-tier delegation and does not need unrelated constructor API churn. Signed-off-by: Change72 <changg@nvidia.com>
This reverts commit c891bab. Signed-off-by: Change72 <changg@nvidia.com>
Pin the tier-owned CPU event contract: when a store reuses evicted capacity, BlockRemoved events are drained before the resulting BlockStored event. Signed-off-by: Change72 <changg@nvidia.com>
|
@orozery Updated as discussed: each tier now owns its events, while the tiering manager only aggregates them. Secondary-tier events remain a follow-up. I also kept the orthogonal |
| When set, the connector emits a Stored event with this medium on | ||
| complete_store, and the manager tags its eviction Removed events with | ||
| it. Whether events are collected at all is controlled by the connector's | ||
| enable_kv_cache_events. |
There was a problem hiding this comment.
We should clarify that Stored events are emitted automatically by the offloading connector (the manager should not emit them), while Removed events are expected to be emitted by the manager itself (via take_events).
Also, the manager CAN emit events with a different medium (for example, the tiered manager).
|
|
||
| Returns: | ||
| The keys that transitioned to stored by this call. The connector | ||
| uses these to emit a Stored KV event (see medium()). The default | ||
| implementation stores nothing and returns an empty list. | ||
| """ | ||
| return | ||
| return [] |
There was a problem hiding this comment.
Why do we need this?
The offloading connector already has all of the information it needs in the keys parameter.
| policy: OffloadPolicy = OffloadPolicy.BLOCK_LEVEL | ||
|
|
||
|
|
||
|
|
Signed-off-by: Change72 <changg@nvidia.com> Signed-off-by: Chang Guo <changg@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Or Ozeri <oro@il.ibm.com>
Signed-off-by: Change72 <changg@nvidia.com> Signed-off-by: Chang Guo <changg@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Or Ozeri <oro@il.ibm.com>
Motivation
KV event responsibility should follow storage ownership. A tier is the only component that can distinguish an actual store from a no-op and report its own placement changes. Having a parent construct child-tier events would assume a single destination medium and make the
take_events()contract harder to reason about.This PR establishes the ownership and aggregation contract. Actual secondary-tier event implementations remain a follow-up.
It also removes
LoadStoreSpec.medium(). Since #45053, eachOffloadingWorkerserves one offloaded medium and transfer direction is explicit throughsubmit_store()/submit_load(), so the spec-level medium is no longer used for dispatch.Changes
CPUOffloadingManagerresponsible for its ownBlockStoredandBlockRemovedevents.SecondaryTierManager.take_events()hook.TieringOffloadingManager.take_events()aggregate the primary tier followed by each secondary tier, without constructing events for them.MEDIUM_CPUnext toMEDIUM_GPUand use it for CPU KV events.LoadStoreSpec.medium()and the GPU, CPU, and test overrides.BlockRemoved-before-BlockStoredordering.Behavior
Existing single-tier CPU wire behavior is unchanged: the same keys and
CPUmedium are emitted, and eviction events precede the stores that reuse their capacity.Tiering deployments continue to expose primary CPU events. Secondary managers now have an event hook and aggregation path, but the built-in secondary tiers do not emit events in this PR.
Out of scope
BlockStoredimplementations and their opt-in configuration.BlockRemoved/ invalidation events.Testing
tests/v1/kv_offload/cpu/test_manager.py: 18 passed.tests/v1/kv_offload/tiering/test_tiering_offloading.py: 26 passed.Related: #38260
AI assistance
This PR was developed with assistance from Claude and Codex. I reviewed the changes and ran the tests listed above.