Repository navigation
[skycap] Land the W&B record index and pull on main (#2351, #2436); pull writes a directory per phase - #2441
Conversation
…2351, #2436) #2351 and #2436 were reviewed and merged into their stacked base branches after #2350 had already landed on main, so neither reached main. This brings their merged content over unchanged: - SkycapRecordIndex (record_index.py): one W&B artifact version per step, `skycap-records-<phase>-<run id>` aliased `<phase>-step-N`, holding `step.json` (the run index, run_index.md) and a reference per mirrored record file. Fails open on a background worker. - `record_index pull`: a run's records, read from the mirror, and its index, into a local directory. - trainer.py passes `trajectory_ids` to `on_step_end`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
`<out_dir>/train/` and `<out_dir>/eval/` each hold that phase's records, with its step indexes beside them in `index/step-<N>.json`. Each is a plain skycap record directory, so a reader of records needs nothing W&B- or index-specific to show a phase. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a per-step Weights & Biases (W&B) index for skycap records in the Harbor-Skycap integration, allowing trajectory records to be indexed in W&B artifacts and pulled back locally. It adds the SkycapRecordIndex trainer callback, a CLI tool for pulling runs, comprehensive documentation, and updates the RL trainer to pass trajectory IDs to step-end callbacks. Feedback on the changes suggests adding defensive checks to prevent potential TypeError exceptions: specifically, when converting result.record.files to a list in record_index.py and when casting cfg.skycap.record_mirror_config to a dictionary in main_harbor_skycap.py.
|
… config docstrings pull: - A mirror on this machine's own filesystem (file://, a bare path) is refused unless --allow-local-mirror / allow_local: its location comes from the artifact, which could otherwise copy a private local file out. - A download that fails partway is deleted, so a truncated file is never moved in as a record. - A record goes in whole or not at all: a file it replaces is set aside until the whole record is in, and put back if it fails. Index: at most MAX_ABANDONED (2) timed-out W&B calls may still be running; past that, new calls are refused (counted as timed out) rather than starting more threads and temp directories. Config: `record_mirror_config: null` no longer crashes start-up, and the `wandb.enabled` and `record_mirror` docstrings open with a complete sentence. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 0f1b861. Configure here.
…ined URLs A mirror location from the artifact like `simplecache::file://...` passed the local-files check, since the outer filesystem is the cache's. pull now refuses any chained fsspec URL, and reads only remote object stores (s3, s3a, gs, gcs, az, abfs, abfss) or fsspec's in-process memory; file:// and bare paths still need --allow-local-mirror, and anything else (http, ...) is refused. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>

What does this PR do?
TLDR: lands #2351 (the per-step W&B record index) and #2436 (
record_index pull) onmain. Both were reviewed and merged, but into their stacked base branches after #2350 had already landed, so neither reachedmain. On top of that,pullwrites each phase as a record directory of its own.Commits
The merged content of [skycap] Index each step's skycap records in W&B #2351 and [skycap] Pull a run's records and step index from W&B #2436, unchanged (byte-identical to
skycap/wandb-indexat its [skycap] Pull a run's records and step index from W&B #2436 merge):SkycapRecordIndexlogs one W&B artifact version per step,skycap-records-<phase>-<run id>, aliased<phase>-step-N, holdingstep.json(the run index,run_index.md) and a reference per mirrored record file. It fails open on a background worker.record_index pullbrings a run's records, read from the mirror, and its index into a local directory.trainer.pypassestrajectory_idstoon_step_end.New:
pullwrites<out_dir>/train/and<out_dir>/eval/. Each holds that phase's records, plus its step indexes asindex/step-<N>.json. Each is a plain skycap record directory, so a reader of records (e.g. skycap-viewer) shows a phase with nothing W&B- or index-specific.Review fixes:
pullrefuses a mirror on this machine's own filesystem unless--allow-local-mirroris passed, because the mirror's location comes from the artifact.Test plan
pytest tests/integrations/harbor_skycap tests/train/test_rl_callbacks.py: 72 passed. The pull tests check the per-phase layout, train and eval included.skycap-records-train-4gyajb5w):pullwrote 8 records (16 files) andtrain/index/step-1.jsonto<out>/train/. skycap-viewer frommain, with no index support, reads<out>/trainas a normal run: 8 trajectories, all calls bridged.🤖 Generated with Claude Code
Note
Medium Risk
Touches the RL training loop (
on_step_end+ new callback) and pull reads mirror URIs from artifacts (mitigated by protocol allowlist and local-mirror opt-in). W&B indexing is isolated on a fail-open background thread so training steps should not block on logging failures.Overview
Adds per-step W&B indexing of skycap trajectory records for Harbor training: a
RecordLogcollects every attempt (including retries) per phase;SkycapRecordIndexpublishes one artifact version per step (skycap-records-<phase>-<run id>) withstep.json(run index perrun_index.md) and URI references to mirrored record files—no record bytes uploaded. Logging runs on a background worker that fails open (timeouts, retries, queue drops, shutdown wait).Wires this through
main_harbor_skycap: newskycap.record_mirror/record_mirror_config/skycap.wandbconfig, generator hooks inharbor_generator, and trainer callback registration.RayPPOTrainernow passestrajectory_idsonon_step_endso the index can mark which trajectories actually trained (vialoss_mask).Adds
record_index pullto reconstruct local record trees from W&B:out_dir/train/andout_dir/eval/each hold that phase’s*.zstfiles (fetched from the mirror via fsspec, not W&B blob download) plusindex/step-<N>.json. Pull is defensive: mirror protocol allowlist, optional--allow-local-mirror, skip partial records, atomic moves with rollback, plain filename checks.Docs/README updated; integration tests cover index logging and pull behavior; notes W&B index is not available under the fully-async trainer.
Reviewed by Cursor Bugbot for commit cf86829. Bugbot is set up for automated code reviews on this repo. Configure here.