Skip to content

[subscriber] add KVCache event subscriber - #256

Open
Martin7-1 wants to merge 1 commit into
mainfrom
feat/kvcache-event-subscriber
Open

[subscriber] add KVCache event subscriber#256
Martin7-1 wants to merge 1 commit into
mainfrom
feat/kvcache-event-subscriber

Conversation

@Martin7-1

Copy link
Copy Markdown
Collaborator

Summary

  • Add a standalone Python process subscriber/ (managed by uv, independent of the parent Bazel build) that subscribes to inference engine KV cache events and forwards them to the kvcm service.
  • Engine adapters behind AbstractEngineAdapter: vLLM incremental (ZMQ pub/replay with seq-gap replay via DEALER socket) and snapshot pipelines, plus an SGLang placeholder adapter; the event loop is engine-agnostic.
  • Health integration with DashServing ("live and die together"): engine liveness driven by GetWorkerStatus.alive, EngineHealthCoordinator epoch gating for sends, seq_id-linearized state reporting, idempotent AllBlocksCleared per sendable epoch.
  • kvcm side: client, manager client, service discovery, event payload expansion; KVCM is a lossy bypass — unavailability never blocks forwarding.
  • Observability: per-stage span timing (StageTimer), unified metrics via subscriber/metrics/ with kvcache_subscriber_ prefix, cataloged in docs/metrics.json and enforced by test_metrics_catalog.py.
  • Hand-maintained protobuf pb files compatible with protobuf 3.20.3 (no runtime_version/_builder); kv_cache_group_metadata.proto remains the authoritative wire schema.

Why

KVCache reuse requires the kvcm service to learn block store/remove events from co-located inference engines in real time. This subscriber decouples that forwarding from the engine and from kvcm availability, with explicit health semantics so DashServing never routes traffic to a node whose events are not being forwarded.

Validation

  • uv run pytest — 523 passed
  • uv run ruff check subscriber/ tests/ — all checks passed
  • uv run mypy subscriber/ — no issues in 48 source files

chatgpt-codex-connector[bot]

This comment was marked as outdated.

qoderai[bot]

This comment was marked as resolved.

@github-actions github-actions Bot added the ai reviewed AI has reviewed this PR label Jul 29, 2026
@Martin7-1
Martin7-1 force-pushed the feat/kvcache-event-subscriber branch from 41e3803 to 4e31383 Compare July 29, 2026 06:12
@Martin7-1

Copy link
Copy Markdown
Collaborator Author

@codex review

@Martin7-1
Martin7-1 force-pushed the feat/kvcache-event-subscriber branch from 4e31383 to 65c2f9f Compare July 29, 2026 06:18
chatgpt-codex-connector[bot]

This comment was marked as outdated.

@Martin7-1
Martin7-1 force-pushed the feat/kvcache-event-subscriber branch from 65c2f9f to c143343 Compare July 30, 2026 03:16
@Martin7-1

Copy link
Copy Markdown
Collaborator Author

@codex review

chatgpt-codex-connector[bot]

This comment was marked as outdated.

@Martin7-1
Martin7-1 force-pushed the feat/kvcache-event-subscriber branch from c143343 to 4db5d13 Compare July 31, 2026 07:02
@Martin7-1

Copy link
Copy Markdown
Collaborator Author

@codex review

chatgpt-codex-connector[bot]

This comment was marked as outdated.

@Martin7-1
Martin7-1 force-pushed the feat/kvcache-event-subscriber branch from 4db5d13 to c2b1280 Compare August 13, 2026 06:56

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2b1280d83

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

# kvcm SDK
kvcm_heartbeat_interval_s: float = 5.0
kvcm_request_timeout_s: float = 5.0
kvcm_query_type: str = KvcmQueryType.QT_PREFIX_MATCH_WITH_MAMBA

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Set mamba matching only when mamba groups exist

With the default config, every registration advertises QT_PREFIX_MATCH_WITH_MAMBA; when the engine metadata has only full-attention/default groups, KVCM's GetHostCacheState falls back to this default for unspecified queries and PrefixMatchWithMambaByHost rejects instances that have no mamba-state group. Ordinary full-attention deployments that rely on the registered default therefore get BADARGS/no hosts unless they override this option, so derive the default from the descriptor or default to QT_PREFIX_MATCH when no mamba component is registered.

Useful? React with 👍 / 👎.

Comment on lines +9 to +11
BUILTIN_ATTENTION_TYPE_CATEGORIES: Final[Mapping[str, str]] = MappingProxyType(
{
"full_attention": "F",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use KVCM-compatible full group names

When a group-aware model is registered against the KVCM code in this commit's base, full-attention groups are named F0/F1, but kv_cache_manager/manager/meta_searcher.cc classifies full groups only when the location-spec group name starts with full or FULL. For QT_PREFIX_MATCH_WITH_MAMBA, KVCM then sees no full group and rejects host-cache queries even though the locations were reported; use names KVCM recognizes or land the corresponding server-side classifier change with this subscriber.

Useful? React with 👍 / 👎.

Comment on lines +104 to +106
valid_component_ids={
component.component_id for component in bootstrap.components
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Bypass component filtering for empty bootstraps

When the accepted vLLM bootstrap has an empty components tuple (the metadata contract treats this as a valid default-spec topology), this passes an empty set rather than disabling validation. VllmIncrementalSource treats any non-None set as authoritative, so every BlockStored/BlockRemoved with a missing or any group_idx is dropped even though KVCM registered the fallback default spec; pass None when there are no components or map those events to the default spec.

Useful? React with 👍 / 👎.

Comment on lines +92 to +94
self._adapter = adapter or AbstractEngineAdapter.create(
config.engine_type, config
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Report failed for unsupported engine types

When --engine-type is unsupported (for example a typo), these lines call AbstractEngineAdapter.create() in the lifecycle constructor, which raises KeyError before _state_reporter is constructed and before _graceful_startup()'s _FatalStartupError handler can call report_failed(). The CLI only logs and re-raises, so DashServing never receives the terminal failed state for this unsupported startup; validate engine_type up front or create the adapter after the reporter is available and translate the error into the fatal path.

AGENTS.md reference: subscriber/AGENTS.md:L273-L273

Useful? React with 👍 / 👎.

},
)
return effective
block_size = self._config_int(self._engine_config, "block_size")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not default missing block size to one

When the engine returns no component metadata and DS_LLM_ENGINE_CONFIG is absent or has an invalid block_size, this silently registers the instance and fallback location spec with block size 1. KVCM stores that block size and the subscriber also folds it into the instance id, so normal deployments using 16/32-token blocks can register/report under a different instance and block geometry than clients query; fail startup or obtain the real engine block size instead of defaulting to 1.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai reviewed AI has reviewed this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant