Skip to content

[skycap] Index each step's skycap records in W&B - #2351

Merged
kouroshHakha merged 6 commits into
skycap/record-mirrorfrom
skycap/wandb-index
Oct 8, 2026
Merged

kouroshHakha merged 6 commits into
skycap/record-mirrorfrom
skycap/wandb-index

Conversation

@kouroshHakha

@kouroshHakha kouroshHakha commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

TLDR: each training step's skycap records are indexed in W&B: one artifact version per step listing which trajectories made up the step, and how each one ended up. Mirrored records (#2350) appear as W&B references, so no record bytes are uploaded. It replaces #2333's upload path, and its design (config shape, per-phase log, eval opt-in) comes from that PR. Stacked on #2350.

How it works

  • SkycapRecordIndex (examples/train_integrations/harbor_skycap/record_index.py), a TrainingCallback.
    • At on_step_end (and on_eval_end when enabled) it logs one version of skycap-records-<phase>-<run id>, aliased <phase>-step-N and latest, through the trainer's Tracking.
    • The version holds:
      • step.json: the step's run index. One row per trajectory attempt, with instance_id, repetition_id, attempt, status, annotations, superseded, trained, and finish's record {host, path, mirror, files}. The format is in run_index.md.
      • records/<file>: one reference per file of each mirrored record. A local-only record is listed in the index only.
  • RecordLog: the generator logs every attempt it opened, per phase, retries included.
    • trained comes from the step's batch: whether the final attempt had trainable tokens.
    • Train entries still in the log at on_step_start came from a batch the trainer dropped unfinished. They're discarded, not indexed with the next step.
  • Failure behaviour. Building the index runs on the training thread, so a bug in that code fails the step. Everything that talks to W&B runs on one background worker and fails open:
    • a timeout per version, after which the version is abandoned, not retried;
    • bounded retries for transient errors, only before W&B accepts the version;
    • a bounded queue that drops when full;
    • a deadline at on_train_end;
    • counters in stats().
  • Config (skycap.* in the entrypoint):
    • wandb.enabled is on by default when trainer.logger=wandb; wandb.phases is [train] by default;
    • record_mirror and record_mirror_config are passed to every server.
  • Core change: trainer.py passes trajectory_ids in CallbackInput at on_step_end (the batch's rows carry their loss masks), so the index knows which trajectories trained.

Test plan

  • tests/integrations/harbor_skycap/test_record_index.py (fake W&B, no network):
    • one version per step however many generate() calls the step took;
    • superseded and untrained attempts are marked;
    • mirrored records become references, one per file;
    • eval goes to its own artifact only when enabled;
    • a dropped partial batch isn't indexed with the next step;
    • W&B that fails, is slow, or already accepted a version fails open without duplicates;
    • a full queue drops, and shutdown respects its deadline;
    • a bug in the callback raises into the step.
  • pytest tests/integrations/harbor_skycap: 56 passed. pre-commit is clean.

🤖 Generated with Claude Code

@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 new SkycapRecordIndex callback to track and index training trajectories in Weights & Biases. It adds support for mirroring records to remote storage, updates the HarborSkycapGenerator to log trajectory metadata, and modifies the RayPPOTrainer to pass trajectory IDs to callbacks. The review feedback identified a potential thread leak in the worker loop and suggested safer dictionary access for record metadata, both of which are actionable improvements.

Comment thread examples/train_integrations/harbor_skycap/record_index.py
Comment thread examples/train_integrations/harbor_skycap/record_index.py Outdated
@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Medium risk] Adds W&B logging for skycap training records.

The PR should not merge until discarded dynamic-sampling batches are excluded from later step indexes.

Findings

  1. P1 Discarded trajectories enter later indexes ▶
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  G[Skycap generator] -->|attempts and record locations| L[RecordLog by phase]
  T[Trainer step or eval callback] -->|take phase entries| L
  T -->|batch trajectory IDs and loss mask| I[Build step index]
  L --> I
  I --> Q[Background logging queue]
  Q --> W[W&B step.json and mirror references]
Loading

Reviews (1) · Last reviewed commit: "[skycap] Index each step's skycap record..."

Comment thread examples/train_integrations/harbor_skycap/record_index.py

@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 using default effort 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 6e73183. Configure here.

Comment thread examples/train_integrations/harbor_skycap/record_index.py
kouroshHakha added a commit that referenced this pull request Oct 7, 2026
… Harbor integration

skycap writes records; which trajectories made up a training step is the
trainer integration's knowledge, so its index file is specified there (#2351),
not in skycap's format.md or README.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
kouroshHakha and others added 5 commits October 7, 2026 23:54
SkycapRecordIndex, a trainer callback, logs one version per step of
`skycap-records-<phase>-<run id>`, aliased `<phase>-step-N` and `latest`:
a step.json with every attempt the generator opened (trained or
superseded, and its record location from FinishResult.record), plus a
checksum-free W&B reference per mirrored document. No record bytes are
uploaded. The index is built on the trainer's thread; the W&B calls run on
a background thread and fail open (bounded queue, per-call timeout without
retry, bounded retries before W&B accepts a version, shutdown deadline).

The generator logs its trajectories into a RecordLog per phase. The
trainer passes the batch's trajectory_ids to on_step_end, so the index can
tell which trajectories trained. skycap.record_mirror is passed through to
the servers, and skycap.wandb.{enabled,phases} configures the index.

Builds on #2333's design (SkycapWandbConfig, per-phase RecordLog, the
trained/superseded marking, eval opt-in).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
…ons to every server

e.g. `+skycap.record_mirror_config={exclude: [experts, sampling_mask]}` keeps
routed experts and sampling masks out of the remote copy.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
… per record file

step.json becomes the run index skycap's docs/format.md specifies: a header
(format_version, run, phase, step) and the rows, each carrying its record's
`files`. The per-row `phase` moves to the header. A mirrored record gets one
W&B reference per file in `record.files` (records/<name>, beside the mirrored
document, checksum=False) instead of one for its document, so pulling the
artifact gets whole records, sidecars included.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
…hout omitted_sidecars

- Each index row's `record` includes `host`, the machine `path` is on, as
  finish now reports it, so a head node can reach a local-only record.
- The mirror test checks that `exclude` leaves the sidecar out and copies the
  document unchanged (#2350 no longer writes `omitted_sidecars`).
- Tests pass `records=` by keyword: main added `train_paths` before it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
…_index.md

It moved out of skycap's format.md (#2350 review): skycap writes records, and
which trajectories made up a step is this integration's knowledge.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
…step; index worker races

- Dynamic sampling that reaches an epoch's end drops its partial batch, and
  the next epoch retries the same global step. The index now discards train
  trajectories still in the log at `on_step_start` (a finished step takes its
  own at `on_step_end`), counting them as `discarded`, so they never appear in
  the next step's `step.json`.
- The worker waits on the queue with a timeout and exits once the index is
  stopping, so it can't hang when close couldn't queue or drained `_STOP`.
- `_submit` checks `_closed` and queues under the lock `close` sets it with.
- `pending` is a counter, lowered together with the outcome's count, so it
  never reads 0 while a version is between the queue and W&B.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
@kouroshHakha
kouroshHakha merged commit 7c727b3 into skycap/record-mirror Oct 8, 2026
8 checks passed
kouroshHakha added a commit that referenced this pull request Oct 8, 2026
**TLDR:** `record_index pull` turns a run's W&B record index (#2351)
back into a local record directory: the step's records, read from the
mirror (#2350), plus the step index. Stacked on #2351.


---------

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
kouroshHakha added a commit that referenced this pull request Oct 8, 2026
…ull writes a directory per phase (#2441)

**TLDR:** lands #2351 (the per-step W&B record index) and #2436
(`record_index pull`) on `main`. Both were reviewed and merged, but into
their stacked base branches after #2350 had already landed, so neither
reached `main`. On top of that, `pull` writes each phase as a record
directory of its own.

---------

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch was successfully deployed

1 active deployment
Preview — da208009 Deployed Oct 8, 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