[draft] proposal: generic trainer.backend registry (alternative to PR #1) - #3
Merged
Merged
Conversation
…n-specific routing) Addresses PR #1 review feedback while preserving recipe-portability: - Core stays integration-agnostic: no arctic-specific code in skyrl/train/ - Any existing recipe can swap backends via one flag (trainer.backend=arctic_rl) - Pattern generalizes to future training backends (megatron, nemo, etc.) Changes: - skyrl/train/config/config.py: replace `arctic_rl: Optional[ArcticRLTrainerConfig]` field with generic `backend: str = "fsdp"`. Drop ArcticRLTrainerConfig class (moved to integration). - skyrl/train/entrypoints/main_base.py: replace arctic-specific routing block with 3-line generic dispatch that imports `integrations.<name>.entrypoint`. - skyrl/train/utils/utils.py: drop `_propagate_arctic_env_vars` (moved into arctic's own entrypoint). - pyproject.toml: revert `[tool.setuptools.packages.find]` to upstream; arctic_rl runs as a namespace package under integrations/, like harbor. - integrations/arctic-rl/arctic_rl/config.py: define ArcticRLTrainerConfig here, plus ArcticTrainerConfig (extends TrainerConfig) and ArcticSkyRLConfig = make_config(trainer_cls=ArcticTrainerConfig). Mirrors HarborSkyRLConfig from main_harbor.py:36-48. - integrations/arctic-rl/arctic_rl/entrypoint.py: main(cfg=None) supports both invocation modes: direct via uv (cfg=None → parse with ArcticSkyRLConfig) and via core dispatch (cfg passed in). Inlines ARCTIC_* env-var forwarding. - integrations/arctic-rl/examples/run_gsm8k_grpo_4gpu.sh: shows both invocation styles. Adds `trainer.backend=arctic_rl` flag. Why a generic backend field instead of the harbor pattern: Harbor is an RL environment integration — different env = different recipe by nature (different data, different reward fn). Per-integration-entrypoint fits. Arctic RL is a training backend (DeepSpeed engine + ArcticInference vLLM). Backends should swap orthogonally under any existing recipe (gsm8k, math, etc.) without forking the recipe. A `trainer.backend: str` generic extension hook is the right shape — same lever as `make_config(trainer_cls=...)` (the blessed extension path, per review feedback) but at the entrypoint level. No specific integration is hardcoded in core. Both invocation styles work after this: - `python -m skyrl.train.entrypoints.main_base trainer.backend=arctic_rl ...` (any-recipe + flag mode) - `uv run --extra arctic-rl -m integrations.arctic_rl.entrypoint ...` (direct, harbor-style)
…/port defaults
Fixes from running E2E convergence test:
1. main_base dispatch: peek ``trainer.backend=`` from sys.argv BEFORE calling
``SkyRLTrainConfig.from_cli_overrides``. Otherwise the parse fails on
integration-specific fields (e.g. ``trainer.arctic_rl``) that core does
not know about. After dispatch, the integration entrypoint parses with
its own ``make_config(trainer_cls=...)``-extended config.
2. Dispatch import path: ``{backend}.entrypoint`` instead of
``integrations.{backend}.entrypoint``. The integration is a top-level
importable package (``arctic_rl``), made available either via
``uv run --extra arctic-rl`` or by adding the integration dir to
PYTHONPATH. Folder names like ``integrations/arctic-rl/`` (with hyphen)
are not valid Python module identifiers.
3. ``ArcticRLTrainerConfig.host/port`` default to "localhost" / 7000
instead of None. AT-dss ``ArcticRLClientConfig`` (Pydantic) requires
non-None values even when comm_protocol=ray ignores them.
Validated: 4 steps GRPO on GSM8K Qwen3-0.6B converges with reward climbing
0.125 → 0.0 → 0.188 → 0.375 (avg_raw_reward) at ~20s/iter steady-state.
Same noisy-but-improving shape as the original arctic-rl-public-sf run.
- arctic_rl/entrypoint.py: drop cfg=None parameter (dispatch never passes cfg, so dual-mode handling was dead). main() always parses with ArcticSkyRLConfig from sys.argv. Net 14 lines removed. - skyrl/train/config/config.py: shorten 'backend' field docstring (4 lines → 3 lines). Same information, less verbose. - run_gsm8k_grpo_4gpu.sh: collapse 16-line dual-mode comment block into 3-line note. Same info; recipe is the running example, not a tutorial. Net: -38 / +12 lines. Behavior unchanged — both invocation modes still work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Alternative refactor for the Arctic RL integration in response to PR #1 review. Honors the reviewer's "core stays integration-agnostic" principle while preserving the property that any existing SkyRL recipe can flip to Arctic via a single flag — without forking the recipe or changing its entrypoint.
This PR is a draft for discussion, parallel to #1. No reviewers tagged.
Reasoning — why this differs from the harbor pattern
RayPPOTrainer→ArcticPPOTrainerGenerator→ArcticGeneratorThe harbor pattern fits envs naturally because env = experiment. For training backends, users expect orthogonal swapping — analogous to
trainer.algorithm.advantage_estimator=grpo|reinforce|.... Strict harbor pattern would require forking every standard SkyRL recipe (run_gsm8k.sh,run_math.sh, etc.) intointegrations/arctic-rl/examples/to enable Arctic RL.This PR proposes a generic backend registry: one
trainer.backend: strfield on coreTrainerConfigplus a 3-line lazy dispatch inmain_base.main()that importsf"{backend}.entrypoint". No integration name is hardcoded in core.Design
Code layout
Reviewer's 5 asks — all addressed
[tool.setuptools.packages.find]to upstreaminclude = ["skyrl*"]ArcticRLTrainerConfigout of coreconfig.py_propagate_arctic_env_varsfrom core utilsmain_base.pyarctic_rlstrings in core code)make_config(trainer_cls=...)extensionArcticSkyRLConfig = make_config(trainer_cls=ArcticTrainerConfig)uv run --extra arctic-rl -m ...entrypointmode supportedtrainer.backend=arctic_rlflag mode also worksOne delta from harbor
Core gains exactly:
backend: str = "fsdp"onTrainerConfigmain_base.main()(peek argv →import_module(f"{backend}.entrypoint").main)Both integration-agnostic. Same architectural lever as
make_config(trainer_cls=...)(the reviewer-blessed extension path) but at the entrypoint level.Convergence test ✅
Validated end-to-end with the original
arctic-rl-public-sf-validated stack (1×2 H200 colocated, GSM8K GRPO, Qwen3-0.6B), via the newtrainer.backend=arctic_rldispatch. 9 GRPO steps, reward climbing:Same noisy-but-improving GRPO shape as PR #1's validation table. Steady-state ~12s/iter.
trainer/global_step: 9confirmed.Diff summary
7 files, +107/-86 lines. Core net change: -60 lines (mostly
ArcticRLTrainerConfigmoving out). Integration: +60 (the moved class + harbor-style extension).Test plan
python -m skyrl.train.entrypoints.main_base trainer.backend=arctic_rl trainer.arctic_rl={} ...reaches arctic flowarctic_rlorarctic_training(grep -rn arctic skyrl/returns only docstring examples)uv run --extra arctic-rl -m arctic_rl.entrypoint ...(harbor-style direct mode) — not yet tested but trivially equivalentbackenddefaults to"fsdp"