Skip to content

feat(client): add Cortex transport + DSSST1 wire codec - #47

Closed
sfc-gh-mwyatt wants to merge 1 commit into
mainfrom
mwyatt/unified-client-3
Closed

sfc-gh-mwyatt wants to merge 1 commit into
mainfrom
mwyatt/unified-client-3

Conversation

@sfc-gh-mwyatt

Copy link
Copy Markdown
Collaborator

Add the Cortex (cortex-training) transport over the SnowAPI GS REST surface: an op->handler table that builds the CreateJob wire schema, submits each op, and polls the async request to completion. Add the DSSST1 safetensors wire codec (wire.py) it depends on -- the single serialization source of truth (codec + op metadata + byte chunking), pickle-free on decode. Wire cortex into the config (cortex backend + cortex_* fields), make_transport, and the example.

Tests: tests/client/test_wire.py (roundtrip, chunking, pickle rejection) and tests/client/test_cortex.py (wire schema, sub-job gating/order, result decoders).

@sfc-gh-mwyatt
sfc-gh-mwyatt changed the base branch from mwyatt/unified-client-2 to main July 27, 2026 17:07
@sfc-gh-mwyatt sfc-gh-mwyatt mentioned this pull request Jul 27, 2026
@sfc-gh-mwyatt
sfc-gh-mwyatt force-pushed the mwyatt/unified-client-3 branch 2 times, most recently from 53c03bb to e80a270 Compare July 27, 2026 17:45
@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator

Reviewed the whole three-PR stack end-to-end and ran the tests on each branch (13/13, 14/14, 44/44 pass). Full write-up here since this PR sits at the top of the stack; specific per-PR notes on #45 and #46.

wire.py is genuinely excellent — pickle-sniff-and-refuse on decode, sha256 frame integrity, per-chunk agreement checks, structure-preserving skeleton, 15 focused tests. Please keep going in this style.

Splitting the remaining feedback into code fixes for this stack vs. plan-level work (UNIFICATION_NOTES.md follow-ups) because most of the friction is server-side and cannot land here.


Code fixes in this stack

F1 — CortexTransport._save_checkpoint silently drops client arguments (this PR)

Reproduced live:

body = {"stage_info": {"step": 42, "epoch": 3}, "path": "/my/ckpt"}
# HTTP request body actually sent: {'checkpoint_type': 'resumable'}
# DROPPED: {'path', 'stage_info'}

The client-facing signature save_checkpoint(stage_info=..., path=...) promises these fields matter; the handler ignores both:

# arctic_platform/client/transports/cortex.py:322-328
def _save_checkpoint(self, body: dict) -> dict:
    request_id = self._send(
        "POST",
        f"{self._prefix}/{self.job_id}/save",
        json={"checkpoint_type": "resumable"},
    ).json()["request_id"]
    return self._poll(request_id)

Any downstream caller that relies on stage_info bookkeeping to survive a resume gets silent behavioral divergence between backends — the worst kind of bug because it looks like it worked. Either forward the fields, or raise NotImplementedError when they're non-None. "Accept and ignore" isn't safe.

F2 — onprem_http._loads enables pickle on decode (#46, one-liner)

_loads uses torch.load(io.BytesIO(data), map_location="cpu", weights_only=False) at arctic_platform/client/transports/onprem_http.py:44. weights_only=False re-enables pickle on the wire, so a compromised, misconfigured, or wire-buggy server can execute arbitrary code in the client. Same repo ships wire.loads() in this PR with an explicit pickle/zip sniff-and-refuse — please either switch to it, or at minimum set weights_only=True. Not gated on the on-prem server change (that's P2 below); this one is a local safety fix.

F3 — Add router_replay_discard to ArcticRLClient + Cortex handler (this PR)

UNIFICATION_NOTES.md enumerates the arctictraining ArcticRLClient router-replay ops (bootstrap/meta/attach) and notes the Neutrino branch's router-replay support, but the unified client exposes no router-replay ops. Any downstream caller running with enable_router_replay=True that does mid-rollout sample discards has no path to migrate. Small addition: one op on ArcticRLClient + one handler in CortexTransport.


Plan-level work (belongs in UNIFICATION_NOTES.md follow-up track)

These can't land in this stack but must be sequenced explicitly — otherwise "one client, two backends" stays a design goal instead of a shipped reality. Suggest turning each into a checklist row with an owner and target PR.

P1 — Converge servers on one fwd_bwd batch contract

The client's frontend signature fwd_bwd(batch, ...) is uniform but batch's content is not. The on-prem server reads a pre-tokenized batch with meta:

# arctic_platform/rl/ray_server.py:606-628
async def fwd_bwd(self, job_id: int, batch: dict) -> dict[str, Any]:
    ...
    shards, _ = ray_split_batch(batch, len(workers))
    ...
    shards[0]["meta"]["worker_return_tensors"] = True

The Cortex path forwards body to a server that tokenizes RPC-style {args, kwargs} on its side. arctic_platform/client/client.py:46-58 documents this in the TODO. Practical consequence: callers still have to know which backend they're targeting when they construct batch, which contradicts "switching deployment == changing config.backend" from the client docstring.

Pick one before the first caller migrates: (a) on-prem server accepts {args, kwargs}, (b) Cortex server accepts pre-tokenized batches, or (c) client-side tokenization lives in arctic_platform.client (dss-client's build_forward_backward_kwargs is roughly the right shape per UNIFICATION_NOTES.md) and both servers accept the same shape.

P2 — Collapse to one wire codec across backends

Reproduced live, byte level:

on-prem HTTP frame:  1641 bytes  first-4-bytes: b'PK\x03\x04'         (torch.save zip)
cortex   wire frame:  264 bytes  first-4-bytes: b'\xd0\x00\x00\x00'    (safetensors header)

[wire  -> HTTP loads] FAILS: UnpicklingError: invalid load key, '\xd0'.
[HTTP -> wire  loads] FAILS: WireError: refusing to deserialize a legacy pickle/torch.save payload

wire.py's docstring advertises "the single source of truth for how dss serializes binary training payloads", but two codecs currently coexist because onprem_http._loads mirrors what arctic_platform/rl/http_server.py emits today. UNIFICATION_NOTES.md is honest about the ordering constraint ("onprem_http mirrors the pickle path to match today's server; it cannot switch to wire until the on-prem server does").

Follow-up sequence: land a PR that flips arctic_platform/rl/http_server.py to DSSST1, then delete the _dumps/_loads pair from onprem_http.py. Until then the on-prem path has an unnecessary attack surface (F2 mitigates it locally) and any future wire change has to land twice.

P3 — Retire the async arctic_platform.rl.*_client.py

Public-method AST scan:

arctic_platform/rl/http_client.py (ArcticRLHTTPClient) — every op is `async def`
arctic_platform/client/client.py  (ArcticRLClient)     — every op is `sync`

Merging this stack adds a second client with a different calling convention; it doesn't retire the first. Every existing in-repo caller (the integrations/verl/ RemoteBackend, the recipes/rl/skyrl/ recipes) has to be rewritten before arctic_platform/rl/client.py can be deleted.

Plan needed: which caller migrates on what PR; when rl/*_client.py gets removed. Otherwise "one client" becomes "one-plus-legacy" indefinitely.

P4 — Fill the remaining op-parity gap before deleting the old client

Present in arctic_platform/rl/*_client.py, missing from ArcticRLClient: sleep_/wake_{inference,training,log_prob}, empty_training_cache, weight_norm. Not on the immediate critical path for the near-term caller migration, but they block P3 (deletion of the async clients). Enumerate them in UNIFICATION_NOTES.md under a "not yet in unified client" section so the gap is tracked.


Not blocking

Everything else — Ray transport event-loop-per-op churn, branch hygiene (all three PRs are behind main past #36; pr-45's branch also carries stray commit e697039 from #32), shutdown() traversal order, no OnPremTransport unit tests, TLS / retry-adapter / per-request-timeout hardening on both HTTP paths, hard-coded polling & chunk-size constants in CortexTransport, subprocess stderr capture in _launch_server, silent-except on _destroy/shutdown, no mock-server end-to-end _poll test, PR #46 description overstating the delivered surface ("+ dss backend" isn't delivered until this PR as backend="cortex") — all real, none change the shape of the unification. Track as ordinary follow-ups, don't gate merge.

Merge sequence

Land in order #45 → #46 → #47, each rebased onto current main. Fix F1 and F2 in the PRs they live in (both are single-digit LOC). F3, P1, P2, P3, P4 belong on the follow-up plan; they should have owners and target-PR rows in UNIFICATION_NOTES.md before the first downstream caller starts migrating, otherwise the client-side work in this stack lands in front of server-side prerequisites and we relitigate it under time pressure later.

Add the Cortex (cortex-training) transport over the SnowAPI GS REST surface: an
op->handler table that builds the CreateJob wire schema, submits each op, and
polls the async request to completion. Add the DSSST1 safetensors wire codec
(wire.py) it depends on -- the single serialization source of truth (codec +
op metadata + byte chunking), pickle-free on decode. Wire cortex into the config
(cortex backend + cortex_* fields), make_transport, and the example.

Tests: tests/client/test_wire.py (roundtrip, chunking, pickle rejection) and
tests/client/test_cortex.py (wire schema, sub-job gating/order, result decoders).

Co-authored-by: Cursor <cursoragent@cursor.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