Skip to content

feat(integrations/verl): swap adapter to arctic_platform.client (unified client) - #51

Closed
sfc-gh-kganesan wants to merge 1 commit into
sfc-gh-kganesan/skyrl-verl-cortex-compatfrom
sfc-gh-kganesan/verl-adapter-unified-client
Closed

feat(integrations/verl): swap adapter to arctic_platform.client (unified client)#51
sfc-gh-kganesan wants to merge 1 commit into
sfc-gh-kganesan/skyrl-verl-cortex-compatfrom
sfc-gh-kganesan/verl-adapter-unified-client

Conversation

@sfc-gh-kganesan

Copy link
Copy Markdown
Collaborator

Depends on / stacked on top of #50 (base branch is sfc-gh-kganesan/skyrl-verl-cortex-compat, not main) so the diff is only the adapter delta. Do not merge before #47 and #50.

Tracked by #49.

Why

arctic_platform/integrations/verl/adapter.py still imports from the pre-unification arctic_platform.rl package. Every call site through that module is async def even though the underlying HTTP/Ray calls are already synchronous, backend selection is hardcoded to backend="local", and there's no way to point the adapter at Cortex without forking it.

This PR does the minimal swap onto the unified arctic_platform.client shipped by #45#47 + #50. Combined they give verl a serverless-capable backend switchable by a single yaml knob.

What

arctic_platform/integrations/verl/adapter.py

Import swaparctic_platform.rlarctic_platform.client:

  • ArcticRLClientConfig, create_arctic_rl_client from the unified package.
  • ArcticRLRayServerState moved to a TYPE_CHECKING import from arctic_platform.client.transports.onprem_ray, so a Cortex-only install (which never pulls ray in) still imports the adapter cleanly. Reconnect-path parameter typing preserved.

One backend knob, no adapter-side branching:

  • _initialize_client reads backend from yaml (remote_backend.backend, default "onprem") and passes it straight through. Dispatch to RayTransport / HttpTransport / CortexTransport happens inside ArcticRLClient.__init__ via make_transport(config) — the adapter has zero knowledge of Cortex-specifics.
  • Cortex sub-block (remote_backend.cortex.*) is whitelisted through into ArcticRLClientConfig: cortex_host, cortex_database, cortex_schema, cortex_endpoint, cortex_pat_env_var, cortex_base_url, max_seq_len. Any missing key falls back to CortexTransport defaults or CORTEX_* env vars.
  • Dropped the hardcoded backend=\"local\" and log_prob_engine=\"deepspeed\". If a stale yaml still sets backend: local, the client's legacy alias (in feat(rl): Cortex-serverless dispatch via arctic_platform.rl (zero integration change) #50) warns and coerces to onprem.
  • CUDA_VISIBLE_DEVICES widening (needed because Arctic's driver actor is scheduled with num_gpus=0) is now gated on backend == \"onprem\" — driver-side GPU placement is meaningless on Cortex.

Sync-under-async:

  • All the underlying calls in arctic_platform.client are synchronous. Every await self._client.* becomes a plain call; every await self._send_* becomes a plain call.
  • RemoteBackend's interface methods (compute_log_prob, update_actor, generate, save_checkpoint, update_weights, wake_up_inference, sleep_inference, reset_prefix_cache, destroy) stay async def — verl's RemoteBackendTrainer awaits them and swapping the interface would be a breaking change I don't want in this PR.
  • Private _send_compute_ref_log_prob / _send_compute_log_prob / _send_update_actor become plain def; their bodies were already effectively synchronous once you strip the awaits.

What this PR does NOT touch

  • Payload construction (_prepare_padded_arctic_batch_dict, _no_padding_2_padding_prompt_response, _create_ds_config, _create_ds_worker_config, _send_update_actor's left-pad handling) — byte-identical to main, so on-prem behavior is unchanged.
  • Response shape handling (response[\"batch\"][\"log_probs\"], .pop(\"logprobs\"), response[\"metrics\"] aggregation) — same reads; the compat layer in feat(rl): Cortex-serverless dispatch via arctic_platform.rl (zero integration change) #50 makes the Cortex path emit the same shape.
  • The RemoteBackend interface. No signature changes visible to verl.

Tests

Existing tests/client/* pass (69 pass; the compat test suite from #50 pins every call site the adapter makes against the client). No new adapter-level tests in this PR — the compat suite already covers the client-facing contract; an integration test lives on the E2E smoke below.

Test plan

  • python -m pytest tests/client/ — 69 pass on this branch.
  • Adapter syntax + interface preservation (AST check: 9 public methods stay async, 3 private helpers become sync).
  • On-prem regression smoke — one Qwen3-0.6B GSM8K run against recipes/rl/skyrl/simple_gsm8k/run_qwen3_0.6b_gsm8k_grpo_arl.sh using remote_backend.backend: onprem. Loss curves vs main must match to within numerical noise.
  • Cortex first-run — same recipe with remote_backend.backend: cortex + a populated remote_backend.cortex block, once the Cortex-side blockers in Cortex-serverless enablement for verl + SkyRL via the unified ArcticRLClient #49 §2 land (fwd-no-grad endpoint, log-probs endpoint, metrics block on fwd_bwd/step responses).

Follow-ups (see #49)

  • Neutrino GS: POST /{job_id}/forward-no-grad, POST /{job_id}/log-probs.
  • Cortex fwd_bwd / step responses grow a metrics: {grad_norm, ppo_kl, pg_clipfrac, pg_loss, kl_loss, kl_coef} block.
  • Symmetric SkyRL adapter swap in Snowflake-AI-Research/arctic-skyrl (same import swap, same backend knob).

Made with Cursor

…nditional)

Move the verl RemoteBackend adapter off `arctic_platform.rl` and onto
the unified `arctic_platform.client`. Backend selection collapses to one
yaml knob (`remote_backend.backend`) resolved inside the client by
`make_transport(config)`; the adapter itself no longer branches on
"cortex" vs "onprem".

- Imports swap `arctic_platform.rl` -> `arctic_platform.client`.
- `ArcticRLRayServerState` moved to a `TYPE_CHECKING` import from
  `arctic_platform.client.transports.onprem_ray` so a Cortex-only
  install (no ray) still imports the adapter cleanly.
- `_initialize_client` reads `backend` from yaml (default `onprem`)
  and forwards a whitelisted set of cortex keys
  (`cortex_host`/`_database`/`_schema`/`_endpoint`/`_pat_env_var`/
  `_base_url`, `max_seq_len`) when a `remote_backend.cortex` sub-block
  is present.
- Drop the hardcoded `backend="local"` and `log_prob_engine="deepspeed"`.
  Legacy aliases live on `ArcticRLClientConfig` now (compat PR #50); if
  a stale yaml still sets `backend: local` the client warns and coerces.
- `CUDA_VISIBLE_DEVICES` widening only runs on the on-prem path — the
  driver actor on Cortex has no local GPU placement to widen.
- Drop `await` on every `self._client.*` call (unified client is sync).
  The `RemoteBackend` interface stays `async def` (verl requirement);
  private `_send_*` helpers become plain `def` — their bodies were
  already synchronous once you strip the awaits.

No behavioral change on the on-prem path (the four-fields commit in
#50 threads `ds_worker_config` / `arctic_inference_config` /
`log_prob_ds_config` / `full_determinism` through the unified init).
Cortex path is enabled by setting `remote_backend.backend: cortex` in
the arctic.yaml + populating the `remote_backend.cortex` sub-block.

Depends on #50 (compat layer + four fields). PR against
`sfc-gh-kganesan/skyrl-verl-cortex-compat`.

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

Copy link
Copy Markdown
Collaborator Author

Closing this PR — after re-reading the merged upstream integrations (NovaSky-AI/SkyRL#1837, verl-project/verl#6422) I confirmed neither integration should have to change to gain Cortex support.

Both the merged SkyRL integrations/arctic_rl/ and the verl RemoteBackend implementation route through arctic_platform.rl.create_arctic_rl_client(config, server_state) — a single entry point on the Arctic-Platform side. The right move is to make that entry point dispatch to the unified client's Cortex transport when config.backend == "cortex", not to swap either adapter's imports.

Rewiring will land as new commits on #50; this branch will be deleted.

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.

1 participant