Skip to content

[skycap] Upload each step's skycap documents to W&B - #2333

Closed
jlee-lila wants to merge 8 commits into
NovaSky-AI:mainfrom
jlee-lila:skycap/wandb-artifact
Closed

jlee-lila wants to merge 8 commits into
NovaSky-AI:mainfrom
jlee-lila:skycap/wandb-artifact

Conversation

@jlee-lila

@jlee-lila jlee-lila commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Adds SkycapUploads, a trainer callback that uploads skycap documents to W&B when trainer.logger=wandb. It fetches each document from its skycap server and uploads on a background thread; on_train_end waits for it.

Train records upload once per step to skycap-records-train-<run id>, aliased step-N and latest. skycap.wandb.phases adds eval, which goes to its own skycap-records-eval-<run id> artifact. skycap.wandb.enabled=false turns uploads off.

Each version holds a step.json that marks every attempt trained or dropped (retries, dynamic sampling). The metadata counts uploaded and missing documents. For this, CallbackInput gains trajectory_ids at on_step_end.

The fully-async trainer fires no callbacks, so it uploads nothing. run_codecontests_modal.sh also takes the Modal credentials from the environment, and its default run names carry a UTC timestamp.

Tested: tests/integrations/harbor_skycap and tests/train/test_rl_callbacks.py pass on 8xB200 (21 passed).

🤖 Generated with Claude Code

jlee-lila and others added 3 commits September 29, 2026 15:21
skycap.wandb_artifact (default false) uploads each step's {id}.json.zst
documents, without sidecars, as a version of the skycap-records-<run id>
artifact, aliased step-N and latest. Retried attempts are included. An
upload failure is logged and does not fail the step.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nment, timestamped run names

MODAL_TOKEN_ID and MODAL_TOKEN_SECRET in the environment skip MODAL_KEY_FILE.
EXPERIMENT can be set, and the default name carries a UTC timestamp.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… arguments

The servers always write records to record_dir; wandb_artifact alone
turns the upload on.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kouroshHakha kouroshHakha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Using a W&B artifact for this is the right tool, and it isn't slow: the heavy upload happens in wandb's background service, the call runs off the event loop, and errors are caught, so a W&B outage can't fail rollouts. The main issue is that it uploads once per generate() call, and that isn't once per training step. Three things to fix before merging:

  1. Eval takes the step-N and latest aliases. Eval calls the same generate() with the same global_step, once per eval batch, right after training step N and at step 0. An alias belongs to one version at a time, so the eval upload moves step-N off the training records. training_phase is in batch_metadata and isn't checked.
  2. Several versions per step. Dynamic sampling calls generate() repeatedly within one step, and each call moves step-N to itself, so the earlier versions are reachable only as vK.
  3. Silent partial uploads across nodes. Documents that aren't on the trainer's disk are skipped quietly. With placement_strategy="SPREAD" and the node-local default record_dir, that's the likely case on multi-node, and num_trajectories still counts every id.

What I'd do:

  • Upload once per training step, train phase only. Accumulate ids per global_step, upload when the step changes and at shutdown, and send eval to its own skycap-records-eval-<run> artifact or leave it off by default.
  • Fetch documents from the server that owns them (GET /trajectories/{id} already reads from disk) rather than from the trainer's filesystem. The pool knows which server holds each id, so the "one node or a shared record_dir" constraint goes away.
  • Don't await it on the step. Run it as a background task, optionally with sampling. Hashing 256 long documents costs roughly 10-100 ms per step at 32x8, which isn't big, but the step doesn't need to wait for it.

Smaller notes are inline. Size looks fine: roughly 10-80 MB per step at 32x8 with 32k-token trajectories, and memory stays flat since it streams files. A wandb.Table would have been much worse.

Comment thread examples/train_integrations/harbor_skycap/harbor_generator.py Outdated
Comment thread examples/train_integrations/harbor_skycap/harbor_generator.py Outdated
Comment thread examples/train_integrations/harbor_skycap/artifacts.py Outdated
Comment thread examples/train_integrations/harbor_skycap/artifacts.py Outdated
Comment thread examples/train_integrations/harbor_skycap/artifacts.py Outdated
Comment thread examples/train_integrations/harbor_skycap/artifacts.py Outdated
Eval batches call generate with the training step's global_step, so a
step-N alias moved to the eval records. Uploads are now aliased
train-step-N or eval-step-N, and the metadata records training_phase.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread examples/train_integrations/harbor_skycap/entrypoints/main_harbor_skycap.py Outdated
jlee-lila and others added 4 commits September 30, 2026 10:20
…ir servers

SkycapUploads uploads the train records once per step and the eval records
once per eval pass, on a background thread. Each document comes from the
server that wrote it. A step.json marks each attempt trained or dropped.
Each phase gets its own artifact, aliased step-N and latest. The
skycap.wandb group replaces skycap.wandb_artifact. CallbackInput carries
the step's trajectory_ids at on_step_end.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without MODAL_TOKEN_ID or MODAL_KEY_FILE, the script runs when
~/.modal.toml exists, and the Modal SDK reads it. That covers one node
only, because SkyRL forwards Modal credentials to Ray workers from the
environment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kouroshHakha kouroshHakha left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

leaving some comments.

record_dir: Optional[str] = None
"""Where ended trajectories are written. Defaults to ``{trainer.export_path}/skycap``; each server writes
on its own node, so point it at a shared filesystem to have one directory for the run."""
wandb: SkycapWandbConfig = field(default_factory=SkycapWandbConfig)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit style: comments are not added

Comment thread examples/train_integrations/harbor_skycap/harbor_generator.py Outdated
Comment thread examples/train_integrations/harbor_skycap/harbor_generator.py Outdated
Comment thread examples/train_integrations/harbor_skycap/artifacts.py Outdated
Comment thread examples/train_integrations/harbor_skycap/artifacts.py Outdated
Comment thread examples/train_integrations/harbor_skycap/artifacts.py Outdated
Comment thread examples/train_integrations/harbor_skycap/artifacts.py Outdated
kouroshHakha

This comment was marked as resolved.

@kouroshHakha

Copy link
Copy Markdown
Collaborator

A thought on where this should live, before more iteration on the upload path. I think it splits cleanly in two, and neither half belongs in the Harbor example.

1. Getting records to durable storage is skycap's job. skycap already writes each trajectory once, when it ends, off the event loop. The semantics we want for the upload (async, fail open, bounded queue, timeouts, a deadline at shutdown) are exactly a record writer's. A remote store is just another destination for the same records. It also removes the multi-node problem at the source: today records land on each server's node-local disk, so this PR has to fetch them back over HTTP through the trainer and re-upload the bytes. If skycap writes to shared or remote storage (e.g. record_dir accepting a URL like s3://… or gs://…), there's nothing to fetch. Every skycap user gets it, and the viewer can read the same place. skycap stays free of SkyRL and W&B dependencies.

2. What goes to W&B is the trainer's job. "The trajectories of training step N, their phase, which were trained and which superseded" is SkyRL knowledge. A callback can log that per step through Tracking as a small index artifact: ids, phase, trained/superseded, and where each record lives. With remote storage those can be W&B reference entries, so no bytes are copied, and failing open is trivial. Any skycap-based generator wants this, so it belongs next to the skycap generator code rather than inside Harbor specifics.

With that split, most of this PR's fetch/re-upload/retry/timeout machinery goes away. I'm opening the two pieces as separate PRs and will link them here. Happy to fold anything from this PR's design into them (the SkycapWandbConfig shape, the phase handling, the eval opt-in).

@kouroshHakha

Copy link
Copy Markdown
Collaborator

Following up on the comment above, the two pieces are up:

Together they replace this PR's upload path. Reviews and pushback welcome there.

kouroshHakha added a commit that referenced this pull request Oct 7, 2026
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>
kouroshHakha added a commit that referenced this pull request Oct 7, 2026
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>
kouroshHakha added a commit that referenced this pull request Oct 7, 2026
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>
kouroshHakha added a commit that referenced this pull request Oct 7, 2026
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>
kouroshHakha added a commit that referenced this pull request Oct 8, 2026
**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.

---------

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

2 participants