Restructure remote_backend per @zw0610 review (stacked on #3) by sfc-gh-kganesan · Pull Request #4 · Snowflake-AI-Research/verl

sfc-gh-kganesan · 2026-05-23T16:22:14Z

Why

Addresses all 7 of @zw0610's structural comments on
verl-project/verl#6422
in one place so Tunji + Wang can review the direction before it lands
upstream. Stacks on top of #3 (the wang-rename layer); see the
restructure-only diff here:

https://github.com/sfc-gh-kganesan/verl/compare/karthik/wang-rename...karthik/wang-restructure

(this PR's full diff is a superset because GitHub can't target a fork
branch as base on SAR).

What changes (mapped 1:1 to Wang's comments)

#	Wang's ask	Change
1	Split `remote_backend` config out of `ppo_trainer.yaml`	New `verl/trainer/config/remote_backend/arctic.yaml`; inline block in `ppo_trainer.yaml` replaced by a Hydra optional defaults entry (`- optional remote_backend@remote_backend: null`). Users opt in with `remote_backend=arctic` (standard Hydra option-group syntax).
2	Move `arctic_rl_client.py` to `verl/workers/remote_client/`	`git mv verl/trainer/ppo/arctic_rl_client.py verl/workers/remote_client/arctic_rl_client.py` + new `__init__.py`.
3	Move `arctic_rollout/` to `verl/workers/rollout/remote_rollout/arctic_rollout/`	`git mv verl/workers/rollout/arctic_rollout verl/workers/rollout/remote_rollout/arctic_rollout` + new `__init__.py`.
4	Add TODO(@zw0610) in `replica.py:360` for lazy-init each `RolloutReplicaRegistry` option	Added above the `RolloutReplicaRegistry.register("vllm" …)` block, citing dependency-conflict risk and noting the `_load_*` callables already defer imports.
5	Trim ABC: remove non-lifecycle abstract methods	`RemoteBackend` ABC dropped `compute_log_prob`, `update_actor`, `generate`. Remaining abstracts: `from_config`, `reconnect_handle`, `destroy`, `update_weights`, `save_checkpoint`, `requires_single_forwarder`. Compute/update/generate stay on the Arctic adapter as plain (non-abstract) methods, called from the Arctic-specific worker.
6	Leave `MODULES` empty, register explicitly	Dropped the lazy `RemoteBackendRegistry.MODULES` table. Adapter modules are now imported explicitly: by `main_ppo.py` on the driver and by the per-backend worker module on Ray child procs (one line at the top of `workers/arctic_rl/worker.py`).
7	Per-backend worker, not a parallel generic class	New `verl/remote_backend/workers/arctic_rl/` package. `RemoteBackendActorRolloutRefWorker` renamed to `ArcticRLActorRolloutRefWorker`. `main_ppo.py` selects the per-backend worker class with an explicit `if backend_name == "arctic"` branch and a clear error message on unknown names. NOT yet done (carve-out): making this worker actually inherit from `verl.workers.engine_workers.ActorRolloutRefWorker` (Wang's full prescription with `init_model` override) requires first decoupling `ActorRolloutRefWorker.__init__` from megatron-specific config fields (e.g. `config.actor.megatron.router_replay`) that the Arctic config tree doesn't carry. Left as a follow-up.

Commits

9e9f6afb — move adapter + rollout into per-backend paths + add TODO (Wang Generic RemoteBackend abstraction + Arctic adapter (parallel to PR #1) #2/[refactor] Rename arctic_rl config namespace to generic remote_backend.arctic #3/Restructure remote_backend per @zw0610 review (stacked on #3) #4)
9b82e95a — ABC trim + registry trim + external YAML + per-backend worker rename + main_ppo.py selector (Wang Arctic RL: VeRL integration #1/[arctic-rl] align adapter with upcoming zorro_train config grouping #5/[arctic_rl] backport rl-correctness payload-encoding deltas onto arctic_rl_share_v0.7.1 #6/verl abstract remote backend #7)
e4aaee0c — eager-import adapter in arctic_rl worker for Ray child procs (caught during smoke after the MODULES trim)

Verified

6-step GSM8K GRPO smoke (Qwen3-0.6B, single-GPU, zorro=True, gbs=16, rollout n=5):

step	actor/loss	perf/mfu/actor
1	0.0443	198.2
2	0.0295	223.3
3	0.0309	213.0
4	0.0426	216.3
5	0.0322	197.7
6	-0.0012	215.2

All within the established 186–218 baseline; convergence and MFU
unchanged.

Open questions for the reviewers

@zw0610: is the partial verl abstract remote backend #7 (rename + per-backend module path, parent
class unchanged) acceptable as an intermediate step, with the full
ActorRolloutRefWorker inheritance as a follow-up that also touches
the megatron config coupling? If you want both in one PR, I can
extend.
@sfc-gh-truwase: this is the design Wang asked for; matches the
RFC ask on [trainer] feat: generic remote backend abstraction for RL Training verl-project/verl#6422 modulo the follow-up above.

Made with Cursor

Now that ArcticTraining-dss commit af1ab8d (`Async+bf16 opt`) makes `fwd_no_grad`, `fwd_bwd`, and `step` async on `ArcticRLRayClient`, `asyncio.to_thread` is no longer needed for these three. Switching to bare `await self._client.<op>(...)` per Tunji's retry-await comments on PR Snowflake-AI-Research#3 (lines 458, 459) — symmetry with the already-await'd `save_checkpoint` and `sync_weights`. Also drops the now-unused `import asyncio` from `arctic_rl_client.py`. `RemoteBackendTrainer.destroy` still uses `asyncio.run` to bridge the sync shutdown path to the async `backend.destroy`. Smoke: 6-step GSM8K GRPO on H200, MFU 186-218; matches the `asyncio.to_thread` baseline (199-224), no perf regression from dropping the thread hop. Co-authored-by: Cursor <cursoragent@cursor.com>

@zw0610

…e-AI-Research#2/Snowflake-AI-Research#3/Snowflake-AI-Research#4) Addresses @zw0610's structural review on verl-project#6422: - Snowflake-AI-Research#2: verl/trainer/ppo/arctic_rl_client.py -> verl/workers/remote_client/arctic_rl_client.py - Snowflake-AI-Research#3: verl/workers/rollout/arctic_rollout/ -> verl/workers/rollout/remote_rollout/arctic_rollout/ - Snowflake-AI-Research#4: TODO(@zw0610) on RolloutReplicaRegistry registrations in verl/workers/rollout/replica.py:360 about lazy-init each option to avoid pulling in vLLM + SGLang + TRT-LLM transitive deps eagerly. Pure code-motion + a TODO; no behavior change. Imports inside the moved files and in workers/rollout/replica.py updated. Co-authored-by: Cursor <cursoragent@cursor.com>

@zw0610

…Snowflake-AI-Research#5/Snowflake-AI-Research#6/Snowflake-AI-Research#7) Addresses the remaining @zw0610 structural comments on verl-project#6422: - Snowflake-AI-Research#1 (split remote_backend out of ppo_trainer.yaml): * New verl/trainer/config/remote_backend/arctic.yaml with the arctic block content. * In verl/trainer/config/ppo_trainer.yaml, replace the inline remote_backend: {} block with a Hydra optional defaults entry (`- optional remote_backend@remote_backend: null`). Users opt in with `remote_backend=arctic` (standard Hydra option group syntax). * Example script (run_gsm8k_grpo_arl_zorro_yes.sh) updated accordingly. - Snowflake-AI-Research#5 (trim RemoteBackend ABC to lifecycle only): * Drop `compute_log_prob`, `update_actor`, `generate` from verl/remote_backend/base.py. ABC now enforces only: `from_config`, `reconnect_handle`, `destroy`, `update_weights`, `save_checkpoint`, `requires_single_forwarder`. * Concrete compute/update/generate methods stay on the Arctic adapter as plain methods (called from the Arctic-specific worker, not via the ABC contract). - Snowflake-AI-Research#6 (empty MODULES, explicit registration): * Drop the lazy `RemoteBackendRegistry.MODULES` table from base.py so no transitive deps (vLLM, arctic-training, tinker, ...) get pulled in eagerly. * main_ppo.py now imports the adapter module explicitly (`from verl.workers.remote_client import arctic_rl_client`) when `trainer.remote_backend=arctic`; the import side-effect registers the class. - Snowflake-AI-Research#7 (per-backend worker, not a generic parallel class): * New verl/remote_backend/workers/arctic_rl/ package. * Rename `RemoteBackendActorRolloutRefWorker` to `ArcticRLActorRolloutRefWorker` (the worker.py move happened in the previous commit). * main_ppo.py selects the per-backend worker class explicitly (currently arctic; extends backend-by-backend, with a clear error message on unknown backend names). * NOTE: this PR does NOT yet make the worker inherit from `verl.workers.engine_workers.ActorRolloutRefWorker` (Wang's full Snowflake-AI-Research#7 prescription). Doing so requires first decoupling `ActorRolloutRefWorker.__init__` from megatron-specific config fields (e.g. `config.actor.megatron.router_replay`) that the Arctic config tree doesn't carry; left as a follow-up RFC item. Verified end-to-end via 6-step GSM8K GRPO smoke (Qwen3-0.6B, single-GPU, zorro=True); convergence and MFU unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>

Ray child procs don't inherit the driver's import side-effects, so the `@RemoteBackendRegistry.register("arctic")` decorator never ran in the WorkerDict actor and `RemoteBackendRegistry.get("arctic")` raised `KeyError`. Fix by eager-importing the adapter at the top of the per-backend worker module — every process that loads this worker now also loads (and registers) its adapter. The driver still imports the adapter explicitly in `verl.trainer.main_ppo`; this is the matching import in the worker process. Caught by the 6-step GSM8K GRPO smoke after the registry/MODULES trim. Co-authored-by: Cursor <cursoragent@cursor.com>

sfc-gh-truwase · 2026-05-23T17:05:09Z

@@ -159,7 +158,7 @@ def requires_single_forwarder(self) -> bool:
        return True

    # ------------------------------------------------------------------ #
-    # Core RL ops — called by `RemoteBackendActorRolloutRefWorker`
+    # Core RL ops — called by `ArcticRLActorRolloutRefWorker`


Given new file location, rename to remote_client/arctic_rl.py

Renamed verl/workers/remote_client/arctic_rl_client.py → arctic_rl.py in 9fc25fb, and updated all 8 callsites (main_ppo.py, arctic_rollout.py, remote_backend/base.py, remote_backend/workers/arctic_rl/worker.py + __init__.py, remote_backend/__init__.py, plus the yaml docstring).

sfc-gh-truwase · 2026-05-23T17:10:24Z

+# defaults (`+remote_backend@remote_backend=arctic`). Companion adapter:
+# `verl.workers.remote_client.arctic_rl_client.ArcticRLClientWrapper`.
+# Selected at runtime by `trainer.remote_backend=arctic`.
+arctic:


Is the top-level arctic: namespace still needed given this is arctic yaml?

You're right — flattened in 9fc25fb. Dropped the top-level arctic: key (file name already names the backend) and removed the _BACKEND_CONFIG_KEY = "arctic" indirection in the adapter so it now reads config.remote_backend directly. Also updated the example Hydra overrides (remote_backend.arctic.X=Y → remote_backend.X=Y) in run_gsm8k_grpo_arl_zorro_yes.sh.

zw0610

Generally this pull request looks great. Please let me know when it's ready.

@sfc-gh-truwase

…ctic.yaml (Tunji PR#4) Addresses 2 outstanding @sfc-gh-truwase comments on PR Snowflake-AI-Research#4: 1. `verl/workers/remote_client/arctic_rl_client.py` -> `arctic_rl.py` The new directory `remote_client/` already names the role, so the `_rl_client` suffix is redundant. Updated all 8 callsites (main_ppo.py, arctic_rollout.py, base.py, worker.py, __init__.py files, and adapter self-references in docstrings). 2. `trainer/config/remote_backend/arctic.yaml`: dropped the top-level `arctic:` namespace — the file name (`arctic.yaml`) is already the backend name, so nesting was redundant. Adapter now reads `config.remote_backend` directly (removed `_BACKEND_CONFIG_KEY` indirection). Updated the example shell script Hydra overrides (`remote_backend.arctic.X=Y` -> `remote_backend.X=Y`). Verified end-to-end: `verl.remote_backend`, the renamed adapter, the per-backend worker, and the rollout chain all import cleanly under the arctic_rl venv. Co-authored-by: Cursor <cursoragent@cursor.com>

sfc-gh-kganesan · 2026-05-29T21:41:01Z

Status update — upstream RFC for the broader abstraction is now at verl-project#6537. Both of your inline comments on this PR addressed in 9fc25fb (rename + arctic.yaml flatten + adapter unwrap).

sfc-gh-kganesan · 2026-05-29T22:17:17Z

@zw0610 ready for re-review — both of @sfc-gh-truwase's May 23 inline comments are addressed in 9fc25fb99f:

arctic_rl_client.py → arctic_rl.py (rename + 8 callsites updated)
Flattened arctic.yaml (dropped top-level arctic: key) + removed the _BACKEND_CONFIG_KEY indirection in the adapter so it reads config.remote_backend directly; example Hydra overrides flattened to match.

Import chain verified end-to-end under arctic_rl venv. PR is MERGEABLE.

Drive-by correctness fix surfaced by the 4-step GSM8K smoke against the CI-fix head: `destroy()` is async (line 461) and the upstream ArcticTraining-dss `shutdown()` is async, but the call was missing `await`, producing: RuntimeWarning: coroutine 'ArcticRLRayClient.shutdown' was never awaited Regression introduced by the wang-restructure merge (the await was present on the pre-#4 branches; rename + move dropped it). Matches the `save_checkpoint` / `update_weights` async pattern in the same file. Co-authored-by: Cursor <cursoragent@cursor.com>

sfc-gh-kganesan and others added 4 commits May 23, 2026 15:52

sfc-gh-kganesan mentioned this pull request May 23, 2026

[trainer] feat: generic remote backend abstraction for RL Training verl-project/verl#6422

Merged

8 tasks

sfc-gh-truwase reviewed May 23, 2026

View reviewed changes

zw0610 reviewed May 26, 2026

View reviewed changes

sfc-gh-kganesan marked this pull request as ready for review May 29, 2026 22:30

sfc-gh-kganesan merged commit bf8ea8f into Snowflake-AI-Research:arctic_rl_share_v0.7.1 May 29, 2026
1 check passed

Navigation Menu

Uh oh!

Restructure remote_backend per @zw0610 review (stacked on #3) - #4

Restructure remote_backend per @zw0610 review (stacked on #3)#4
sfc-gh-kganesan merged 5 commits into
Snowflake-AI-Research:arctic_rl_share_v0.7.1from
sfc-gh-kganesan:karthik/wang-restructure

sfc-gh-kganesan commented May 23, 2026

Uh oh!

sfc-gh-truwase May 23, 2026

Uh oh!

sfc-gh-kganesan May 29, 2026

Uh oh!

sfc-gh-truwase May 23, 2026

Uh oh!

sfc-gh-kganesan May 29, 2026

Uh oh!

zw0610 left a comment •

edited

Loading

Uh oh!

sfc-gh-kganesan commented May 29, 2026

Uh oh!

sfc-gh-kganesan commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

sfc-gh-kganesan commented May 23, 2026

Why

What changes (mapped 1:1 to Wang's comments)

Commits

Verified

Open questions for the reviewers

Uh oh!

sfc-gh-truwase May 23, 2026

Choose a reason for hiding this comment

Uh oh!

sfc-gh-kganesan May 29, 2026

Choose a reason for hiding this comment

Uh oh!

sfc-gh-truwase May 23, 2026

Choose a reason for hiding this comment

Uh oh!

sfc-gh-kganesan May 29, 2026

Choose a reason for hiding this comment

Uh oh!

zw0610 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-kganesan commented May 29, 2026

Uh oh!

sfc-gh-kganesan commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zw0610 left a comment •

edited

Loading