Skip to content

[metrics 3/5] Publish weighted fresh-run vLLM summaries - #2367

Merged
SumanthRH merged 3 commits into
mainfrom
metrics/run-summaries
Oct 4, 2026
Merged

SumanthRH merged 3 commits into
mainfrom
metrics/run-summaries

Conversation

@SumanthRH

@SumanthRH SumanthRH commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

What does this PR do?

Publish weighted vLLM run aggregates to W&B Summary before tracking closes.

TLDR: Reduce raw observed counters, histogram statistics and measured durations once at the end of a fresh run.

Why this is needed

Last-step values and averages of step rates do not describe a whole run. For example, 1,000 tokens in 10 seconds followed by 9,000 tokens in 30 seconds gives 250 tokens/s overall, while averaging the two rates gives 200 tokens/s.

How it works

Accumulate counter and histogram deltas over non-overlapping windows. Compute throughput from total tokens or bytes divided by total measured seconds, cache hit rates from total hits/queries, and latency means/P90 from merged histogram statistics. Include token, preemption and offload-byte totals plus measurement_seconds.

Ray omits counters until their first nonzero increment. After validating a complete scrape and the expected worker set, a live engine gauge establishes zero baselines for prompt and generated token totals. This includes the first generation in step throughput and run aggregates; missing scrapes, reset handling and the aggregate intersection rule retain their existing behavior.

Sync train/eval windows remain separate. Fully async collection takes a baseline before generation and uses elapsed time between samples, including idle and weight-sync time. Publish summaries under vllm_correct_aggregate/{train,eval,combined}/*; keep vllm/* in step history. Gauges remain step snapshots.

finalize_metrics() makes one final collection attempt with a 10-second outer bound and closes HTTP clients. Finalization runs on success and handled failure. Tracking publishes run_status and uses W&B exit code 1 for failures. Loading a checkpoint suppresses aggregates; LATEST without a checkpoint remains fresh. Generic PD summaries stay disabled in this layer; #2401 adds separate role scopes.

Collection limits

Missing/reset windows omit the affected scope; counters missing from part of a scope are omitted. Reused external servers require an observed baseline. Collection does not wait for another Ray export, so short runs or delayed terminal exports can leave aggregates unavailable or incomplete. No metric checkpoint history or local summary files are added.

SkyRL requests summary="none" and removes SDK summary entries for step keys, but W&B can still infer last values. Use vllm_correct_aggregate/* for comparisons. A hard kill cannot publish final status or summaries.

Validation

Tests cover weighted rates/ratios, request-weighted latency, merged histograms, missing/reset windows, reused baselines, checkpoint detection and finalization on success/failure/cancellation.

The cold-start regression covers sync and async collection through the HTTP parser: the baseline contains only a live engine gauge, token counters appear after generation, and both first-window rates and weighted run totals include those tokens. 2 cases failed before the fix with missing first-window throughput keys. After the fix, 54 focused tests passed on this PR at f9ca0a21, and 72 focused tests passed on the complete stack at 481238da (#2401), including both regression cases. Ruff, Black and secret checks passed.

These CPU tests use mocked HTTP responses and do not attach to the live Ray cluster. --noconftest skips the repository's automatic Ray lifecycle fixture; Ray 2.58.0 matches the validation cluster. --offline uses cached uv dependencies.

uv run --isolated --offline --frozen --extra skyrl-train --extra dev --with ray==2.58.0 pytest --noconftest \
  tests/train/test_grafana_annotations.py tests/train/test_metrics_lifecycle.py \
  tests/train/test_vllm_run_statistics.py tests/train/test_vllm_metrics_scraper.py \
  tests/train/test_vllm_window_statistics.py tests/train/test_vllm_engine_imbalance.py \
  tests/train/test_vllm_pd_metrics.py tests/train/test_tracking.py -q

A read-only W&B/Prometheus audit of metrics-offload-0930-freshdash-191711 matched 15 aggregates within floating-point rounding: 24,576 output tokens, 73 preemptions and 259.904735 output tok/s.

The audit used workload bounds 2026-09-30 19:21:51.471927–19:23:26.153083 UTC, the two owned WorkerIds and ClusterId, and terminal visibility through end +35 seconds. It checked counter resets, reduced raw totals/histograms, and used the recorded 94.557723 seconds of generation for throughput. Workload-wall throughput was 259.565906 tok/s, a 0.1304% difference.

That historical run used the older namespace and an export wait. It verifies its bookkeeping, not completeness of this PR's one-attempt terminal collection. No new GPU benchmark was run for this layer.


Note

Medium Risk
Touches training teardown, W&B lifecycle, and metrics on failure paths; incorrect finalization could drop summaries or double-finish runs, but behavior is guarded and heavily tested.

Overview
Adds end-of-run vLLM aggregates under vllm_correct_aggregate/{train,eval,combined}/* by accumulating counter/histogram deltas across non-overlapping windows (RunStatistics), then publishing weighted throughput, cache rates, latency, and totals via W&B summary (not step history). Step metrics stay on vllm/*; W&B auto-summaries for those keys are disabled and stripped on finish so dashboards should use the aggregate namespace.

finalize_metrics() runs once on success or handled failure (10s-bound final scrape, HTTP client close). Fresh runs publish scraper summaries; checkpoint resume and PD skip aggregate summary upload. The PPO entrypoint keeps the tracker on the experiment, finalizes trainer metrics before exception logging, and always **finish()**es tracking with run_status and a non-zero exit code on failure.

The scraper now records run windows on sample()/stop(), adds KV offload store bytes, requires a full worker set when filtered, and treats incomplete scrapes as omitting scope totals. Fully-async training samples a baseline before the loop and uses the shared finalization path.

Reviewed by Cursor Bugbot for commit f9ca0a2. Bugbot is set up for automated code reviews on this repo. Configure here.

@SumanthRH
SumanthRH added this pull request to stack #2378 October 1, 2026 14:18
@SumanthRH
SumanthRH force-pushed the metrics/run-summaries branch from bd657ee to 78144a5 Compare October 1, 2026 14:36
@SumanthRH
SumanthRH force-pushed the metrics/run-summaries branch from 78144a5 to eff942a Compare October 1, 2026 18:47
@SumanthRH
SumanthRH force-pushed the metrics/run-summaries branch from eff942a to 1933383 Compare October 1, 2026 19:54
@SumanthRH SumanthRH changed the title [metrics 4/5] Finalize weighted vLLM run summaries [metrics 4/5] Finalize weighted vLLM summaries for fresh runs Oct 1, 2026
@SumanthRH
SumanthRH removed this pull request from stack #2378 October 4, 2026 01:08
@SumanthRH
SumanthRH force-pushed the metrics/run-summaries branch from 1933383 to a8545eb Compare October 4, 2026 01:08
@SumanthRH
SumanthRH added this pull request to stack #2400 October 4, 2026 01:08
@SumanthRH SumanthRH changed the title [metrics 4/5] Finalize weighted vLLM summaries for fresh runs [metrics 3/4] Publish weighted fresh-run vLLM summaries Oct 4, 2026
@SumanthRH SumanthRH changed the title [metrics 3/4] Publish weighted fresh-run vLLM summaries [metrics 3/5] Publish weighted fresh-run vLLM summaries Oct 4, 2026
@SumanthRH
SumanthRH force-pushed the metrics/run-summaries branch from a8545eb to 56f4819 Compare October 4, 2026 05:03
@SumanthRH
SumanthRH marked this pull request as ready for review October 4, 2026 05:15

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a robust metrics finalization lifecycle for vLLM metrics, ensuring that run statistics and final statuses are correctly aggregated and reported to the tracker (such as WandB) even in the event of training failures. It adds a new RunStatistics utility to aggregate non-overlapping metric windows, prevents automatic WandB summaries for step-level vLLM metrics, and implements comprehensive unit tests to verify the lifecycle and tracking behavior. There are no review comments provided, so I have no feedback to address.

@greptile-apps

greptile-apps Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Medium risk] Adds metrics finalization and summary publishing to training lifecycle.

The PR should not merge until the synchronous run-level eval summary accounts for baseline evaluation.

Findings

  1. P1 Baseline evaluation missing from totals ▶
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Sync train/eval windows] --> C[RunStatistics]
  B[Async samples] --> C
  C --> D[Final collection]
  D --> E[Weighted W&B summaries]
  E --> F[Tracking finish and run status]
Loading

Reviews (1) · Last reviewed commit: "Finalize metrics with elapsed async obse..."

self._window_prev = None
self._active_since = None
self._paused = False
self.run_statistics.add(label.removeprefix("vllm/"), prev, new_snapshot, window)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Baseline evaluation missing from totals When eval_before_train is enabled, the synchronous trainer runs its initial evaluation without opening a vllm/eval window. This new aggregate records only closed windows, so vllm_correct_aggregate/eval/* excludes that evaluation's requests and tokens even though it is presented as a run-level eval total. Include the initial evaluation in the eval window accounting.

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread skyrl/train/utils/vllm_metrics_scraper.py
Comment thread skyrl/train/utils/vllm_run_statistics.py
@SumanthRH
SumanthRH force-pushed the metrics/run-summaries branch from 56f4819 to 62aa920 Compare October 4, 2026 05:31
@SumanthRH
SumanthRH force-pushed the metrics/run-summaries branch from 62aa920 to a051625 Compare October 4, 2026 05:35
Base automatically changed from metrics/engine-imbalance to main October 4, 2026 05:41
@SumanthRH
SumanthRH force-pushed the metrics/run-summaries branch from a051625 to ef23c55 Compare October 4, 2026 05:41
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
@SumanthRH
SumanthRH force-pushed the metrics/run-summaries branch from ef23c55 to 57f20fb Compare October 4, 2026 05:52

@cursor cursor 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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 57f20fb. Configure here.

Comment thread skyrl/train/utils/vllm_run_statistics.py
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
@SumanthRH
SumanthRH merged commit 66b68d0 into main Oct 4, 2026
8 checks passed

This branch was successfully deployed

1 active deployment
Preview — f9ca0a21 Deployed Oct 4, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant