Repository navigation
[skycap] Pull a run's records and step index from W&B - #2436
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a pull command-line utility to the record_index module, allowing users to retrieve a run's records and step index from Weights & Biases (W&B) into a local directory. It includes comprehensive unit tests mocking the W&B API and updates the documentation accordingly. The feedback suggests optimizing the file comparison in _move_in by checking file sizes before computing SHA-256 digests to avoid unnecessary hashing.
|
86b833e to
216b85e
Compare
9ac84f5 to
5377d80
Compare
216b85e to
51c5a34
Compare
5377d80 to
668b6fa
Compare
kouroshHakha
left a comment
There was a problem hiding this comment.
leaving reviews.
`python -m examples.train_integrations.harbor_skycap.record_index pull <entity>/<project>/<artifact>[:<alias>] <out_dir>` (and `pull()`) turns the record index's artifact back into a record directory: per version, W&B downloads the record files from the mirror with the caller's credentials, they move into out_dir by atomic rename (an identical file already there is left alone), and step.json goes to index/<phase>/step-<N>.json. With an alias it pulls that version, without one every version. It fails open per record: a record whose files can't all be fetched is reported and left out whole, the rest go on; a version that can't be read is reported and the others go on. Local-only records are indexed with nothing to pull, and reported. It prints a summary and exits non-zero only when nothing was pulled. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
The summary names where a record that isn't in the mirror is, from the index row's `record.host` and `path` (`10.0.0.5:/data/record/tr_c.json.zst`), so it can be fetched from the node that wrote it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
…at can't move in; no viewer mentions - A record file name from the downloaded step.json must be a plain file name (no `/`, `\`, `.`, `..`), or the record is reported and not pulled: a crafted `../x` would otherwise move a file out of out_dir. - A record that can't move in (a directory where a file goes) is reported, the files it newly placed are taken back out, and the other records and the step index still go in. - Comparing an existing file checks its size before hashing. - The docs no longer mention the viewer: this pulls a run's records out of W&B, and readers of a record directory are their own business. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
668b6fa to
4a7c455
Compare
…mes from W&B W&B's download of an s3:// reference added with checksum=False first calls ListObjectVersions, which a role that can read the bucket may not be allowed (s3:ListBucketVersions): on a real run every record failed to fetch that way. pull now takes only step.json from W&B and reads each record file from the mirror through fsspec with the caller's credentials, so read access to the mirror is enough. A file it can't read is left out, and the record is reported missing, naming exactly the files that failed. The whole-artifact download and its entry-by-entry fallback are gone. 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 d2f7dca. Configure here.
| fs, path = fsspec.core.url_to_fs(uri) | ||
| fs.get_file(path, str(dest / name)) | ||
| except Exception as error: # noqa: BLE001 - this record is reported missing | ||
| logger.warning(f"skycap record pull: fetching {uri} failed: {type(error).__name__}: {error}") |
There was a problem hiding this comment.
Partial fetches treated as complete
Medium Severity
A failed fs.get_file can leave an empty or partial file in the scratch directory. _move_record only checks that each name is_file(), so that leftover is treated as a complete fetch and the record is moved in and counted as pulled.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d2f7dca. Configure here.
…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>


What does this PR do?
TLDR:
record_index pullturns 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.How it works
train-step-3,v7) pulls that version; with no alias, every version is pulled.step.jsoncomes from W&B, written asout_dir/index/<phase>/step-<N>.json.s3fsfors3://,gcsfsforgs://), so read access to the mirror is enough. W&B's own reference download isn't used: fors3://it needss3:ListBucketVersions, which a reader may not have.out_dirwith an atomic rename from a scratch directory inside it. A file already there and identical is left alone, so pulling again only adds what's new.step.jsonis reported, and the other versions still pull.host:path, so they can be fetched from the node that wrote them.Test plan
tests/integrations/harbor_skycap/test_record_pull.py, with a fake W&B API and an in-memory mirror: one version and all versions; identical files left alone; an unreadable file; a record that can't move in; an unsafe name; a bad version; local-only rows; exit codes.pytest tests/integrations/harbor_skycap: 66 passed.record_dir, plusindex/train/step-1.json.🤖 Generated with Claude Code
Note
Low Risk
Offline tooling with defensive path validation and isolated test coverage; no changes to training or W&B indexing callbacks.
Overview
Adds
record_index pull, a CLI (andpull()API) that reconstructs a local skycap record directory from W&B training artifacts so runs can be opened inskycap-viewer.For each artifact version it downloads
step.jsonfrom W&B, writes it underindex/<phase>/step-<N>.json, and fetches mirrored record bytes directly from the object-store mirror via fsspec (avoiding W&B reference downloads that need extra S3 permissions). Optional alias pulls one version; omitting it pulls all versions. Fails open per version/record (missing files skip whole records; local-only rows are reported withhost:path). Idempotent moves use SHA256 checks so re-pulls only add new content.PullSummaryand exit code 1 only when nothing was pulled.README documents usage;
tests/integrations/harbor_skycap/test_record_pull.pycovers the flow with a fake W&B API and memory mirror.Reviewed by Cursor Bugbot for commit d2f7dca. Bugbot is set up for automated code reviews on this repo. Configure here.