feat(colocate): support AWEX colocated actor-rollout training - #1500
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an AWEX colocate adapter and reader to enable zero-copy weight transfer via CUDA IPC and MetaServer between MegatronEngine and SGLang. It adds manual GPU-to-CPU memory offloading/onloading, updates Slurm scheduling to support colocation, and optimizes memory usage during HF weight saving. The review feedback highlights two key issues: a potential deadlock in physical GPU ID detection when CUDA_VISIBLE_DEVICES contains multiple comma-separated GPUs, and a missing initialization guard in get_weight_metadata that could lead to malformed MetaServer keys.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| cuda_visible = os.environ.get("CUDA_VISIBLE_DEVICES", "") | ||
| if cuda_visible and "," not in cuda_visible: | ||
| self._physical_gpu_id = int(cuda_visible) | ||
| else: | ||
| self._physical_gpu_id = torch.cuda.current_device() |
There was a problem hiding this comment.
The physical GPU ID detection logic fails when CUDA_VISIBLE_DEVICES contains a comma-separated list of multiple GPUs (e.g., "2,3"). In this scenario, "," not in cuda_visible evaluates to False, causing the code to fall back to torch.cuda.current_device(). This returns the relative device index (e.g., 1) rather than the actual physical GPU ID (e.g., 3), leading to a mismatch with the inference side and potential silent deadlocks during weight transfer.
To fix this, parse the comma-separated list and map the current device index to the corresponding physical GPU ID.
| cuda_visible = os.environ.get("CUDA_VISIBLE_DEVICES", "") | |
| if cuda_visible and "," not in cuda_visible: | |
| self._physical_gpu_id = int(cuda_visible) | |
| else: | |
| self._physical_gpu_id = torch.cuda.current_device() | |
| cuda_visible = os.environ.get("CUDA_VISIBLE_DEVICES", "") | |
| self._physical_gpu_id = torch.cuda.current_device() | |
| if cuda_visible: | |
| try: | |
| gpu_ids = [int(x.strip()) for x in cuda_visible.split(",") if x.strip()] | |
| dev_idx = torch.cuda.current_device() | |
| if dev_idx < len(gpu_ids): | |
| self._physical_gpu_id = gpu_ids[dev_idx] | |
| except ValueError: | |
| pass |
There was a problem hiding this comment.
Fixed in ca17ad8 per the suggestion: the physical GPU id is now resolved by indexing the parsed CUDA_VISIBLE_DEVICES list with torch.cuda.current_device(), with a fallback to the relative index for UUID-style entries.
# Conflicts: # areal/api/cli_args.py # areal/infra/controller/train_controller.py # areal/utils/stats_tracker.py # docs/en/cli_reference.md # docs/zh/cli_reference.md
6e5693e to
8d417bd
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
|
||
| update_finished_key = f"weights_update_finished{key_suffix}" | ||
| try: | ||
| self._meta_server_client.get_object( |
There was a problem hiding this comment.
Can we consider adding an alert message for timeouts here?
There was a problem hiding this comment.
Added in ca17ad8: the writer now logs an explicit error (timeout budget + MetaServer key + likely cause) before re-raising when the inference side never consumes the published weights.
ca17ad8 to
0985d21
Compare
4faf515 to
173c650
Compare
| import sys | ||
|
|
||
|
|
||
| def _early_set_alloc_conf() -> None: |
There was a problem hiding this comment.
AWEX_ACTOR_ALLOC_CONF appears only as a consumer in this diff — I don't see it defined in cli_args.py or documented anywhere, so it is currently an env-only knob that users have to set through SchedulingSpec.env_vars. If it moves into the scheduler it would be natural to promote it to a real config field at the same time. Was leaving it out of the config surface intentional?
There was a problem hiding this comment.
Good question, and the answer is that it should never have been a config field in the first place. AWEX_ACTOR_ALLOC_CONF is removed entirely in 835fea7.
It could not become a real config field. PYTORCH_CUDA_ALLOC_CONF is read once when the allocator is first initialised, and importing areal already initialises CUDA, so by the time Hydra has parsed a config the setting is frozen (torch.cuda.memory_snapshot()[...]['is_expandable'] shows this directly). The only place early enough was argv sniffing at the very top of areal/__init__.py, which is exactly the import-time side effect worth avoiding. So 'env-only' was not an oversight in the config surface, it was inherent to doing it inside the process at all.
The knob belongs to whoever starts the process, so it is now a per-role scheduling_spec.env_vars entry. That is also why this PR depends on #1584: without the user-mapping re-apply, an explicit env_vars entry is overwritten by the framework defaults.
Two follow-ups from digging into this:
sglang_plugin.pyused to rewrite the allocator config under__main__. That was dead code for the same reason (the module importsareal.utilsat line 35, so the config is already frozen). It is now an assertion at the top of the module instead, which fails loudly rather than silently doing nothing.tests/test_alloc_conf_import_side_effects.pygrepsareal/and fails if the name comes back.
The last thing this file still carried was a cosmetic import reformat left over from the removal. Reverted in 51105df, so areal/infra/rpc/rpc_server.py is out of this PR's diff now.
| } | ||
|
|
||
|
|
||
| def query_terminal_state_sacct(job_id: int) -> JobState | None: |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1583. NODE_FAIL was unmapped and query_terminal_state_sacct is what lets the scheduler tell 'job finished' from 'slurmctld hiccup' once squeue has forgotten the job.
| # Must tolerate a partially-constructed trainer (called from | ||
| # __init__'s failure path), and one engine's destroy() failure must | ||
| # not keep the remaining workers alive. | ||
| saver = getattr(self, "saver", None) |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1585. close() had to tolerate a partially constructed trainer and keep going after one component fails, otherwise the remaining workers stay alive.
| cancel_jobs, | ||
| parse_slurm_nodelist, | ||
| query_jobs, | ||
| query_terminal_state_sacct, |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1583 together with the function it imports.
| cached_state, cached_time = self._job_status_cache[job_id] | ||
| if current_time - cached_time < self._status_cache_ttl: | ||
| if cached_state in [JobState.FAILED, JobState.CANCELLED]: | ||
| if not cached_state.active(): |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1583. The cached branch had the same FAILED/CANCELLED-only assumption as the live one.
| self._job_status_cache[job_id] = (state, current_time) | ||
|
|
||
| if state in [JobState.FAILED, JobState.CANCELLED]: | ||
| # Workers are long-lived rpc_server processes: any terminal state |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1583. Workers are long-lived rpc_server processes, so COMPLETED also means they are gone - for instance the batch script exiting 0 after a container FATAL.
| f"{role}/*", -1, f"Job {job_id} {state}. Logs:\n{logs}" | ||
| ) | ||
| except subprocess.CalledProcessError as e: | ||
| # squeue exits non-zero once a job leaves the queue (e.g. |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1583. This is the squeue-exits-non-zero-after-completion case, resolved by asking sacct instead of guessing.
| sbatch_options.append(f"--nodelist={nodelist}") | ||
| if exclude: | ||
| sbatch_options.append(f"--exclude={exclude}") | ||
| if spec.reservation: |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1584, along with the reservation/exclusive config fields.
| ) | ||
| sch.env_vars.update(thread_env) | ||
|
|
||
| # Re-apply user env vars to allow explicit overrides |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1584. This is the half that re-applies the user's mapping last so an explicit env_vars entry is not overwritten by the framework defaults.
|
|
||
| # Amend environment variables | ||
| for sch in schedulings: | ||
| # Save user-specified env vars so they take precedence over system defaults |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1584, paired with the re-apply below it.
| # after the (larger) retry budget is exhausted, so this does not mask | ||
| # real crashes. | ||
| long_op = method in ("save", "load") | ||
| retry_kw = dict(max_retries=8, retry_delay=2.0) if long_op else {} |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1585, together with the SIGTERM handling and the sbatch container staggering - all three are about not leaving processes behind.
| exclude: str | None = field( | ||
| default=None, metadata={"help": "sbatch/srun's `--exclude` option for slurm."} | ||
| ) | ||
| reservation: str | None = field( |
There was a problem hiding this comment.
This content does not belong to the current awex colocate card scope. Please remove it from the current PR and open a new PR.
There was a problem hiding this comment.
Moved to #1584. The two fields and the sbatch options that consume them are now in one PR.
| ) | ||
| if dp_rank == mpu.get_data_parallel_rank(): | ||
| self._context_and_model_parallel_group = group | ||
| for dp_rank, ranks in enumerate(context_and_model_parallel_ranks): |
There was a problem hiding this comment.
Can a judgment be added to create in the common colocate mode?
There was a problem hiding this comment.
Good catch - gated in 0d8198e.
resolve_broadcast_target only reads cpu_model_parallel_group once an
offloaded engine has handed the accelerator to rollout and device collectives are
unusable, so a separation run was paying one gloo new_group per data-parallel
group at startup for a group nothing ever reads.
One thing worth noting for anyone touching this later: the obvious guard,
self.is_offload, is the wrong one. It is initialised to False and only flips
inside offload(), which runs long after _init_context_and_model_parallel_group
during setup - gating on it would mean the group is never built and colocation
breaks. The guard is the static self.config.offload instead, which is also what
the trainer uses to decide whether a role offloads at all.
No consumer change was needed: the attribute already defaults to None and
resolve_broadcast_target falls back to the device group in that case. The
existing test_engine_without_cpu_group_keeps_device_broadcast covers that
fallback and still passes. Added tests/test_gloo_mirror_group_gating.py, which
asserts the gloo new_group stays inside an offload-guarded branch - the
invariant here is where the call sits, so a behavioural test would not catch a
regression.
Add shared-GPU colocated training: the Megatron actor and the SGLang rollout engine time-share the same devices, coordinated through AWEX weight transfer. - AWEX weight-sync writer with tag-based offload/onload and an SGLang engine plugin implementing the colocate reader protocol. - Colocation scheduling support in the Slurm scheduler and controllers (pause/resume generation around the train phase, engine-level offload orchestration, recover handling). - Megatron model registration and fast HF checkpoint loading for the Bailing hybrid MoE family. Both sides key the CUDA IPC handoff on physical GPU ids, mapping through the device mask. Deriving them differently only agrees when the mask starts at zero, so a process pinned to any other GPU would await a key its peer never publishes. Pausing for a weight update keeps SGLang's default mode, which aborts in-flight requests and returns their partial output so the client resumes them by extending the prompt. A second in-place pause raises the scheduler's paused flag so the colocate loop services awex work. Splitting it this way leaves the scheduler fully idle, which SGLang requires before releasing memory, and other backends keep their single-request pause. Supports SGLang 0.5.9 and 0.5.10, whose removed decode-stat hooks are now optional. Verified on multi-node colocated RL runs: training statistics align with the separated-GPU baseline, and single-batch replays produce bitwise-identical training stats. A single-node example test covers the colocate path end to end.
weight_update_mode alone does not identify the colocated v1 run. Controller v2 selects AWEX from use_lora and never reads that field, so a v2 separation run may legitimately carry weight_update_mode="awex" and would then take the v1 colocation handover: its offload was disabled, a stray meta server started, awex_colocate_mode was forced onto its SGLang servers, and both checkpoint branches were skipped so it wrote nothing at all. Gate on _is_v1_awex_colocate, which also requires controller v1 and an actor-rollout colocation strategy. The weight-update meta dispatch keeps its comparison: it sits in an elif chain that v2 short-circuits earlier. Behaviour is unchanged for every configuration that exists today; the three colocated example configs still select the path and the separation config still does not.
…port
AWEX_ACTOR_ALLOC_CONF existed because the colocate examples point rollout at
${actor.scheduling_spec}, so both roles shared one env_vars mapping while the
actor wants expandable_segments and SGLang's memory saver cannot tolerate it.
Applying it required rewriting PYTORCH_CUDA_ALLOC_CONF from the first lines of
the top-level package, because the `from .infra` chain initializes CUDA and
freezes the allocator config; that put argv sniffing and an environment mutation
into every `import areal`, and the same block was duplicated in rpc_server.
Give each colocated role its own scheduling_spec env_vars instead: the env then
reaches the process through `srun --env`, before it starts, so no import-time
hook is needed. Drop the mechanism and both copies.
The mirror of it in the SGLang plugin never worked. It ran from the __main__
block, long after the module-level `from areal.utils import ...` had already
frozen the allocator config, so it rewrote the variable while allocations stayed
expandable. Replace it with an assertion that runs before any areal import and
fails loudly, since a silently self-disabled memory saver surfaces much later as
a colocate OOM or an invalid CUDA IPC target.
7926187 to
835fea7
Compare
|
Split the out-of-scope work into three PRs, all based on current
This PR is rebased onto Verification that nothing was lost or gained: merging the three PRs into the One dependency worth flagging: the per-role Still open on my side: the two free functions in |
…load resolve_broadcast_target reads cpu_model_parallel_group only after an offloaded engine has handed the accelerator to rollout and device collectives are unusable. Creating it unconditionally cost one gloo new_group per data-parallel group at startup on every run, including separation runs that never offload. Gate it on the engine's offload config. Nothing changes on the consumer side: the attribute already defaults to None and resolve_broadcast_target falls back to the device group in that case.
The gate added earlier also demanded an actor-rollout colocation scheduling strategy. AWEX runs opt in through weight_update_mode and leave actor and rollout on the default separation strategy, so the gate went false for every such run: the controller never started the AWEX meta server nor passed its address to the rollout, each training worker then started a server of its own, and the rollout registered against a different one. The run waited on 'infer_conf' until the timeout with no error. Keep the controller-version check, which is what stops a v2 separation run from taking this path, and drop the strategy check. The truth table in the test was asserting the broken behaviour, so it is corrected alongside.
Mirrors the two-GPU setup the example test exercises: a Megatron actor and an SGLang rollout time-sharing both GPUs, with weights handed over through AWEX. The TMS switches live in the actor's own scheduling_spec env_vars rather than a global toggle: SGLang opens its own memory-saver regions for the colocated rollout, and an auto-opened region on the training side would nest inside them.
The parenthesised import left behind by removing the AWEX allocator environment knob carries no semantic change, so rpc_server.py no longer needs to appear in this change set at all.
Both helpers are only reachable from RecoverHandler.load(), so keeping them at module level widened the public surface of areal.utils.recover for no caller. They become private static methods next to the existing _ensure_recover_supported/_normalize_recover_engines pair.
Description
Add shared-GPU colocated RL training: the Megatron actor and the SGLang rollout engine time-share the same devices, coordinated through AWEX weight transfer.
examples/swe/apply_sgl_radix_cache_patch.py).Related Issue
N/A
Type of Change
Checklist
tests/test_stats_tracker.py,tests/test_functional.py,tests/test_ppo_stats.py,tests/test_train_controller.py)main/create-prAdditional Context
Verified on multi-node colocated RL runs: training statistics align with the separated-GPU baseline, and replaying an identical batch produces bitwise-identical float64 training stats. Integration requires multi-node GPU hardware, so end-to-end colocation runs are not covered by CI; unit coverage focuses on the loss/stats math and controller logic.
Validation
Colocated GSM8K on SGLang 0.5.10.post1, in two shapes.
Two nodes, sixteen GPUs:
One 50-step run on this branch and two 100-step runs on the same colocate path
all finished cleanly. For the 50-step run: 56.2 s/step, every expected
weight-update completion signal present (50 steps x 16 ranks), and no
tracebacks, illegal memory accesses, invalid-argument errors, or fully-idle
assertion failures.
One node, two GPUs, matching the example test added here:
Ten steps at 19 s/step, clean exit, no errors.
Ablation established which changes the SGLang 0.5.10 port actually needs.
Guarding the removed decode-stat hooks and draining before memory release are
both required: without the guard the scheduler dies at startup, and without the
drain the third training step trips SGLang's fully-idle assertion. Explicitly
managing the new
cuda_graphmemory-saver tag turned out not to be required,so it is not included here.
Unit tests cover the pause sequence and the physical-GPU-id mapping that keys
the CUDA IPC handoff, and pass against both SGLang 0.5.9 and 0.5.10.post1.