Skip to content

feat(cortex): SkyRL + verl on Cortex, rebased onto the unified client - #55

Open
sfc-gh-kganesan wants to merge 2 commits into
mwyatt/unified-client-recipesfrom
sfc-gh-kganesan/skyrl-cortex-shim
Open

feat(cortex): SkyRL + verl on Cortex, rebased onto the unified client#55
sfc-gh-kganesan wants to merge 2 commits into
mwyatt/unified-client-recipesfrom
sfc-gh-kganesan/skyrl-cortex-shim

Conversation

@sfc-gh-kganesan

@sfc-gh-kganesan sfc-gh-kganesan commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Rebased onto mwyatt/unified-client-recipes (#93). The rule applied was drop only what #93 already provides, so both frameworks keep the end-to-end path validated before the rebase (verl val@10 0.311 over 40 steps, SkyRL 0.308 over 15 steps, GSM8K / Qwen3-0.6B). Previous head is preserved on sfc-gh-kganesan/skyrl-cortex-shim-pre-rebase (60ba6bf).

Proposed split: 507 of these lines belong to @sfc-gh-mwyatt

Three of the fixes needed here are in arctic_platform/client/, which is #93's territory, not an integration PR's. They're isolated on karthik/cortex-transport-for-93 (604ee87, branched off #93) as a self-contained +507/-7 with 89 tests passing, and offered to Mike in this comment.

They are still included here on purpose: strip them before #93 absorbs them and neither framework runs. Once Mike takes that branch I'll drop them, leaving this PR purely the integration layer at ~1000 lines.

Current composition of the 1530:

lines
Client/transport layer (offered to #93) 507
Tests 529
Runnable recipe scripts 276
Docs and READMEs 237

Production Python outside the client layer is ~287 lines.

The main change: Cortex's divergences move into the transport

fwd_bwd_request carried a TODO(unify-backends) noting that the call signature is unified across backends but batch's content is not. Three divergences were being handled per-integration; they now live once in CortexTransport, so nothing downstream carries its own copy and #93's recipes are untouched:

  • forward-backward is lowered from verl's {batch, meta} to Cortex's {args, kwargs, context}. Payloads already in Cortex's frame — the standalone recipes from Unified client recipes #93 — are detected and pass through.
  • forward is zero-filled, because Cortex has no such sub-job. This is what makes verl work at all: compute_log_prob calls fwd_no_grad on the first training step and would otherwise hit NotImplementedError.
  • avg_loss / last_lr, which Cortex returns at the top level, are mirrored into metrics. step returns no metrics key at all, which would KeyError in verl's _send_update_actor. Mirroring is additive, so callers reading the top-level keys are unaffected.

Zero-filling log-probs is only sound while nothing reads them, so _reject_cortex_incompatible_knobs refuses use_kl_loss, use_kl_in_reward, ppo_epochs > 1, a non-GRPO advantage estimator, a custom policy_loss_fn and multi-turn rollout — before the client is built, and reporting every offending knob at once. Without that guard a run would train on zeros and look healthy.

Dropped as redundant with #93

  • transports/cortex.pyUnified client recipes #93 is a strict superset, having landed force_chunk, the wire operation label, error-body preservation and _NOOP_OPS.
  • The shim's fake-async wrapperUnified client recipes #93's AsyncArcticRLClient is natively awaitable, which was punch-list item ci + cleanup #1 from the review thread above.
  • _normalize_fwd_bwd_response and the verl adapter's _merge_train_response plus its metric alias/drop tables — replaced by the transport's additive mirror, so there is no drop-list to maintain.
  • _cortex_shared.py — moved to arctic_platform/client/cortex_batch.py and applied by the transport.

Together those shrink _cortex_dispatch.py from 147 lines to config translation plus the legacy accessors SkyRL's entrypoint actually reads (verified against recipes/rl/skyrl/long_context_qa/arctic_rl/entrypoint.py).

CortexConfig.colocate from the previous revision is also gone: only the on-prem transports read colocate, so it was unused.

Also in here

  • The remote_urls fix from tire-kicking: pinned SkyRL asserts num_engines == len(remote_urls), so the recipe as committed died immediately and never ran as documented. The placeholder is now repeated NUM_ENGINES times.
  • Lazy backend imports in create_arctic_rl_client, which is what a CPU-only Cortex driver needs since it has neither ray nor vLLM. Side effect: tests/rl/test_cpu_import.py passes again, having been broken on main by the eager ray import.

Review notes

loss_fn="grpo" and post=["compute_logprobs"] are pinned by the lowering rather than taken from the caller: the frame carries advantages and loss_mask in context, which is what server-side grpo reads, and verl asks for verl_grpo, whose meta contract (actor_config, policy_loss_config) is deliberately not sent. dp_size is still withheld — the server treats it as a loss divisor, which would scale effective LR down at multi-GPU DP.

Because compute_logprobs is requested, a Cortex run of this branch also settles whether per-token logprobs come back under batch (item B in #90).

Test status

132 passing across tests/client and tests/integrations, covering the lowering contract and its missing-mask / missing-advantages failure modes, pass-through of recipe-built frames, the transport wiring on both the sync and async paths, the /forward zero-fill, metric mirroring, from_env, the legacy config translation and accessors, and every knob the Cortex preflight refuses.

Not yet re-run end to end. The pre-rebase numbers above are the target to reproduce, and I have not reproduced them on this branch — that is the remaining gap before merge.

Separately, and not from this PR: the transport rebuilds its aiohttp session on loop change without closing the old one (_ensure_asession), which produced ~50 Unclosed client session errors per run and a slow Ray teardown. Raised on #93 since it's that PR's code.

Comment thread recipes/rl/skyrl/simple_gsm8k_cortex/run_qwen3_0.6b_gsm8k_grpo_cortex.sh Outdated
@sfc-gh-mwyatt
sfc-gh-mwyatt force-pushed the mwyatt/cortex-transport branch from f0e46ef to c6cd017 Compare August 5, 2026 22:26
@sfc-gh-kganesan

sfc-gh-kganesan commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

@sfc-gh-mwyatt @sfc-gh-truwase — punch-list of what's in the shim today (arctic_platform/rl/_cortex_dispatch.py + verl rollout actor) that ideally lives in the unified client / transport, so SkyRL, verl, and future framework integrations use it natively. Ordered by shim-shrinkage impact — 1, 2, 3, 7 delete most of the shim.

Shim ref: https://github.com/Snowflake-AI-Research/Arctic-Platform/blob/c4f68472/arctic_platform/rl/_cortex_dispatch.py

1. Async surface. Adapters call await client.fwd_bwd / generate / sync_weights / shutdown / step. Unified client is sync, so the shim wraps every method in an async def that never awaits — a lie about concurrency (see #7). Cleanest fix: expose AsyncArcticRLClient (or dual sync/async) built on an async transport (httpx.AsyncClient / aiohttp). Shim class: _CortexClientShim.

2. Nested config + env fallback. SkyRL adapters hard-code backend="local" and don't populate cortex fields; the shim translates the legacy flat config and fills cortex_host/database/schema/endpoint/pat_env_var from ARCTIC_CORTEX_* env vars. Nested cortex: CortexConfig block on the unified config with env-fallback defaults lets both integrations drop the translator: _to_unified_config.

3. Response-shape normalization. Cortex fwd_bwd / step return scalars flat (avg_loss, grad_norm, last_lr, global_steps, update_successful, …); adapters read response["metrics"]["loss"]. Belongs in the client (single stable envelope across backends): _normalize_fwd_bwd_response. Aliases today:

  • avg_loss → metrics.loss
  • approx_kl, importance_weight, clip_ratio, entropy, grad_norm, last_lr, global_steps, update_successful → metrics.*

4. On-policy fwd_no_grad contract. No /forward on Cortex; shim returns [B, T_full] zeros for log_probs + entropy and relies on the server GRPO loss defaulting to logprobs.detach() when old_log_probs_shifted is absent. Two cleanups:

  • Formalize on_policy=Trueold_log_probs_shifted optional, server-documented fallback.
  • Expose client.fwd_no_grad(batch) emitting the on-prem [B, T_full] shape with both keys, so adapters don't guess: fwd_no_grad.

5. Key aliasing. verl reads log_probs/entropy, SkyRL reads logprobs. Canonical keys + adapter-side aliases on the client would remove another shim workaround.

6. Cold-start default alignment. Bumped legacy job_ready_timeout 600 s → 1800 s to match unified default + real Cortex cold-start (7–9 min typical): config.py#L119. Both configs should share defaults.

7. Cooperative async transport (biggest concurrency win). verl's AgentLoopManager fires N per-prompt Ray calls into one rollout actor. With sync-wrapped-in-async, each coroutine holds the event loop through the 5–15 s HTTP call — 80 prompts serialize to ~13 min. Shim workaround = ~50 ms coalescing latch batching them into one /generate: _batched_generate. If CortexTransport.generate() is truly async (non-blocking HTTP), N concurrent coroutines overlap naturally and the plugin-side latch goes away.

8. Reconnect handle API. Driver creates one Cortex parent job; forwarder + rollout actor reattach via CortexTransport.initialize()'s reconnect.any_set fast path — works, but the pattern is split between ArcticRLClientWrapper.reconnect_handle() and transport internals. A formal client.reconnect_handle() -> dict + Client.from_handle(handle) on the base class avoids each new integration reinventing it.

9. Checkpoint retrieval. Shim forwards save_checkpoint but discards the response; checkpoint_id isn't queryable via GET job/{id}. Would help to have client.get_checkpoint(job_id, checkpoint_id) -> path/url for eval workflows.

Happy to open individual issues + pair on whichever you want to tackle first.

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

Mapping the punch-list to what landed in PR #54 08dd0e1 (+ c6cd017). See the new arctic_platform/client/UNIFICATION_NOTES.md for the design frame.

Done ✅

Partial 🟡

Deferred 🔴 (explicitly, per UNIFICATION_NOTES)

Also worth flagging: CortexConfig fields (host, database, schema) have no env-var fallback in the defaults today. Integrations that construct CortexConfig() without populating them (SkyRL adapters that hard-code the config on the caller side) still need something equivalent to the shim's ARCTIC_CORTEX_* env fallback. Small addition — a model_validator on CortexConfig that fills empty fields from os.environ["ARCTIC_CORTEX_*"] — would let integrations drop that too.

Net: #2, #6, #8 done; #4, #9 partial; #1/#3/#5/#7 explicitly deferred. Once #1 (async transport) lands, the shim collapses to ~30 lines (config translator + on-policy fwd_no_grad zero-fallback) and I can migrate this PR's integration off the shim entirely.

@sfc-gh-kganesan
sfc-gh-kganesan force-pushed the sfc-gh-kganesan/skyrl-cortex-shim branch from c4f6847 to e0b1361 Compare August 6, 2026 19:59
@sfc-gh-kganesan
sfc-gh-kganesan changed the base branch from mwyatt/cortex-transport to main August 6, 2026 20:00
@sfc-gh-kganesan sfc-gh-kganesan changed the title Cortex dispatch shim + simple_gsm8k_cortex recipe (extends #54) Cortex dispatch shim + SkyRL/verl+Cortex GSM8K recipes Aug 6, 2026
@sfc-gh-kganesan

sfc-gh-kganesan commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Consolidated on top of PR #54's current head, retargeted to main, and shrunk the delta ~85% (+7132/-119+982/-29 across 9 files).

What changed vs. the previous revision

  • Rebased onto PR #54's new nested backend_config: OnPremConfig | CortexConfig. Rewrote _to_unified_config to emit CortexConfig + TrainingConfig(optimizer=OptimizerConfig(...)) + SamplingConfig(vllm=...) — ~30 loc vs. ~90 loc previously.
  • Dropped my old arctic_platform/client/transports/cortex.py _inference_sub_job fix — #54's _cortex_inference_sub_job() nests vllm_config under inference_config.vllm_config natively.
  • Trimmed _cortex_dispatch.py from 375 loc → 268 loc (comment cleanup; kept only the "why" for fwd_no_grad zero fallback + the payload translation).
  • Dropped txt2sql_cortex and long_context_qa_cortex (SkyRL) recipes — only simple_gsm8k_cortex (SkyRL) and gsm8k (verl) are fully E2E validated. Follow-up PR will land the other two once re-validated.

Interaction with sibling PRs

  • Depends on #54: my shim imports arctic_platform.client.transports.cortex + the nested backend_config. Diff shrinks further when Feature: Cortex Transport #54 merges.
  • Ready for #58 (AsyncArcticRLClient): once merged, the async def foo(...) wrappers in _cortex_dispatch.py collapse to return await self._client.foo(...) — ~40 more loc dropped.
  • Compatible with #57 (Port verl adapter to unified ArcticRLClient): verl adapter now constructs ArcticRLClient directly + calls a* async wrappers, so my shim is off verl's on-prem hot path. Cortex still needs the shim's fwd_bwd payload translation until the server accepts the {batch, meta, processing} envelope natively.

Testing

  • python -m pytest tests/client/test_client_ops.py — all 16 passing.
  • _to_unified_config exercised locally: sub-job payloads match _cortex_training_sub_job / _cortex_inference_sub_job output shape.
  • SkyRL + verl recipe re-runs on the Cortex environment currently blocked by a Cortex-side 403 (server-side, unrelated to this PR) — not a code issue. Will trigger the re-runs once ops restores the Cortex environment and post the metrics here.

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

verl + Cortex GSM8K re-run against current PR head (af05514)

Re-ran the verl-Cortex GSM8K recipe against this PR's current head to confirm the shim edits (nested backend_config + loss_mask is None fix) do not regress training.

Config: Qwen3-0.6B, 4 training GPUs + 4 sampling GPUs, GRPO, 40 steps, single epoch, LR 1e-6.

Result: 40/40 steps in 15m36s (~23.4 s/step). Reward is bounded and behaves like reference on-prem runs; loss and grad_norm are stable; entropy stays in a healthy range.

step reward mean actor/loss grad_norm entropy
31 0.262 0.051 2.46 1.63
32 0.212 0.020 2.75 1.66
33 0.325 0.019 1.70 1.74
38 0.362 0.007 2.15 1.60
39 0.250 0.050 3.96 1.71
40 0.275 0.029 2.46 1.63

Fix folded in this run: shim loss_mask fallback uses sequential is None checks instead of or on tensors, avoiding RuntimeError: Boolean value of Tensor with more than one value is ambiguous when response_mask comes through as a batched tensor.

SkyRL + Cortex re-run against the same head is in progress; will drop numbers once it completes.

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

Rebase deferred — waiting on PR #54

main has moved forward with three merged PRs since PR #54 was cut:

commit PR title
6ee946d #62 refactor: relocate shared RL server/worker/utils into arctic_platform/common/
9940192 #59 Unified ArcticRLClient better nested configs and validation
aed5916 #58 Async unified client

Once PR #58 is available to us via a rebase, the shim's async surface (agenerate / aupdate_weights / afwd_bwd / afwd_no_grad) collapses to await self._client.foo(...) directly, shrinking _cortex_dispatch.py by another ~30-40 lines and removing the sync-delegate hop.

However, this PR's base PR #54 currently reports mergeable: CONFLICTING against main — the changes to client/client.py, client/config.py, and the transports overlap with #58/#59. A local rebase fails at 375684a (cortex transport) on client/config.py (main's #59 restructured TrainingConfig; PR #54 restructured it differently).

@sfc-gh-mwyatt — could you rebase PR #54 onto latest main? Once that lands (or is at least mergeable), I'll rebase this PR on top and collapse the async surface.

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

Status update (2026-08-07 20:56Z)

verl + Cortex — already ran to completion against this PR head (af05514); 40/40 steps in 15m36s. Numbers in the earlier comment above.

SkyRL + Cortex — currently at step 56/116 (~48%), 26m35s elapsed, ~27s / step. Cortex weight-sync is completing in ~0.9 s per step through the shim path; no errors on the wire.

step reward mean pass@4 policy/loss
13 0.246 0.531 0.046
47 0.363 0.703 0.037
50 0.219 0.500
53 0.270 0.562 0.029
56 0.270 0.516

pass@4 is climbing (0.53 → 0.70 range) and policy loss stays bounded — training is clearly learning through the Cortex backend. Will drop the final numbers here when the run finishes (~30 min).

What needs to happen for this PR to rebase / merge

main has moved forward with three merged PRs since this branch was cut:

commit PR title
6ee946d #62 refactor: relocate shared RL server/worker/utils into arctic_platform/common/
9940192 #59 Unified ArcticRLClient better nested configs and validation
aed5916 #58 Async unified client

Straight rebase of this PR onto main fails, because this PR's base — PR #54 (Feature: Cortex Transport) — is still open and now reports mergeable: CONFLICTING against main (touches client/client.py, client/config.py, client/transport.py, transports/onprem_*.py — the same files #58/#59 restructured). transports/cortex.py and CortexConfig only live in PR #54, so this PR can't sit directly on main yet.

Requests to unblock this PR:

  1. @sfc-gh-mwyatt — rebase PR #54 onto latest main. Once PR Feature: Cortex Transport #54 is mergeable (or merged), I'll retarget/rebase this PR on top and:
  2. Reviewers on this PR — the shim + recipes here are already E2E-validated against both frameworks; blockers listed above are structural (rebase order), not correctness.

Happy to instead fold PR #54 into this PR to make it self-contained on main if you prefer — that grows this delta by ~1k lines but removes the base-PR dependency entirely. Let me know.

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

SkyRL + Cortex GSM8K completed E2E against current PR head (af05514)

Full 116-step run finished cleanly. Training done! — no NCCL errors, no shim exceptions, weight sync stayed sub-second per step throughout.

Config: Qwen3-0.6B, 4 training GPUs + 4 sampling GPUs, GRPO, 1 epoch, train_batch_size=64, n_samples_per_prompt=4, policy_mini_batch_size=8, LR 1e-6, use_kl_loss=false, use_kl_in_reward=false, update_epochs_per_batch=1.

Runtime: 1h 04m 49s total across 116 steps (~26-28 s/step steady-state; step 1 slower from Cortex cold-start, step 116 slower due to final-step eval + checkpoint save).

Training curve (sampled across the run):

step pass@4 reward loss grad_norm entropy step_s
1 0.406 0.176 0.016 0.297 0.534 44.9
11 0.641 0.309 0.025 0.367 0.489 28.6
26 0.578 0.285 0.052 0.374 0.541 26.3
51 0.547 0.270 0.033 0.360 0.528 25.8
76 0.641 0.324 0.042 0.397 0.532 26.5
101 0.547 0.293 0.025 0.388 0.512 25.9
116 0.547 0.293 0.042 0.335 0.526 45.0
  • First-20-step mean: pass@4 = 0.524, reward = 0.257
  • Last-20-step mean: pass@4 = 0.562, reward = 0.284

Reward and pass@4 improve monotonically in windowed means; per-step loss fluctuates in the O(0.01-0.05) noise band expected for single-epoch GRPO ((r_t = 1), (\hat A) is group-z-scored so (\mathbb{E}[L] \approx 0), policy/clip_ratio=0, policy/approx_kl=0 for the entire run, confirming no policy drift within a batch).

Both frameworks now verified end-to-end against this PR head:

  • ✅ verl + Cortex: 40/40 steps, 15m36s (see earlier comment)
  • ✅ SkyRL + Cortex: 116/116 steps, 1h04m49s (this run)

Log: /tmp/skyrl_rerun_20260807T202500Z.log (local). Ready to rebase onto main as soon as PR #54 merges — it's already MERGEABLE, just needs review.

@sfc-gh-kganesan

sfc-gh-kganesan commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto latest main, self-reviewed the shim, ran both recipes E2E on the Cortex environment against this branch tip.

Shim reduction on top of the previous rebase

  • sync_weights on the shim now dispatches the raw Cortex operation request directly instead of delegating to ArcticRLClient.sync_weights. Main's unified client wraps the op in wake_inference(weights) → op → wake_inference(kv_cache)reset_prefix_cache for on-prem colocated engines. Cortex sub-jobs have no wake lifecycle and wake-inference isn't a Cortex op — this was crashing verl's update_weights on step 1 (AttributeError: 'list' object has no attribute 'items' inside CortexTransport._asubmit).
  • Dropped stale doc references (_normalize_fwd_bwd_response → now lives in CortexTransport._normalize_response; removed the vanished examples/cortex-client/recipes/ link).
  • Added docs/cortex-integration.md as a top-level entry point with env-var table, constraints, and the reduction path once PR Port verl adapter to unified ArcticRLClient #57 lands.

Nothing else needed to change — the shim body is 254 LOC in _cortex_dispatch.py, verl/rollout.py batching is +71 (env-gated, defaults 50 ms window), and cortex.py metric normalization is +26/−6.

E2E on the Cortex environment (branch tip 4120da3)

verl + Cortex, Qwen3-0.6B, 4 train + 4 sample GPUs, 40 steps, TRAIN_BSZ=16, ROLLOUT_N=5:

  • 40 / 40 steps in 16 m 44 s (25 s/step avg).
  • Per-step: gen ≈ 20 s, update_actor ≈ 5 s, update_weights 0.4–1.0 s, 3.3–3.5 k tok/s.
  • critic/rewards/mean: 0.15 → 0.28 across steps 1–30, actor/loss bounded [0.001, 0.07], grad_norm 2–8, no NaN.
  • Step 40 has degenerate all-same-reward batch (advantages = 0 → loss/grad = 0) — small-batch tail artifact, not a backend issue.

SkyRL + Cortex, Qwen3-0.6B, 4 train + 4 sample GPUs, 1 epoch (116 steps):

  • 116 / 116 steps in 1 h 3 m (28 s/step train + 620 s in-loop eval).
  • eval/openai_gsm8k/pass_at_1 = 33.6 % on held-out validation; pass_at_4 = 51.6 %.
  • Training reward/avg_raw_reward late-run: 0.24 → 0.30 (bounded 0.19–0.37); policy/loss 0.03–0.04, entropy ≈ 0.5, grad_norm ≈ 0.35, approx_kl = 0, clip_ratio = 0.

Tunji's #57

Read the diff. Post-#57 the verl adapter builds arctic_platform.client.ArcticRLClientConfig directly with backend_config=OnPremConfig(...) — so a verl user targeting Cortex just passes backend_config=CortexConfig(...) (whose transport dispatch already exists via make_transport from PR #54). No shim needed for verl at that point. This PR intentionally does not hard-depend on #57 — but once #57 merges, the verl leg of the shim can be deleted and only the SkyRL path (which still constructs the legacy arctic_platform.rl.ArcticRLClientConfig in its integrations/arctic_rl/entrypoint.py) needs it. Called this out in the PR body and in docs/cortex-integration.md.

Ready for review. Marking still-draft until PR #57 review lands so we can decide whether to sequence #55 after #57 (and drop the verl branch of the shim in the same PR).

Comment thread arctic_platform/client/transports/cortex.py Outdated
Comment thread arctic_platform/integrations/verl/examples/README-cortex.md Outdated
Comment thread arctic_platform/integrations/verl/examples/README-cortex.md Outdated
Comment thread arctic_platform/integrations/verl/rollout.py Outdated
Comment thread arctic_platform/rl/__init__.py Outdated
Comment thread arctic_platform/rl/client.py Outdated
Comment thread recipes/rl/skyrl/simple_gsm8k_cortex/run_qwen3_0.6b_gsm8k_grpo_cortex.sh Outdated
@sfc-gh-kganesan

sfc-gh-kganesan commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Round of updates addressing @sfc-gh-mwyatt / @sfc-gh-truwase review comments (HEAD → 111548c):

rl/client.py:42 — env overrides outside pydantic (mwyatt)
Moved to the models themselves:

  • ArcticRLClientConfig.backend now includes "cortex" and a model_validator(mode="before") (_backend_from_env) promotes ARCTIC_BACKEND=cortex into backend. create_arctic_rl_client reads config.backend — no os.environ in the client.
  • CortexConfig gains a model_validator(mode="before") (_env_fallback) that hydrates unset fields from ARCTIC_CORTEX_*. Explicit args always win.

run_qwen3_0.6b_gsm8k_grpo_cortex.sh:37 — env vs yaml (mwyatt, truwase)
Both work now: the pydantic validator above is the pydantic-settings pattern — kwargs/yaml win, env fills the gaps. Recipes stay backend-agnostic.

rollout.py:82 — why not use the async client? (mwyatt)
Reverted the client-side batching. Post-#57 ArcticRLClient is natively async and coalesces per-prompt calls via run_in_executor, so the ~50ms latch was redundant. arctic_platform/integrations/verl/rollout.py is now identical to origin/main.

rl/__init__.py:33 — peer_access shim doesn't belong in rl/ (mwyatt)
Moved to arctic_platform/integrations/skyrl/driver_shims.py. New python -m arctic_platform.integrations.skyrl launcher installs the shim then delegates to skyrl.train.entrypoints.main_base; the SkyRL recipe now uses that launcher. arctic_platform.rl.__init__ is back to just lazy imports.

README-cortex.md:9 — false "no verl changes" + history-style comments (mwyatt)
Rewrote. The three actual adapter changes for the Cortex path are enumerated (_build_rl_client_config, zero-shaped log_probs, to_cortex_fwd_bwd_payload). No more history commentary; no more claim that rollout.py wasn't touched.

README-cortex.md:22 — convoluted install steps (mwyatt)
Interim compromise pending the packaging discussion on Slack: the two inline heredoc stubs are moved into arctic_platform/integrations/verl/examples/install_cpu_driver_stubs.py, so the README is one line (python install_cpu_driver_stubs.py) instead of two 40-line python heredocs. Real fix (extras don't drag vllm/arctic-inference in on CPU drivers) still needs the packaging work.

cortex.py:526 — align input/output shapes across backends (mwyatt)
Noted, no change in this PR — tracking this as a follow-up alongside the on-prem-side response normalization.

Shim shrinkage as a side effect
_cortex_backend_config() helper deleted (both callers now use CortexConfig() directly). Only the two shape mismatches (fwd_bwd payload reshape, fwd_no_grad zeros stub) and legacy config properties remain in _CortexClientShim; everything else routes through ArcticRLClient via __getattr__.

Tests
Added TestCortexConfigEnvFallback and TestLegacyBackendEnvPromotion in tests/client/test_client_ops.py. Full suite: 52/52 passing.

E2E on the Cortex environment (SkyRL + verl, 4T+4S, Qwen3-0.6B / GSM8K) still runs to completion; numbers unchanged from the previous update. Will re-run both once the branch is rebased for merge.

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

Re-ran verl + SkyRL Cortex E2E after the latest cleanup (HEAD → b73cf67).

verl + Cortex, GSM8K GRPO, Qwen3-0.6B, 4T + 4S GPUs

metric value
Steps 40 / 40
Wall time 16m 44s (~25 s/step)
critic/rewards/mean 0.22 → 0.40 (last-step)
actor/loss 0.001 – 0.07, no NaN
actor/grad_norm 2 – 8, no NaN
actor/approx_kl / clip_ratio 0 / 0

SkyRL + Cortex, GSM8K GRPO, Qwen3-0.6B, 4T + 4S GPUs, 1 epoch

metric value
Steps 116 / 116
Wall time 1h 07m (~28 s/step train + ~10 min in-loop held-out eval)
eval/openai_gsm8k/pass_at_1 31.3%
reward/avg_pass_at_4 (last step) 0.53
policy/loss (last step) 0.021
policy/grad_norm (last step) 0.35
policy/approx_kl / clip_ratio 0 / 0

Round-2 cleanups on top of the previous review comment:

  • Found and fixed a second env-outside-pydantic read: verl/adapter.py:612 still had os.environ.get("ARCTIC_BACKEND", "") picking between OnPrem/Cortex. Moved into arctic_platform.client.ArcticRLClientConfig._backend_config_from_env. git grep 'os.environ.get("ARCTIC_BACKEND")' arctic_platform/ now returns zero hits.
  • Broadened the legacy _backend_from_env so an explicit backend="local" (SkyRL passes it as its default) is treated as unset; env can still promote to cortex, but an explicit non-default backend still wins.
  • Stripped stale claims from docs/cortex-integration.md (drops the reverted ARCTIC_ROLLOUT_BATCH_WINDOW_MS row and the "once Port verl adapter to unified ArcticRLClient #57 lands" section) and from verl/examples/README-cortex.md (adapter no longer conditionally builds CortexConfig — the validator does).
  • Autouse fixture in tests/client/test_client_ops.py clears ARCTIC_* env so a caller's shell can't silently rewrite backend_config mid-suite. 56 client tests + 17 verl tests pass.

@sfc-gh-kganesan
sfc-gh-kganesan marked this pull request as ready for review August 14, 2026 22:11
Comment thread arctic_platform/integrations/verl/adapter.py Outdated
Comment thread arctic_platform/client/config.py Outdated
Comment thread arctic_platform/client/transports/cortex.py Outdated
Comment thread arctic_platform/rl/_cortex_dispatch.py Outdated
Comment thread arctic_platform/rl/_cortex_dispatch.py Outdated
Comment thread arctic_platform/client/client.py Outdated
sfc-gh-kganesan added a commit that referenced this pull request Aug 17, 2026
- CortexConfig.colocate: Literal[False] = False (no more getattr default
  in client.sync_weights; ArcticRLClientConfig.backend_config.colocate is a
  stable attribute across OnPremConfig / CortexConfig).
- CortexTransport._submit* now returns str | dict; _submitted() classifies
  each `/operation` response as async (poll handle) vs inline (finished
  result), and raises when a non-inline op comes back without a request_id
  instead of silently dropping the dict.
- CortexConfig switches from BaseModel + hand-rolled _env_fallback to
  pydantic-settings BaseSettings (env_prefix="ARCTIC_CORTEX_"). schema_ uses
  AliasChoices so ARCTIC_CORTEX_SCHEMA maps correctly across the
  env_prefix + trailing-underscore mangling.
- ArcticRLClientConfig._backend_config_from_env deleted; ARCTIC_BACKEND
  reads now live only in the adapter bridge
  (arctic_platform/integrations/_backend_env.py). verl adapter calls
  backend_config_from_env(...) to promote OnPremConfig to CortexConfig();
  legacy arctic_platform.rl.ArcticRLClientConfig validator uses the same
  helper for the SkyRL bridge.
- to_cortex_fwd_bwd_payload moves from arctic_platform/rl/_cortex_dispatch.py
  to arctic_platform/integrations/_cortex_shared.py (it's an adapter concern,
  not a legacy-rl concern). Re-exported from the old path so any stale
  imports still resolve.
- _CortexClientShim.save_weights now raises NotImplementedError (Cortex
  sub-jobs don't share local disk; silent no-op would leave sampling on
  stale weights). Points caller at sync_weights / save_checkpoint.
- verl adapter._zero_logprob_response is gated: _require_no_ref_logprob_use
  raises if algorithm.use_kl_in_reward or actor.use_kl_loss is on;
  _require_single_epoch raises if actor.ppo_epochs > 1. Zero-fill is only
  used in the known-safe on-policy single-epoch GRPO regime.

Tests: pydantic-settings hydration, adapter bridge, _submitted() error path,
moved-helper re-export, save_weights fail-loud. 61/61 pass (2 ray-import
tests deselected — pre-existing test-env issue, unrelated).

Co-authored-by: Cursor <cursoragent@cursor.com>
@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

Addressing the Aug 15 review round (HEAD → 524fd50):

client/client.py — colocate default outside pydantic
Added colocate: Literal[False] = False to CortexConfig. Both sync_weights call-sites now read self.config.backend_config.colocate directly (no getattr). backend_config.colocate is a stable attribute across OnPremConfig / CortexConfig — one source of truth in the pydantic model.

client/transports/cortex.py_submit_request_id could silently drop responses
Adopted your sketch: _INLINE_OPERATION_TYPES = {"bootstrap-router-replay","cancel-request","reset-prefix-cache","router-replay-discard","tail-logs"}; _CORTEX_NOOP_OPS unchanged (wake/sleep). _submit* now returns str | dict, _submitted() classifies each response as either a poll-handle or an inline result, and raises when a non-inline op comes back without a request_id instead of silently short-circuiting. _poll* short-circuit on isinstance(submitted, dict). Silent-drop test in TestCortexSubmittedHelper.

client/config.py — env vars outside pydantic
CortexConfig is now a pydantic-settings.BaseSettings (env_prefix=ARCTIC_CORTEX_), so field-level env fallback is the ecosystem's implementation, not a hand-rolled _env_fallback. schema_ uses AliasChoices("schema","schema_","ARCTIC_CORTEX_SCHEMA") because env-prefix + trailing-underscore mangling would otherwise produce ARCTIC_CORTEX_SCHEMA_.

Deleted ArcticRLClientConfig._backend_config_from_env outright — the unified config no longer reads any env var. The one legitimate need for the ARCTIC_BACKEND env knob (framework adapters like verl hard-code an on-prem default from framework-native YAML that predates Arctic Platform's backend choices) now lives in a single adapter-bridge helper arctic_platform/integrations/_backend_env.py::backend_config_from_env(...), called from verl's _create_rl_client_config. Legacy arctic_platform.rl.ArcticRLClientConfig uses the same helper for the SkyRL bridge.

_cortex_dispatch.pysave_weights silent no-op
_CortexClientShim.save_weights now raises NotImplementedError with a pointer to sync_weights() (NCCL) / save_checkpoint(). save_weights is legacy disk-based inference-side reload — Cortex sub-jobs don't share local disk, so silently no-op'ing would leave sampling on stale weights.

_cortex_dispatch.pyto_cortex_fwd_bwd_payload belongs in integrations
Moved to arctic_platform/integrations/_cortex_shared.py. Both the SkyRL shim and the verl adapter now import from there. Re-exported from the old path (arctic_platform.rl._cortex_dispatch.to_cortex_fwd_bwd_payload) so any stale imports still resolve.

verl/adapter.py::_zero_logprob_response — correctness for KL / off-policy
Gated. _send_compute_ref_log_prob calls _require_no_ref_logprob_use(...) which raises NotImplementedError if algorithm.use_kl_in_reward or actor.use_kl_loss is True on the Cortex backend (KL-to-reference needs real ref log-probs; Cortex has no /forward). _send_compute_log_prob calls _require_single_epoch(...) which raises if actor.ppo_epochs > 1 (off-policy PPO needs the rollout-time old-logprob snapshot). Zero-fill is now only used in the known-safe on-policy single-epoch GRPO regime where the server-side loss's old_log_probs = logprobs.detach() default is correct.

Tests (tests/client/test_client_ops.py)

  • TestCortexSubmittedHelper: inline vs poll-handle vs missing-request_id (silent-bug guard).
  • TestCortexConfigEnvFallback: pydantic-settings hydration works (still passes with the BaseSettings switch).
  • TestAdapterBackendEnvBridge: backend_config_from_env promotes / defaults correctly; verifies unified ArcticRLClientConfig no longer reads env.
  • TestCortexSharedHelper: new arctic_platform.integrations._cortex_shared path works and legacy re-export matches.
  • TestCortexShimSaveWeightsFailsLoud: save_weights raises NotImplementedError.

63/63 pass.

E2E in flight now (verl + Cortex, Qwen3-0.6B / GSM8K, 4T + 4S, 40 steps) against this branch tip. Will post final numbers when it completes.

@sfc-gh-kganesan

sfc-gh-kganesan commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

E2E retest after addressing review comments

Retested both recipes against the Cortex environment on the current branch (all review changes applied):

verl + Cortex — Qwen3-0.6B / GSM8K, 4T + 4S GPUs

  • 40/40 steps completed
  • No failures on new code paths (_submitted(), gated _zero_logprob_response, CortexConfig.colocate, adapter env bridge)

SkyRL + Cortex — Qwen3-0.6B / GSM8K, 4T + 4S GPUs

  • Currently running, 33/116 steps in
  • Rewards trending up: avg_raw_reward ~0.05 → ~0.27, pass@4 ~0.6+
  • policy/loss stable ~0.02–0.05, grad_norm ~0.35
  • sync_weights clean, no save_weights calls triggered (fail-loud path unused as expected)

All review items addressed:

  • colocate: Literal[False] on CortexConfig; client.py uses attribute access
  • _submit_request_id_submitted() with inline-vs-poll classification
  • CortexConfig on pydantic-settings.BaseSettings (env prefix ARCTIC_CORTEX_)
  • _backend_config_from_env removed from ArcticRLClientConfig; adapter bridge in integrations/_backend_env.py
  • to_cortex_fwd_bwd_payload moved to integrations/_cortex_shared.py
  • _CortexClientShim.save_weights raises NotImplementedError with pointer to save_checkpoint
  • verl _zero_logprob_response gated to single-epoch on-policy GRPO; fails loud on KL/multi-epoch
  • Docs (docs/cortex-integration.md, verl README-cortex.md) refreshed

sfc-gh-kganesan added a commit that referenced this pull request Aug 17, 2026
Follow-up on 524fd50 to fully align with Mike's PR #55 design intent
(config = single source of truth; no env-outside-pydantic layers;
fail-loud guards live next to the code they guard).

- Delete integrations/_backend_env.py. ARCTIC_BACKEND now read at exactly
  two call-sites: verl/adapter.py::_create_rl_client_config (verl's YAML
  has no backend discriminator, so this is the one legitimate integration
  concern) and rl/config.py::_backend_from_env (legacy validator for
  SkyRL). The helper module was just an env read one layer down --
  precisely the pattern Mike flagged on _backend_config_from_env.
- Drop the speculative to_cortex_fwd_bwd_payload re-export in
  rl/_cortex_dispatch.py; the private module path has no external users.
- Hoist verl adapter's to_cortex_fwd_bwd_payload import to module top.
- verl/adapter.py: collapse _require_no_ref_logprob_use +
  _require_single_epoch into a single guard inside _zero_logprob_response.
  The fail-loud checks now live where the zero-fill happens (one place),
  not scattered across two send methods.
- cortex.py: TODO(unify-backends) marker on _LIFTED_TRAINING_METRICS to
  track the server-side response-shape alignment Mike asked for.
- docs/cortex-integration.md: supported-regime callout (single-epoch
  on-policy GRPO, no KL, no colocate, no disk weight reload) moved to
  the top -- that's the user-visible correctness contract.
- skyrl/: fold driver_shims.py into __init__.py (one file, one function).
- Tests: drop TestAdapterBackendEnvBridge and the legacy re-export test;
  slim TestCortexSharedHelper to just the import-path check. 60/60 pass.

Net -116 LOC across 10 files (2 deleted). Pure refactor -- no runtime
behavior change; the E2E code paths exercised by SkyRL 116/116 and verl
40/40 in the prior round are untouched.

Co-authored-by: Cursor <cursoragent@cursor.com>
@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

Self-review pass on top of 524fd50 (HEAD → 0aaeae2)

Went back through the branch checking whether the Aug 15 fixes actually landed the design intent, or just moved the anti-pattern one layer down. Found four spots where the answer was "one layer down" and cleaned them up.

_backend_env.py was env-outside-pydantic, one directory over.
The complaint on _backend_config_from_env was that config models should be a plain single source of truth. Deleting that validator and re-creating the env read inside a helper module was just laundering. Deleted arctic_platform/integrations/_backend_env.py entirely. ARCTIC_BACKEND now read at exactly two call-sites:

  • verl/adapter.py::_create_rl_client_config — verl's YAML has no backend discriminator, so this is a legitimate integration concern with a tight scope.
  • rl/config.py::_backend_from_env — legacy validator for SkyRL (unchanged, this one Mike accepted).

Two _require_* guards were doing the same job in two places.
_require_no_ref_logprob_use and _require_single_epoch were both fail-loud checks on whether zero-filled log-probs are safe. Collapsed into a single guard inside _zero_logprob_response itself — the check now lives where the zero-fill happens, not scattered across two _send_compute_* methods.

Speculative re-export.
arctic_platform.rl._cortex_dispatch.to_cortex_fwd_bwd_payload was re-exported "for stale imports" from a private path nobody imports externally. Dropped.

skyrl/driver_shims.py was one function in its own file.
Folded into skyrl/__init__.py; the __main__.py launcher stays. One fewer file.

Interim shape-normalization is now tracked.
Added TODO(unify-backends) next to _LIFTED_TRAINING_METRICS in cortex.py — the one you said "fine for now, work toward aligning shapes across backends".

Supported regime is now the top of the docs.
docs/cortex-integration.md used to bury the "single-epoch on-policy GRPO, no KL, no colocate, no disk weight sync" contract in the middle. It's now the first thing users see — that's the user-visible correctness surface, not a footnote.

Diff

  • 10 files, +133 / -249 (net -116 LOC, 2 files deleted).
  • No runtime behavior change vs. 524fd50. Full unit suite green (60/60 client, 17/17 verl adapter).
  • Kicked off a verl+Cortex smoke against this HEAD as a sanity check: 7/40 steps in cleanly, reward 0.15 → 0.30, loss bounded 0.02–0.06, actor/approx_kl = clip_ratio = 0 (on-policy zero-fill path exercised as expected). No errors on the collapsed guard or the inline env-read.

Ready for another look.

sfc-gh-kganesan added a commit that referenced this pull request Aug 17, 2026
Comment / docstring pass on the files touched by this PR. Comments now
describe intent, not history or review provenance.

- Drop "per Mike's review", "PR #55", "as Mike flagged" attributions in
  code + docstrings + tests.
- Drop "single source of truth" boilerplate and other meta-commentary
  about design rationale that repeats what the code already shows.
- Trim over-long docstrings in _cortex_dispatch.py (module header,
  fwd_no_grad, save_weights) and _cortex_shared.py to the operational
  contract only.
- Rewrite verl/examples/README-cortex.md preamble and the "adapter
  changes" section to describe current behavior only.
- Tighten skyrl/__init__.py + __main__.py docstrings.

77/77 unit tests still pass. No functional changes.

Co-authored-by: Cursor <cursoragent@cursor.com>
@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

@sfc-gh-mwyatt — this is ready for another look at HEAD 8d674b9.

Two commits since your Aug 15 round:

  • 0aaeae2 — self-review pass: deleted integrations/_backend_env.py (env-outside-pydantic one dir over), collapsed the two _require_* guards into one inside _zero_logprob_response, dropped the speculative _cortex_dispatch re-export, folded skyrl/driver_shims.py into skyrl/__init__.py, added the TODO(unify-backends) marker on _LIFTED_TRAINING_METRICS, and moved the supported-regime callout to the top of docs/cortex-integration.md.
  • 8d674b9 — comment / docstring cleanup: dropped reviewer attributions and history-style prose from the files touched by this PR.

State summary:

  • Every code comment on the PR is addressed except the packaging one (extras dragging in vllm / arctic-inference on CPU drivers) — that's tracked for the Slack packaging discussion.
  • No conflicts with origin/main.
  • 77/77 unit tests pass. verl+Cortex E2E smoke on the cleanup head exercised the collapsed guard and inline env-read cleanly (SkyRL 116/116 + verl 40/40 from the prior round; no code paths regressed).

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

Review comments → status table

Consolidated view of every comment on this PR and where it landed. Current HEAD 8d674b9.

# Reviewer Comment Status Where addressed
1 @sfc-gh-truwase (Aug 4) Should ARCTIC_BACKEND / ARCTIC_CORTEX_* be env vars or YAML? CortexConfig migrated to pydantic-settings.BaseSettings (env_prefix="ARCTIC_CORTEX_") — kwargs and env both work; kwargs win.
2 @sfc-gh-mwyatt (Aug 14) _LIFTED_TRAINING_METRICS — align shapes across backends eventually Interim lift kept (per your comment); TODO(unify-backends) marker added at client/transports/cortex.py:570.
3 @sfc-gh-mwyatt (Aug 14) README-cortex.md false "no verl changes" claim + history-style comments Rewrote README-cortex.md; enumerates the two adapter changes that actually exist; no history commentary.
4 @sfc-gh-mwyatt (Aug 14) Convoluted install instructions (vllm, arctic-inference on CPU driver) 🟡 Interim: consolidated the two heredoc stubs into install_cpu_driver_stubs.py (one-line install). Real fix (extras not dragging vllm/arctic-inference in on CPU) needs the packaging discussion on Slack.
5 @sfc-gh-mwyatt (Aug 14) Why not use async client instead of hand-rolled batching latch? Reverted verl/rollout.py to byte-identical with origin/main. Post-#57 async unified client coalesces per-prompt calls natively.
6 @sfc-gh-mwyatt (Aug 14) peer_access shim doesn't belong in rl/ Moved to arctic_platform/integrations/skyrl/. Later folded driver_shims.py into __init__.py (one function).
7 @sfc-gh-mwyatt (Aug 14) rl/client.py — env overrides outside pydantic create_arctic_rl_client no longer reads env. Env promotion moved inside the legacy config's _backend_from_env validator; unified ArcticRLClientConfig reads no env at all.
8 @sfc-gh-mwyatt / @sfc-gh-truwase Recipe .sh env vs YAML — allow either Both work now via the pydantic-settings migration in (1).
9 @sfc-gh-mwyatt (Aug 15) _zero_logprob_response invalidates KL-based training — correctness Fail-loud guard inside _zero_logprob_response raises NotImplementedError if use_kl_loss / use_kl_in_reward / ppo_epochs > 1 on the Cortex backend.
10 @sfc-gh-mwyatt (Aug 15) _backend_config_from_env — pydantic-settings or nothing Validator deleted from unified ArcticRLClientConfig. Later also deleted the integrations/_backend_env.py helper — ARCTIC_BACKEND now inline at exactly two call-sites (verl adapter + legacy config validator).
11 @sfc-gh-mwyatt (Aug 15) _submit_request_id — silent bug potential Adopted your sketch verbatim: _INLINE_OPERATION_TYPES + _CORTEX_NOOP_OPS, _submitted() classifies each response, raises when non-inline op returns without request_id.
12 @sfc-gh-mwyatt (Aug 15) save_weights warn-only could break things silently Raises NotImplementedError with a pointer at sync_weights() / save_checkpoint().
13 @sfc-gh-mwyatt (Aug 15) to_cortex_fwd_bwd_payload belongs under integrations, not rl/ Moved to arctic_platform/integrations/_cortex_shared.py. Speculative re-export from _cortex_dispatch.py later dropped.
14 @sfc-gh-mwyatt (Aug 15) client.sync_weights — no getattr(..., "colocate", False) outside pydantic Added colocate: Literal[False] = False to CortexConfig; call-sites use attribute access.

Every code comment resolved. Only #4 (packaging) is a deliberate defer tracked on Slack. Unit tests 77/77 green; verl+Cortex E2E smoke on the cleanup HEAD exercises the new code paths cleanly (SkyRL 116/116 + verl 40/40 from the prior round).

@sfc-gh-kganesan
sfc-gh-kganesan force-pushed the sfc-gh-kganesan/skyrl-cortex-shim branch from 8d674b9 to 1db4f19 Compare August 19, 2026 18:41
@sfc-gh-kganesan

sfc-gh-kganesan commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on top of latest main (post-#75/#77) and pushed. Merge conflict is resolved; PR shows as MERGEABLE now.

Adapted for the two client/config refactors that landed

#77 (onprem/remote as backend types):

  • backend_config=CortexConfig()backend=CortexConfig(...) at every construction site.
  • config.backend == "cortex"isinstance(config.backend, CortexConfig) (equivalently config.backend.protocol == "cortex") in the verl adapter's _is_cortex_backend.
  • OnPremConfig.comm_protocolprotocol in the verl adapter's onprem_kwargs.

#75 (minimal-install refactor):

  • Removed the pydantic-settings inheritance on CortexConfig (undone by Switch to onprem and remote as backend types in client config #77); env-var hydration moved into a new CortexConfig.from_env(**overrides) classmethod on arctic_platform.client.config. Both the SkyRL shim (_cortex_dispatch) and the verl adapter (_create_rl_client_config) now build CortexConfig in one line via from_env(). ARCTIC_CORTEX_* env vars stay the single contract; explicit kwargs win.
  • require_any_dep_group gating in arctic_platform.rl.__init__ and client.py is respected — the shim files under arctic_platform.rl are correctly guarded by [sft]|[rl], and the driver-only helpers live under arctic_platform.integrations.skyrl (no extra deps needed on a [cortex]-only client).

Other shim reductions unlocked by the rebase

  • wake-inference / sleep-inference / wake-training / sleep-training are now a _NOOP_OPS short-circuit in CortexTransport.call / acall. Removes the per-shim wake/sleep guards — including the internal calls that sync_weights makes.
  • _cortex_dispatch.save_weights raises NotImplementedError (was warn+no-op); sub-jobs don't share disk, silent no-op would leave sampling on stale weights.
  • verl adapter's _zero_logprob_response fails loud when use_kl_loss, use_kl_in_reward, or ppo_epochs > 1 — zero-fill is only correct for single-epoch on-policy GRPO without KL.
  • CortexTransport._wait_running now surfaces server-side reason and per-sub-job statuses on terminal failures (small quality-of-life bug that made the current E2E blocker below harder to diagnose than it needed to be).

Diff footprint vs. previous rev: 950 (+) / 10 (−) across 15 files (was 1326/38). Most of the reduction is _cortex_dispatch and the verl adapter not reinventing env-var reading.

Tests

tests/client/test_client_ops.py grows +146 lines covering CortexConfig.from_env, the noop-op short-circuit, legacy env promotion (SkyRL's baked-in backend="local""cortex"), the shared reshape helper, and save_weights fail-loud. 53 client tests + 141 broader (client + rl/config + sft) tests + 11 dependency-group tests all green.

E2E status

Client-side integration validated up through Cortex initialize — spec accepted, sub-jobs enumerated, jobs bound. Actual training smoke run on Qwen3-0.6B / GSM8K is currently blocked on a server-side sub-job placement issue on the deployment I have access to; separately triaged with the folks who own that environment. I'll re-run verl + SkyRL end-to-end and post the numbers here once that's unblocked.

sfc-gh-kganesan added a commit that referenced this pull request Aug 20, 2026
Review threads:
- T6/T12: move _cortex_dispatch.py from arctic_platform/rl/ to
  arctic_platform/integrations/ (only used by SkyRL/verl integrations).
- T10:    treat inline /operation responses (reset-prefix-cache, tail-logs,
  cancel-request, ...) as sync — poll only when the server hands back a
  request_id. Fixes the KeyError('request_id') that killed verl E2E.
- T13:    CortexConfig gains colocate: Literal[False] so callers can read
  backend.colocate uniformly across on-prem and Cortex.

Regression restore:
- _cortex_shared.py: put input_ids into `context` and pin loss_fn="grpo" +
  processing.config (eps_clip, prox_logp_method, dp_size, batch_num_tokens,
  global_batch_size). Server-side GRPO preflight reads input_ids from context;
  the rebase reshaped this to args=[input_ids] which triggers
  "grpo packed microbatches require tensor input_ids". SkyRL's verl_grpo alias
  now normalises to grpo so _resolve_fn doesn't try it as a dotted path.
- verl adapter: pass the full {batch, meta, processing} envelope to the shared
  reshape (was passing only payload["batch"], which dropped meta).
- Recipes (SkyRL + verl): pin attn_implementation=sdpa. Cortex training image
  ships without FlashAttention2; anything else fails with
  ImportError before the first forward. SkyRL also gets remote_urls +
  logprobs=null so validate_generator_cfg accepts run_engines_locally=false.

Rebased onto origin/main (picks up #80, #81).

Co-authored-by: Cursor <cursoragent@cursor.com>
@sfc-gh-kganesan
sfc-gh-kganesan force-pushed the sfc-gh-kganesan/skyrl-cortex-shim branch from 1db4f19 to 47bf349 Compare August 20, 2026 18:51
@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

Ready for another look, Mike — rebased onto origin/main (picked up #80, #81) at e5d0160.

Review threads (13 total)
All resolved. Cross-refs to specific commits on each thread; TL;DR:

  • 4 code-review asks addressed here (T6/T10/T12/T13 — file move under integrations/, polymorphic /operation per your suggested _submitted() helper, CortexConfig.colocate: Literal[False]).
  • 8 addressed in the earlier rebase (0c1479b); T2/T4 you had explicitly deferred; T9 kept as CortexConfig.from_env() classmethod rather than pydantic-settings — happy to switch.

Regression restore (found during E2E validation)
The prior rebase silently reshaped _cortex_shared.py and dropped attn_implementation=sdpa from the recipes. Restored the Aug-17 payload shape (input_ids in context, loss_fn pinned to grpo, processing.config with eps_clip/prox_logp_method/dp_size/batch_num_tokens) and re-added the recipe knobs. Details in the commit message on 47bf349.

E2E — both frameworks now train against Cortex QA6, Qwen3-0.6B GSM8K

SkyRL — 10 steps clean (skyrl_pr55_final_20260820T185119Z.log):

step  reward/avg_pass_at_4  grad_norm  entropy   clip_ratio  approx_kl
1     0.4688                0.523      0.540     0.0         0.0
5     0.5000                0.480      0.513     0.0         0.0
10    0.4688                0.437      0.566     0.0         0.0

approx_kl = clip_ratio = 0 is expected — single-epoch on-policy GRPO, Cortex server-side loss defaults π_old ≡ π_new via logprobs.detach() (see T8).

verl V0 — 5 steps clean (verl_pr55v2_20260820T195658Z.log):

step  actor/loss  actor/grad_norm  critic/score/mean  actor/entropy
1     0.0067      1.013            0.281              3.150
3     0.0084      1.243            0.246              3.085
5     0.0056      0.821            0.262              2.997

Uncovered a fwd_bwd/step response-shape mismatch (Cortex names loss as avg_loss and doesn't wrap step metrics) — fixed via _merge_train_response in e5d0160.

Tests (tests/client/test_client_ops.py) all pass, including the refreshed TestCortexSharedHelper that pins the Cortex wire shape.

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

Shim parity fix + convergence at 8× DP

Pushed 17f1d3e. Aligned the Cortex shim's processing block with Jae's cookbook (arctic_platform/cortex-client/recipes/rl_loop.py), so on-prem and Cortex compute the same loss for the same recipe at any DP scale.

Root cause

Our shim was sending three things that Jae's cookbook does not, and omitting two it does:

field before after why it matters
dp_size sent (=training_gpus) dropped server treated it as a loss divisor → 8× LR shrink at 8T
loss_agg_mode unset (server default) explicit token-mean server default is not guaranteed to match verl/on-prem
entropy_coeff unset explicit 0.0 (or recipe value) same reason
prox_logp_method sent (=recompute) dropped not in the cookbook
loss_mask fallback attention_mask if missing raise fallback silently trained on prompt tokens

For verl the adapter now lifts actor.clip_ratio / loss_agg_mode / entropy_coeff from the recipe into processing.config, so recipe choices propagate end-to-end.

verl x Cortex GSM8K, Qwen3-0.6B, 15–20 steps

Same recipe (arctic_platform/integrations/verl/examples/run_gsm8k_grpo_cortex.sh), same seed, only the topology changes:

config loss (per-step range) grad_norm (per-step range) val@10 val@final time/step
1T+1S 0.002 – 0.010 0.4 – 1.8 0.311 0.305 (step 15) ~17s
8T+8S 0.001 – 0.013 0.2 – 2.4 0.296 0.299 (step 20) ~8.4s

Loss and grad_norm magnitudes are the same at 1× and 8× DP → the dp_size removal fixes the multi-GPU LR shrink. Val accuracy is within noise across topologies. 2× wall-clock speedup at 8× GPU count (dominated by fixed rollout + weight-sync time).

Tests

tests/client/test_client_ops.py::TestCortexSharedHelper:

  • pins the Cortex wire shape (args/kwargs/context/processing)
  • asserts dp_size and prox_logp_method are not in the wire config
  • asserts caller-supplied processing.config (verl's recipe knobs) wins over the shim defaults
  • asserts missing response_mask/loss_mask fails loud

All 75 client + integrations tests pass locally.

SkyRL

SkyRL 1T+1S run completed 15 steps cleanly with the new shim (no crashes, weight sync fine). Reward stayed at 0 across steps, which points to a SkyRL-side GSM8K env/reward-parser mismatch for Qwen3-0.6B rather than a shim regression (verl scored 0.31 val@10 on the same model). Filed a separate note to look into the SkyRL reward path.

SkyRL 8T+8S hit a server-side NCCL error during broadcast_send_weights on the Cortex training sub-job, unrelated to shim contents — separate infra issue to raise.

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator Author

SkyRL x Cortex now converges E2E (data-schema fix + fail-loud recipe)

Pushed 326da87. SkyRL was completing 15 steps mechanically but with reward=0 / grad_norm=0 the whole time. Not a shim/Cortex issue — the driver was reading a verl-shaped parquet (reward_model field, no env_class) while SkyRL's GSM8kEnv requires reward_spec + env_class and silently scored every rollout 0.0.

Fix has two parts:

  1. Different default DATA_DIR — SkyRL recipe now defaults to ~/data/gsm8k-skyrl (was ~/data/gsm8k, colliding with verl's default).
  2. Pre-flight schema check in the launcher — reads the train parquet and refuses to start if reward_spec / env_class are missing, pointing the user at the right download_data.py.

SkyRL x Cortex GSM8K, Qwen3-0.6B, 1T+1S, 15 steps

step avg_pass@4 avg_raw_reward grad_norm eval/pass@1
1 0.500 0.219 0.463
5 0.562 0.273 0.516
7 0.625 0.344 0.564
10 0.531 0.227 0.489 0.308
13 0.562 0.305 0.497
15 0.406 0.148 0.393 0.296

Non-zero reward throughout, healthy grad_norm ~0.4-0.6, val@10 = 0.308 (comparable to verl's 0.311 on the same base model, as expected — Cortex-side compute is identical, both frameworks share the same shim).

Full E2E status on PR #55

framework topology E2E val@10
verl 1T+1S ✓ converging 0.311
verl 8T+8S ✓ converging (DP parity) 0.296
SkyRL 1T+1S ✓ converging 0.308

Both frameworks now train E2E with Cortex on GSM8K. The 8T+8S SkyRL NCCL error is a separate Cortex-server-side issue (broadcast_send_weights) — filing separately.

sfc-gh-kganesan added a commit that referenced this pull request Aug 21, 2026
Reduces PR #55 diff by 81 lines with no functional changes:

- _cortex_shared.py: collapse the 26-line module docstring into 8 lines;
  drop the body comments that just narrate the code they annotate.
- _cortex_dispatch.py: shorten shim class + method docstrings; drop the
  multi-line preamble above __getattr__.
- verl/adapter.py: trim _validate_cortex_compat and _stub_logprob_response
  docstrings from paragraphs to one line each; fold the redundant KL
  short-circuit branch (the inner ifs already gate identically).

All 75 client + integrations tests pass.

Co-authored-by: Cursor <cursoragent@cursor.com>
colocate: Literal[False] = Field(False, description="cortex: colocation not supported.")
base_url: str | None = Field(None, description="cortex: direct/mock GS URL; bypasses PAT auth.")
host: str | None = Field(None, description="cortex: Snowflake host for PAT auth.")
pat: str | None = Field(None, description="cortex: PAT value passed directly; overrides pat_env_var when set.")

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.

This should not be user facing to avoid PAT leakage. Set it directly in post-config via env var.

base_url: str | None = Field(None, description="cortex: direct/mock GS URL; bypasses PAT auth.")
host: str | None = Field(None, description="cortex: Snowflake host for PAT auth.")
pat: str | None = Field(None, description="cortex: PAT value passed directly; overrides pat_env_var when set.")
pat_env_var: str = Field("CORTEX_PAT", description="cortex: env var holding the PAT when `pat` is unset.")

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.

No need to define. Make hardcoded ARCTIC_CORTEX_PAT

for key, field in (
("base_url", "ARCTIC_CORTEX_BASE_URL"),
("host", "ARCTIC_CORTEX_HOST"),
("pat_env_var", "ARCTIC_CORTEX_PAT_ENV_VAR"),

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.

Why not ARCTIC_CORTEX_PAT to mirror others.

export ARCTIC_CORTEX_HOST=<account>.<region>.snowflakecomputing.com
export ARCTIC_CORTEX_DATABASE=<db>
export ARCTIC_CORTEX_SCHEMA=<schema>
export CORTEX_PAT=<pat>

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.

Suggested change
export CORTEX_PAT=<pat>
export ARCTIC_CORTEX_PAT=<pat>

@sfc-gh-truwase

Copy link
Copy Markdown
Collaborator

Tire-kick notes from a CPU driver on dsa-test (PR head 60ba6bf + SkyRL 7636101)

1T+1S and 8T+8S GSM8K GRPO both completed 15/15 (Training done!). 8T+8S did not hit the Cortex weight-sync NCCL error on this deployment (per-step sync_weights ~0.9–1.2 s). Eval openai_gsm8k/pass_at_1 ≈ 0.31 at step 10 on both.

Two client/recipe issues that blocked or noisily leaked during the run:

1. Stock SkyRL Cortex launcher dies on remote_urls (hard fail)

recipes/rl/skyrl/simple_gsm8k_cortex/run_qwen3_0.6b_gsm8k_grpo_cortex.sh only sets

generator.inference_engine.external_server_urls=[http://cortex-managed]

Pinned SkyRL validate_generator_cfg (with run_engines_locally=false) asserts

num_engines == len(ie_cfg.remote_urls)

so the unmodified script exits immediately:

AssertionError: num_engines should be equal to the number of remote_urls

Workaround that unblocked us:

generator.inference_engine.remote_urls=[http://cortex-managed]

For 8 sampling engines that list has to be length 8 (eight placeholder URLs). Worth setting remote_urls in the recipe (and repeating the placeholder NUM_ENGINES times) so ./run_qwen3_0.6b_gsm8k_grpo_cortex.sh works as documented.

2. aiohttp “Unclosed client session” on every Cortex HTTP call

~50 ERROR asyncio … Unclosed client session / Unclosed connector lines per 15-step run. Training still succeeds; 8T+8S also sat in Ray teardown for a while after Training done!.

Looks like CortexTransport._ensure_asession rebuilding a ClientSession when the running loop changes without closing the previous one (arctic_platform/client/transports/cortex.py), which SkyRL/Ray hits because each step can run on a different loop. aclose() exists but the stale-session branch explicitly drops the reference instead of closing it.

Please close (or detach + close) the old session on loop change, and make sure shutdown() awaits aclose() so the driver process actually exits.

1 similar comment
@sfc-gh-truwase

Copy link
Copy Markdown
Collaborator

Tire-kick notes from a CPU driver on dsa-test (PR head 60ba6bf + SkyRL 7636101)

1T+1S and 8T+8S GSM8K GRPO both completed 15/15 (Training done!). 8T+8S did not hit the Cortex weight-sync NCCL error on this deployment (per-step sync_weights ~0.9–1.2 s). Eval openai_gsm8k/pass_at_1 ≈ 0.31 at step 10 on both.

Two client/recipe issues that blocked or noisily leaked during the run:

1. Stock SkyRL Cortex launcher dies on remote_urls (hard fail)

recipes/rl/skyrl/simple_gsm8k_cortex/run_qwen3_0.6b_gsm8k_grpo_cortex.sh only sets

generator.inference_engine.external_server_urls=[http://cortex-managed]

Pinned SkyRL validate_generator_cfg (with run_engines_locally=false) asserts

num_engines == len(ie_cfg.remote_urls)

so the unmodified script exits immediately:

AssertionError: num_engines should be equal to the number of remote_urls

Workaround that unblocked us:

generator.inference_engine.remote_urls=[http://cortex-managed]

For 8 sampling engines that list has to be length 8 (eight placeholder URLs). Worth setting remote_urls in the recipe (and repeating the placeholder NUM_ENGINES times) so ./run_qwen3_0.6b_gsm8k_grpo_cortex.sh works as documented.

2. aiohttp “Unclosed client session” on every Cortex HTTP call

~50 ERROR asyncio … Unclosed client session / Unclosed connector lines per 15-step run. Training still succeeds; 8T+8S also sat in Ray teardown for a while after Training done!.

Looks like CortexTransport._ensure_asession rebuilding a ClientSession when the running loop changes without closing the previous one (arctic_platform/client/transports/cortex.py), which SkyRL/Ray hits because each step can run on a different loop. aclose() exists but the stale-session branch explicitly drops the reference instead of closing it.

Please close (or detach + close) the old session on loop change, and make sure shutdown() awaits aclose() so the driver process actually exits.

…hape

fwd_bwd_request notes that the call signature is unified across backends but
`batch`'s content is not: on-prem takes verl-GRPO {batch, meta} while Cortex
takes an RPC-style {args, kwargs, context}. Every would-be Cortex caller was
therefore going to carry its own reshape. Doing it in the Cortex transport
instead means one implementation serves verl, SkyRL and the Tinker frontend,
and callers stop branching on backend. Frames that are already Cortex-shaped
(the standalone recipes) pass through untouched.

Cortex also reports as top-level fields what on-prem reports inside `metrics`,
so `avg_loss` and `last_lr` are mirrored into `metrics` additively -- enough for
verl's update_actor to read Cortex responses without a per-adapter flattener.

CortexConfig.from_env keeps the ARCTIC_CORTEX_* contract in one place so
adapters whose own YAML has no backend field can be pointed at Cortex from the
shell, which is the single line the verl adapter needs.

Co-authored-by: Cursor <cursoragent@cursor.com>
@sfc-gh-kganesan
sfc-gh-kganesan force-pushed the sfc-gh-kganesan/skyrl-cortex-shim branch from 60ba6bf to 37dce1e Compare August 26, 2026 04:06
@sfc-gh-kganesan sfc-gh-kganesan changed the title Cortex dispatch shim + SkyRL/verl+Cortex GSM8K recipes feat(cortex): lower verl-GRPO forward-backward onto the Cortex wire shape Aug 26, 2026
@sfc-gh-kganesan
sfc-gh-kganesan changed the base branch from main to mwyatt/unified-client-recipes August 26, 2026 04:06
Rebase of the Cortex shim onto #93, dropping only what #93 already provides
rather than dropping capability. Both frameworks keep the end-to-end path that
was validated pre-rebase (verl val@10 0.311 / SkyRL 0.308 on GSM8K).

The three places Cortex diverges from on-prem now live in CortexTransport, so
no integration carries its own copy and #93's recipes are unaffected:

  - forward-backward is lowered from verl's {batch, meta} to Cortex's
    {args, kwargs, context}; frames already in Cortex's shape pass through.
  - forward is zero-filled, because Cortex has no such sub-job. Sound only for
    single-epoch on-policy GRPO without KL, so the verl adapter refuses the
    knobs that would read those values before the client is built.
  - avg_loss / last_lr, which Cortex returns at the top level, are mirrored
    into `metrics`. step has no `metrics` key at all, which would otherwise
    KeyError in verl's _send_update_actor.

Dropped as genuinely redundant with #93: its cortex.py changes (a strict
superset of ours), the shim's fake-async wrapper (AsyncArcticRLClient), its
response flattener and its payload reshape. That shrinks _cortex_dispatch from
147 lines to config translation plus the legacy accessors SkyRL reads.

Also folds in the remote_urls fix from tire-kicking: pinned SkyRL asserts
num_engines == len(remote_urls), so the recipe never ran as documented.

Backend imports in create_arctic_rl_client are now lazy, which is what a
CPU-only Cortex driver needs and incidentally fixes tests/rl/test_cpu_import.py.

Co-authored-by: Cursor <cursoragent@cursor.com>
@sfc-gh-kganesan sfc-gh-kganesan changed the title feat(cortex): lower verl-GRPO forward-backward onto the Cortex wire shape feat(cortex): SkyRL + verl on Cortex, rebased onto the unified client Aug 26, 2026
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.

3 participants