Add tiering offloading metrics - #48798
Conversation
|
This pull request has merge conflicts that must be resolved before it can be |
fbf37fe to
87660eb
Compare
orozery
left a comment
There was a problem hiding this comment.
Thanks @Srinivasoo7 !
Great work!!
| for metric_name, documentation in ( | ||
| ( | ||
| TieringOffloadingMetrics.READ_BYTES, | ||
| "Total bytes read from secondary tiers into the primary tier.", |
There was a problem hiding this comment.
Maybe it's a good idea to add a short note in each description about the metrics supporting per-tier labels?
There was a problem hiding this comment.
Addressed in 2addc9cc8: added notes to the tier-labeled metric descriptions that they are labeled by tier.
| ), | ||
| ) | ||
| ) | ||
| for metric_name, documentation in ( |
There was a problem hiding this comment.
Just wondering, why are we splitting this loop from the previous one?
Another option is just avoid loops at all (and pay the price for the labelnames=("tier",), line per each metric.
There was a problem hiding this comment.
Addressed in 2addc9cc8: removed the grouped loops and declared these metric definitions explicitly.
| self._transfer_jobs: dict[JobId, JobMetadata] = {} | ||
| self._transfer_job_tiers: dict[JobId, SecondaryTierManager] = {} |
There was a problem hiding this comment.
Let's consolidate _transfer_job_tiers with _transfer_jobs as follows:
- Rename
JobMetadatatoTransferJob. - Introduce
_JobMetadataas a named tuple wrapping aTransferJobandtier_idx: int. - Replace
self._transfer_jobsandself._transfer_job_tierswithself._jobs: dict[JobId, _JobMetadata]
There was a problem hiding this comment.
Addressed in 2addc9cc8: renamed JobMetadata to TransferJob, added _JobMetadata(transfer_job, tier_idx), and consolidated tracking into self._jobs.
| @staticmethod | ||
| def _tier_label(tier_idx: int, tier: SecondaryTierManager) -> tuple[str]: | ||
| return (f"{tier_idx}:{tier.tier_type}",) |
There was a problem hiding this comment.
| @staticmethod | |
| def _tier_label(tier_idx: int, tier: SecondaryTierManager) -> tuple[str]: | |
| return (f"{tier_idx}:{tier.tier_type}",) | |
| @functools.lru_cache(maxsize=None) | |
| def _tier_label(self, tier_idx: int) -> tuple[str]: | |
| return (f"{tier_idx}:{self.secondary_tiers[tier_idx].tier_type}",) |
There was a problem hiding this comment.
Addressed in 2addc9cc8: _tier_label() now uses the manager secondary tier list and caches labels. I used functools.cache, which is equivalent to lru_cache(maxsize=None) here.
| if tier is exclude_tier: | ||
| continue | ||
| labelvalues = self._tier_label(i, tier) | ||
| self._stats.increase_counter( |
There was a problem hiding this comment.
We should increase queries/hit counter only once per [req][block][tier].
Let's do this:
Suggested refactor: per-request lookup metrics dedup in the tiering manager
Tier indexing
Secondary tiers use 0-based tier_idx from enumerate(self.secondary_tiers).
Labels follow "{idx}:{type}" format with +1 offset: "0:primary", "1:fs", "2:p2p".
TierLabel = tuple[str] # e.g. ("0:primary",) or ("1:fs",)
_PRIMARY_TIER_LABEL: TierLabel = ("0:primary",)
@functools.lru_cache(maxsize=None)
def _tier_label(self, tier_idx: int) -> TierLabel:
return (f"{tier_idx + 1}:{self.secondary_tiers[tier_idx].tier_type}",)Define _RequestState in the tiering manager
@dataclass
class _RequestState:
# Keys whose resolution (HIT/MISS) has already been counted, grouped by tier label.
# When set to None, the request has been allocated — no further metrics are emitted.
observed_lookups: dict[TierLabel, set[OffloadKey]] | None = field(default_factory=dict)Stored on the manager as self._request_states: dict[str, _RequestState].
Usage in lookup()
state = self._request_states[req_context.req_id] # created in on_new_request
# --- Primary tier ---
primary_result = self.primary_tier.lookup(key, req_context)
if primary_result is LookupResult.HIT or primary_result is LookupResult.MISS:
self._observe_lookup(state, self._PRIMARY_TIER_LABEL, key, primary_result)
if primary_result is LookupResult.HIT:
return LookupResult.HIT
if primary_result is LookupResult.HIT_PENDING:
return LookupResult.HIT_PENDING
# --- Secondary tiers (tier_idx 0-based) ---
any_retry = False
for tier_idx, tier in enumerate(self.secondary_tiers):
if tier is exclude_tier:
continue
result = tier.lookup(key, req_context)
if result is LookupResult.HIT or result is LookupResult.MISS:
self._observe_lookup(state, self._tier_label(tier_idx), key, result)
if result is LookupResult.HIT:
# initiate promotion...
return LookupResult.RETRY
elif result is LookupResult.RETRY:
any_retry = True
if any_retry:
return LookupResult.RETRY
return LookupResult.MISSHelper:
def _observe_lookup(
self, state: _RequestState, label: TierLabel, key: OffloadKey, result: LookupResult
) -> None:
if state.observed_lookups is None:
return
tier_set = state.observed_lookups.get(label)
if tier_set is not None and key in tier_set:
return
state.observed_lookups.setdefault(label, set()).add(key)
self._stats.increase_counter(TieringOffloadingMetrics.BLOCK_QUERIES, labelvalues=label)
if result is LookupResult.HIT:
self._stats.increase_counter(TieringOffloadingMetrics.BLOCK_HITS, labelvalues=label)Lifecycle
on_new_request: create_RequestState()inself._request_states[req_id].on_schedule_end: for each req innew_req_ids→ setobserved_lookups = None.
(Preempted requests: no action — staysNone. Metrics are only observed on the first allocation attempt.)on_request_finished: delete from_request_states.
Summary
_RequestState serves one purpose: dedup metrics. BLOCK_QUERIES and BLOCK_HITS are
counted once per unique (tier, key) resolution, and only before the request's first
allocation. No lookup caching — tiers are always queried, remaining the source of truth.
Tier labels use "{idx}:{type}" format with primary at 0 and secondaries at 1+.
There was a problem hiding this comment.
Addressed in 2addc9cc8: lookup metrics are now deduped per request/block/tier, include the primary tier label, and stop after the first allocation.
|
|
||
| @override | ||
| def get_finished_jobs(self) -> Iterable[JobResult]: | ||
| """ | ||
| Collect completed jobs from the finished-jobs queue. | ||
| """ | ||
| results = [] | ||
| now = time.monotonic() |
There was a problem hiding this comment.
We want the read/write time to be just the IO time, not including the time the job waits until its collected, and the time until its completion is collected.
This means in the DualQueueThreadPool._worker, time before and after task().
Need to change DualQueueThreadPool.get_finished to return not just the success bool but also the time it took.
There was a problem hiding this comment.
Addressed in 2addc9cc8: FS timing now happens around task() inside DualQueueThreadPool._worker, and get_finished() returns the accumulated transfer time.
| job_id=job_id, | ||
| success=success, | ||
| transfer_size=entry.transfer_size, | ||
| transfer_time=time.monotonic() - entry.submitted_at, |
There was a problem hiding this comment.
We should use agent.get_xfer_telemetry(handle) to get the time.
No need to extend TransferEntry.
There was a problem hiding this comment.
Addressed in 2addc9cc8: object tier now uses agent.get_xfer_telemetry(handle) for transfer time and no longer stores timing fields in TransferEntry.
| job_id=job_id, | ||
| success=success, | ||
| transfer_size=self._job_transfer_sizes.pop(job_id, None), | ||
| transfer_time=transfer_time, |
There was a problem hiding this comment.
Like obj, we should use agent.get_xfer_telemetry(handle)
This requires some more plumbing to expose via DataTransport.
I suggest we leave this as a follow-up to @liranschour .
There was a problem hiding this comment.
Addressed in 2addc9cc8: removed the local P2P submit-time approximation and left P2P telemetry as follow-up, per your suggestion.
| @@ -55,6 +70,8 @@ class JobResult: | |||
|
|
|||
| job_id: JobId | |||
| success: bool | |||
| transfer_size: int | None = field(default=None, compare=False) | |||
There was a problem hiding this comment.
Let's remove this, and instead have the tiering manager compute this size based on the number of keys in the job and the CPU page size.
There was a problem hiding this comment.
Addressed in 2addc9cc8: removed JobResult.transfer_size; the tiering manager now computes bytes from the job key count and primary block size.
|
Hi @orozery Thanks |
@Srinivasoo7 I went over it when you pushed it but I thought it's still WIP since I still see major things not addressed from the previous review. |
My bad @orozery, it's in my local. Just realized. Will review and it's coming your way soon. Thanks! |
|
This pull request has merge conflicts that must be resolved before it can be |
| @@ -629,27 +800,29 @@ def create_store_job( | |||
| self, | |||
| keys: Collection[OffloadKey], | |||
| req_context: ReqContext, | |||
| ) -> JobMetadata: | |||
| tier_idx: int | None = None, | |||
There was a problem hiding this comment.
Let's remove the None option.
| # True: secondary → primary (promotion) | ||
| # False: primary → secondary (cascade) | ||
| self._transfer_jobs: dict[JobId, JobMetadata] = {} | ||
| self._jobs: dict[JobId, _JobMetadata] = {} | ||
| self._tier_states: list[_TierState] = [ |
There was a problem hiding this comment.
Can we assert _tier_states is clean at the end of reset_cache?
|
|
||
| def _observe_lookup( | ||
| self, | ||
| req_state: RequestState | None, |
There was a problem hiding this comment.
Let's remove the None option (and assert not None in the caller)
| primary_write_block_ids: set[int] = field(default_factory=set) | ||
| primary_read_block_ids: set[int] = field(default_factory=set) |
There was a problem hiding this comment.
Let's switch those to integers as well (and assert they are non-negative whenever decreasing).
| # True: secondary → primary (promotion) | ||
| # False: primary → secondary (cascade) | ||
| self._transfer_jobs: dict[JobId, JobMetadata] = {} | ||
| self._jobs: dict[JobId, _JobMetadata] = {} |
There was a problem hiding this comment.
Can we assert _jobs is empty after reset_cache?
| @@ -748,6 +921,7 @@ def on_schedule_end(self, context: ScheduleEndContext) -> None: | |||
| state = self._req_state.get(req_id) | |||
| if state is None: | |||
| continue | |||
| state.observed_lookups = None | |||
There was a problem hiding this comment.
Can we add a test that checks no metrics are observed after allocation?
91e3358 to
56acf8f
Compare
orozery
left a comment
There was a problem hiding this comment.
Thanks @Srinivasoo7 !
I see the metrics code adds quite a bit code to tiering/manager.py.
I asked Claude to plan moving it to tiering/metrics.py:
Suggested refactor: extract TieringMetricsTracker into tiering/metrics.py
Motivation
The tiering manager's metrics code (~130 lines) has a cohesive responsibility
that can be cleanly separated. Extracting it:
- Reduces
manager.pyto pure orchestration logic - Makes the metrics tracker independently testable
- Gives a single class ownership of all per-request metrics state
New file: vllm/v1/kv_offload/tiering/metrics.py
Types and constants
TierLabel = tuple[str]
_PRIMARY_TIER_LABEL: TierLabel = ("0:primary",)Internal state (private to the module)
@dataclass(slots=True)
class _RequestMetricsState:
observed_lookups: dict[TierLabel, set[OffloadKey]] | None = field(default_factory=dict)
sync_lookup_delay: float = 0.0
secondary_lookup_start_time: float | None = None
@dataclass
class _TierState:
active_promotion_count: int = 0
active_cascade_count: int = 0
primary_write_block_count: int = 0
primary_read_block_count: int = 0Class
class TieringMetricsTracker:
def __init__(
self,
tier_types: list[str], # [tier.tier_type for tier in secondary_tiers]
num_primary_blocks: int,
primary_block_size: int,
): ...
# ─── Label helpers ───
@functools.cache
def tier_label(self, tier_idx: int) -> TierLabel:
"""Returns e.g. ("1:fs",) for tier_idx=0 with tier_type="fs"."""
...
@property
def primary_tier_label(self) -> TierLabel: ...
# ─── Request lifecycle ───
def on_new_request(self, req_context: ReqContext) -> None:
"""Create _RequestMetricsState entry."""
def on_request_allocated(self, req_context: ReqContext) -> None:
"""Flush accumulated sync delay histogram.
Flush async delay histogram: time.monotonic() - secondary_lookup_start_time.
Set observed_lookups = None (no further lookup metrics emitted)."""
def on_request_finished(self, req_context: ReqContext) -> None:
"""Flush remaining delays, delete entry."""
# ─── Lookup ───
def on_lookup(
self,
req_context: ReqContext,
key: OffloadKey,
tier_label: TierLabel,
result: LookupResult,
elapsed: float,
) -> None:
"""Called once per tier.lookup() invocation (primary and each secondary).
elapsed: wall-clock time of this tier's lookup call.
Internally:
- Dedup (key, tier_label) via observed_lookups; emit BLOCK_QUERIES /
BLOCK_HITS counters
- Accumulate elapsed into sync_lookup_delay
- On first secondary HIT for this request: record
secondary_lookup_start_time = time.monotonic() (async delay start)
"""
# ─── Job lifecycle ───
def on_job_registered(self, job_metadata: JobMetadata) -> None:
"""Increment _TierState active counts and block counts."""
def on_job_finished(self, job_metadata: JobMetadata, result: JobResult) -> None:
"""Decrement _TierState counters AND emit completion metrics
(transfer bytes/time on success, failure counter otherwise)."""
# ─── Singular events ───
def on_promotion_allocation_failure(self) -> None:
"""Increment PROMOTION_ALLOCATION_FAILURES."""
# ─── Stats / assertions ───
def take_stats(self) -> OffloadingConnectorStats | None:
"""Drain buffered stats + emit active transfer gauges."""
def assert_idle(self) -> None:
"""Assert all _TierState counters are zero. Called by reset_cache."""Changes to manager.py
RequestState loses metrics fields
@dataclass(slots=True)
class RequestState:
req_context: ReqContext
pending_primary_stores: int = 0
is_finished: bool = False
request_level_tiers: set[SecondaryTierManager] | None = NoneJobMetadata (renamed from _JobMetadata) stays in manager.py
class JobMetadata(NamedTuple):
transfer_job: TransferJob
tier_idx: intManager call sites
# on_new_request
self._metrics.on_new_request(req_context)
# on_schedule_end (for req_id in new_req_ids)
self._metrics.on_request_allocated(state.req_context)
# on_request_finished
self._metrics.on_request_finished(req_context)
# _register_job
self._jobs[transfer_job.job_id] = jm
self._metrics.on_job_registered(jm)
# _process_finished_jobs
jm = self._jobs.pop(job_id, None)
assert jm is not None
self._metrics.on_job_finished(jm, completed_job)
# _initiate_promotion (failure path)
self._metrics.on_promotion_allocation_failure()
# lookup() — one on_lookup call per tier visited
t0 = time.monotonic()
primary_hit = self.primary_tier.lookup(key, req_context)
elapsed = time.monotonic() - t0
self._metrics.on_lookup(req_context, key, self._metrics.primary_tier_label, primary_hit, elapsed)
if primary_hit is LookupResult.HIT:
return LookupResult.HIT
if primary_hit is LookupResult.HIT_PENDING:
return LookupResult.HIT_PENDING
any_retry = False
for i, tier in enumerate(self.secondary_tiers):
t0 = time.monotonic()
result = tier.lookup(key, req_context)
elapsed = time.monotonic() - t0
self._metrics.on_lookup(req_context, key, self._metrics.tier_label(i), result, elapsed)
if result is LookupResult.HIT:
self._initiate_promotion(...)
return LookupResult.RETRY
elif result is LookupResult.RETRY:
any_retry = True
return LookupResult.RETRY if any_retry else LookupResult.MISS
# get_stats()
metrics_stats = self._metrics.take_stats()
# ...aggregate with secondary tier stats...
# reset_cache()
assert not self._jobs
self._metrics.assert_idle()Tests
All metrics-related tests should move to a new
tests/v1/kv_offload/tiering/test_metrics.py that tests the
TieringMetricsTracker directly (construct with tier_types list, call
on_* methods, assert take_stats() output). The existing tests in
test_tiering_offloading.py that exercise metrics through the full manager
(test_tiering_manager_records_secondary_lookup_metrics,
test_tiering_manager_stops_lookup_metrics_after_allocation,
test_tiering_manager_records_finished_job_metrics,
test_tiering_manager_reports_active_job_and_primary_usage_gauges,
test_tiering_manager_records_promotion_allocation_failures) should move
there and be rewritten against the tracker's interface.
| assert state.primary_read_block_count >= 0 | ||
| return job_metadata | ||
|
|
||
| def _record_finished_job_stats( |
There was a problem hiding this comment.
| def _record_finished_job_stats( | |
| def _observe_finished_job_stats( |
| time_metric, completed_job.transfer_time, labelvalues | ||
| ) | ||
|
|
||
| def _record_active_transfer_stats(self, stats: OffloadingConnectorStats) -> None: |
There was a problem hiding this comment.
| def _record_active_transfer_stats(self, stats: OffloadingConnectorStats) -> None: | |
| def _observe_active_transfer_stats(self, stats: OffloadingConnectorStats) -> None: |
| tier: SecondaryTierManager, | ||
| tier_idx: int, |
There was a problem hiding this comment.
With the added tier_idx, we can now eliminate the tier: SecondaryTierManager parameter (and self._origin.
|
|
||
| def on_new_request(self, req_context: ReqContext) -> RequestOffloadingContext: | ||
| return self._m.on_new_request(req_context, exclude_tier=self._origin) | ||
| return self._m.on_new_request( | ||
| req_context, exclude_tier=self._m.secondary_tiers[self._origin_idx] |
There was a problem hiding this comment.
Let's switch to exclude_tier_idx=self._origin_idx.
|
|
||
| def lookup(self, key: OffloadKey, req_context: ReqContext) -> LookupResult: | ||
| return self._m.lookup(key, req_context, exclude_tier=self._origin) | ||
| return self._m.lookup( | ||
| key, req_context, exclude_tier=self._m.secondary_tiers[self._origin_idx] |
|
|
||
| def on_request_finished(self, req_context: ReqContext) -> None: | ||
| return self._m.on_request_finished(req_context, exclude_tier=self._origin) | ||
| return self._m.on_request_finished( | ||
| req_context, exclude_tier=self._m.secondary_tiers[self._origin_idx] |
| result: LookupResult, | ||
| elapsed: float, | ||
| *, | ||
| starts_async_delay: bool = False, |
There was a problem hiding this comment.
Right now it seems we're using this to measure time which includes promotion time.
I think it's not clear how to measure it correctly per-request (we want to measure just the delay caused by lookup, not including promotion).
On the other hand, measuring it per-block seems straight-forward.
So this is my suggestion:
I think we should measure like we do in the offloading connector _maybe_observe_lookup_async_delay:
Per request, Per-tier, per-block:
- Take time before the first lookup which returned unresolved (not hit/miss)
- Observe when lookup is resolved. Don't observe in any other case.
So we should keep the start time not in _RequestMetricsState directly, but on _RequestMetricsState.observed_lookups, which will hold the start time of the first deferred lookup:
@dataclass(slots=True)
class _RequestMetricsState:
observed_lookups: dict[TierLabel, dict[OffloadKey, float | None]] | None = field(
default_factory=dict
)
We should also change the sync lookup time to be per-tier. And for conformity we should also change it to be per-block.
The metrics docstrings should also be updated.
@Srinivasoo7 please feel free to suggest a better alternative if you have one.
| self._tier_indices = { | ||
| tier: tier_idx for tier_idx, tier in enumerate(self.secondary_tiers) | ||
| } |
There was a problem hiding this comment.
Let's get rid of this.
Instead, switch all places that need it to use tier indices instead of secondary tier objects:
complete_store— iterates self.secondary_tiers → just use enumerate:_flush_pending_promotions— iterates _pending_load_submissions keyed by tier → change the key to int (tier_idx)_cascade_existing_blocks_to_request_level_tiers— iterates request_level_tiers → change from set[SecondaryTierManager] to set[int]:
| key, | ||
| labelvalues, | ||
| result, | ||
| time.monotonic() - start_time, |
There was a problem hiding this comment.
We should take the second time right after the tier.lookup call (and not add _initiate_promotion to the time).
| result: LookupResult, | ||
| elapsed: float, | ||
| *, | ||
| async_delay_start_time: float | None = None, |
There was a problem hiding this comment.
I think we can drop this param, and instead just take time.monotonic() inside this function.
| key: OffloadKey, | ||
| tier_label: TierLabel, | ||
| result: LookupResult, | ||
| elapsed: float, |
There was a problem hiding this comment.
Can we rename this to lookup_duration?
| ) | ||
| elif result is LookupResult.RETRY and state.observed_lookups is not None: | ||
| observed = state.observed_lookups.setdefault(tier_label, {}) | ||
| observed.setdefault(key, time.monotonic()) |
There was a problem hiding this comment.
| observed.setdefault(key, time.monotonic()) | |
| observed.setdefault(key, time.monotonic() - lookup_duration) |
|
This pull request has merge conflicts that must be resolved before it can be |
83bbf4f to
d728a7c
Compare
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
1e7ef43 to
a6bdf9c
Compare
|
Hi @Srinivasoo7, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com>
|
/ci run |
|
✅ Triggered Buildkite CI #83066 for commit |
orozery
left a comment
There was a problem hiding this comment.
Great work @Srinivasoo7 !
Thanks @orozery, let me know next chapter we can conquer! |
…ng connectors Port connector-side capability from vllm-project#50897 (commits 2b7eaf1, b7af87c): - MooncakeStoreConnector/MooncakeConnector/OffloadingConnector declare supports_eagle_prefix_cache_hashing=True and propagate the toggle to their scheduler/worker. - Store worker skips the legacy EAGLE drop (trailing volatile draft chunk) when successor hashing is active; offloading scheduler stops excluding the trailing chunk and gates offloadable tokens on the publishable-hash boundary. - Store ReqMeta gains max_save_tokens; apply_eagle(a)drop renamed for clarity in coordinator/protocol paths. - Port PR's connector unit tests; drop test_prom_metrics...tiering which depends on vllm-project#48798 (not in this branch). Test: 418 passed / 3 failed (= baseline) across Mooncake store + offloading connector suites; ruff check & format clean.
Signed-off-by: srinivas_oo7 <sklinkedin0120@gmail.com> Co-authored-by: srinivas_oo7 <sklinkedin0120@gmail.com>
Summary