feat (async): support evaluation for fully-async training (dedicated fleet / pause-the-world / external service) - #1740
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated evaluation fleet for fully-async training, allowing evaluations to run on separate engines pinned to Hugging Face checkpoint snapshots without stalling training. It adds new CLI configuration options, an EvalDispatcher to manage async evaluation tasks, a standalone checkpoint evaluation service tool, and corresponding tests. The review feedback focuses on improving robustness and error handling, specifically suggesting to verify if the checkpoint path is a local directory before exporting, replacing assert statements with explicit exceptions (like ValueError and RuntimeError) to prevent them from being stripped under Python optimization, and narrowing exception handling in health check probes.
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.
| if is_writer: | ||
| assert weight_map, f"HF export to {path} produced no weights" | ||
| for meta_file in Path(args.hf_checkpoint).iterdir(): | ||
| # Copy tokenizer/config metadata only — never base weight files or the | ||
| # base checkpoint's safetensors index (ours is written below). | ||
| if meta_file.is_file() and not any(s in meta_file.name for s in HF_METADATA_SKIP_SUFFIXES): | ||
| shutil.copy2(meta_file, path / meta_file.name) |
There was a problem hiding this comment.
If args.hf_checkpoint is a Hugging Face Hub model ID (e.g., Qwen/Qwen2.5-7B-Instruct) rather than a local directory, Path(args.hf_checkpoint).iterdir() will raise a FileNotFoundError and crash the direct HF export process. Check if the path is a directory before attempting to copy metadata files.
| if is_writer: | |
| assert weight_map, f"HF export to {path} produced no weights" | |
| for meta_file in Path(args.hf_checkpoint).iterdir(): | |
| # Copy tokenizer/config metadata only — never base weight files or the | |
| # base checkpoint's safetensors index (ours is written below). | |
| if meta_file.is_file() and not any(s in meta_file.name for s in HF_METADATA_SKIP_SUFFIXES): | |
| shutil.copy2(meta_file, path / meta_file.name) | |
| if is_writer: | |
| assert weight_map, f"HF export to {path} produced no weights" | |
| checkpoint_path = Path(args.hf_checkpoint) | |
| if not checkpoint_path.is_dir(): | |
| raise FileNotFoundError( | |
| f"hf_checkpoint '{args.hf_checkpoint}' is not a local directory. " | |
| f"Direct HF export requires a local directory containing the model metadata files." | |
| ) | |
| for meta_file in checkpoint_path.iterdir(): | |
| # Copy tokenizer/config metadata only — never base weight files or the | |
| # base checkpoint's safetensors index (ours is written below). | |
| if meta_file.is_file() and not any(s in meta_file.name for s in HF_METADATA_SKIP_SUFFIXES): | |
| shutil.copy2(meta_file, path / meta_file.name) |
| if args.eval_num_gpus > 0: | ||
| assert ( | ||
| enable_experimental_rollout_refactor() | ||
| ), "--eval-num-gpus requires the class-based rollout API (MILES_EXPERIMENTAL_ROLLOUT_REFACTOR=1)." | ||
| assert args.eval_interval is not None, "--eval-num-gpus requires --eval-interval." | ||
| assert args.eval_hf_dir is not None or args.save_hf is not None, ( | ||
| "--eval-num-gpus requires a snapshot source: set --eval-hf-dir (staging exports) " | ||
| "or --save-hf (reuse periodic HF checkpoints)." | ||
| ) | ||
| assert not args.colocate, ( | ||
| "--eval-num-gpus is not supported with --colocate; " | ||
| "use tools/checkpoint_eval_service.py against --save-hf checkpoints instead." | ||
| ) | ||
| assert ( | ||
| not args.debug_train_only and not args.debug_rollout_only | ||
| ), "--eval-num-gpus is not supported with debug_train_only/debug_rollout_only." | ||
| assert args.eval_num_gpus % args.eval_num_gpus_per_engine == 0, ( | ||
| f"eval_num_gpus ({args.eval_num_gpus}) must be divisible by " | ||
| f"eval_num_gpus_per_engine ({args.eval_num_gpus_per_engine})." | ||
| ) | ||
| assert args.eval_keep_snapshots >= args.eval_max_in_flight, ( | ||
| f"--eval-keep-snapshots ({args.eval_keep_snapshots}) must be >= --eval-max-in-flight " | ||
| f"({args.eval_max_in_flight}), otherwise a pending eval's snapshot could be GC'd." | ||
| ) | ||
| if args.eval_hf_dir is None: | ||
| # Reuse mode: every eval-due step must coincide with a save-due step. | ||
| assert args.save_interval is not None and args.eval_interval % args.save_interval == 0, ( | ||
| "Reusing --save-hf checkpoints for eval requires eval_interval to be a " | ||
| f"multiple of save_interval (got eval_interval={args.eval_interval}, " | ||
| f"save_interval={args.save_interval}). Set --eval-hf-dir for independent snapshots." | ||
| ) |
There was a problem hiding this comment.
Use ValueError instead of assert for validating command-line arguments. If Python is run with optimization (-O), assert statements are compiled away, which would silently bypass all validation checks.
if args.eval_num_gpus > 0:
if not enable_experimental_rollout_refactor():
raise ValueError("--eval-num-gpus requires the class-based rollout API (MILES_EXPERIMENTAL_ROLLOUT_REFACTOR=1).")
if args.eval_interval is None:
raise ValueError("--eval-num-gpus requires --eval-interval.")
if args.eval_hf_dir is None and args.save_hf is None:
raise ValueError(
"--eval-num-gpus requires a snapshot source: set --eval-hf-dir (staging exports) "
"or --save-hf (reuse periodic HF checkpoints)."
)
if args.colocate:
raise ValueError(
"--eval-num-gpus is not supported with --colocate; "
"use tools/checkpoint_eval_service.py against --save-hf checkpoints instead."
)
if args.debug_train_only or args.debug_rollout_only:
raise ValueError("--eval-num-gpus is not supported with debug_train_only/debug_rollout_only.")
if args.eval_num_gpus % args.eval_num_gpus_per_engine != 0:
raise ValueError(
f"eval_num_gpus ({args.eval_num_gpus}) must be divisible by "
f"eval_num_gpus_per_engine ({args.eval_num_gpus_per_engine})."
)
if args.eval_keep_snapshots < args.eval_max_in_flight:
raise ValueError(
f"--eval-keep-snapshots ({args.eval_keep_snapshots}) must be >= --eval-max-in-flight "
f"({args.eval_max_in_flight}), otherwise a pending eval's snapshot could be GC'd."
)
if args.eval_hf_dir is None:
# Reuse mode: every eval-due step must coincide with a save-due step.
if args.save_interval is None or args.eval_interval % args.save_interval != 0:
raise ValueError(
"Reusing --save-hf checkpoints for eval requires eval_interval to be a "
f"multiple of save_interval (got eval_interval={args.eval_interval}, "
f"save_interval={args.save_interval}). Set --eval-hf-dir for independent snapshots."
)References
- Use ValueError instead of assert for validating function or constructor arguments.
| try: | ||
| await get(f"http://{ip}:{port}/health_generate", max_retries=1) | ||
| return | ||
| except Exception: | ||
| await asyncio.sleep(5) |
There was a problem hiding this comment.
Avoid catching broad exceptions like Exception in health check probes. Only catch expected network, timeout, or unavailability exceptions (such as httpx.HTTPError and asyncio.TimeoutError) to prevent swallowing internal programming errors or bugs.
| try: | |
| await get(f"http://{ip}:{port}/health_generate", max_retries=1) | |
| return | |
| except Exception: | |
| await asyncio.sleep(5) | |
| import httpx | |
| try: | |
| await get(f"http://{ip}:{port}/health_generate", max_retries=1) | |
| return | |
| except (httpx.HTTPError, asyncio.TimeoutError): | |
| await asyncio.sleep(5) |
References
- In health check endpoints or probes, avoid catching broad exceptions (like Exception). Only catch expected network, timeout, or unavailability exceptions.
| assert ( | ||
| str(info.get("weight_version")) == weight_version | ||
| ), f"weight_version pin failed: engine reports {info.get('weight_version')}, expected {weight_version}" |
There was a problem hiding this comment.
Do not use assert for runtime checks that must always execute, as they are stripped when Python is run with optimization (-O). Use RuntimeError instead.
| assert ( | |
| str(info.get("weight_version")) == weight_version | |
| ), f"weight_version pin failed: engine reports {info.get('weight_version')}, expected {weight_version}" | |
| if str(info.get("weight_version")) != weight_version: | |
| raise RuntimeError( | |
| f"weight_version pin failed: engine reports {info.get('weight_version')}, expected {weight_version}" | |
| ) |
| if args.wandb_mode == "shared": | ||
| assert args.wandb_run_id, "--wandb-mode shared requires --wandb-run-id of the training run" |
There was a problem hiding this comment.
Use ValueError instead of assert for validating command-line arguments or configurations to prevent checks from being stripped under Python optimization (-O).
| if args.wandb_mode == "shared": | |
| assert args.wandb_run_id, "--wandb-mode shared requires --wandb-run-id of the training run" | |
| if args.wandb_mode == "shared" and not args.wandb_run_id: | |
| raise ValueError("--wandb-mode shared requires --wandb-run-id of the training run") |
References
- Use ValueError instead of assert for validating function or constructor arguments.
| watch_dir = Path(service_args.watch_dir) | ||
| assert watch_dir.is_dir(), f"--watch-dir {watch_dir} does not exist" |
There was a problem hiding this comment.
Use FileNotFoundError instead of assert for validating file or directory existence to prevent checks from being stripped under Python optimization (-O).
| watch_dir = Path(service_args.watch_dir) | |
| assert watch_dir.is_dir(), f"--watch-dir {watch_dir} does not exist" | |
| watch_dir = Path(service_args.watch_dir) | |
| if not watch_dir.is_dir(): | |
| raise FileNotFoundError(f"--watch-dir {watch_dir} does not exist") |
References
- Use ValueError instead of assert for validating function or constructor arguments.
67c5ca8 to
21a18b9
Compare
7cf1112 to
3068087
Compare
2a5770f to
c92bcb9
Compare
Fully-async rollout had no evaluation story: the producer never stops, so
there is no quiet window to evaluate in, and eval on the training engines is
structurally broken (requests aborted by every weight update, no well-defined
weight version measured).
Eval gets two postures, split by where the weights come from:
- Against the live training engines, version pinned by blocking call order:
FullyAsyncRolloutFn serves eval itself and pauses new producer submissions
for the duration (a gate, not a retract - in-flight requests finish and
buffer).
- Against a checkpoint file, version pinned by the file itself. The only
interface between training and eval is a (rollout_id, HF snapshot dir)
pair; eval engines never join training weight updates and load snapshots
via update_weights_from_disk, which stamps the version atomically.
The checkpoint posture has one contract, CheckpointEvalFn (evaluate_checkpoint
+ dispose; raise EvalSkip(reason) for an attributable skipped point), with two
backends: FleetEvalFn (the dedicated in-job fleet behind --eval-num-gpus,
carrying the health-probe / pin / router logic) and the ExternalSglangEvalFn
example (launches or attaches to its own sglang server outside the placement
group). The trainer owns per-point HF export, async dispatch with an explicit
overflow policy (--eval-max-in-flight x --eval-overflow-policy), logging at
the snapshot's step, and snapshot GC; every failure mode degrades to a skipped
point logged as eval/skipped_{reason}, never a stalled trainer.
Pieces: miles/rollout/checkpoint_eval.py (contract + fleet backend +
resolve_checkpoint_eval_fn), FullyAsyncRolloutFn._call_eval,
miles/ray/rollout/eval_dispatch.py (EvalDispatcher: bounded in-flight,
drain-before-dispose), miles/backends/megatron_utils/hf_export.py
(export_hf_model_direct through the direct megatron->HF converters so export
coverage matches weight-sync coverage, with a .complete marker; save_hf_model
moves here from model.py, which was over the 1000-line file budget),
examples/fully_async runner + external backend, docs, new args + validation,
unit tests, and tests/e2e/megatron/test_qwen3_4b_fully_async_eval.py covering
all three postures as short Qwen3-4B GRPO runs (labels megatron/eval; the eval
label joins tests/ci/labels.py, its run-ci-eval GitHub label already exists).
Eval-path resolution happens after the --fully-async override so
shared-engine eval reaches the same FullyAsyncRolloutFn instance whose
producer it pauses; RolloutManager reuses the instance when the two paths are
equal.
Verified on 4xH200 (MILES_TEST_FEW_GPU=1): all three e2e postures succeeded -
shared (3 pinned points, producer pause/resume), fleet (lag_steps 0-1,
export_time ~7s tmpfs snapshots, training unblocked), external (self-launched
server, same-weights scores). tests/fast: 3934 passed / 0 failed. Earlier
8xH200 matrix on Qwen3.5-4B: fleet+tmpfs 0.640->0.882 over 20 steps with all
points pinned, fleet+ckpt-reuse 0.641->0.847, external matching in-job scores
on the same checkpoint, kill tests recovering a dead eval engine without
touching training.
df1130e to
fc2a4bf
Compare
Reverts the eval-metric semantics change that rode along with the fully-async
eval work. It applied to eval_rollout_single_dataset, which is on the path for
every run — including the sync trainer and every config that uses neither the
dedicated eval fleet nor an external backend — so it silently moved existing
eval curves:
- ABORTED / reward-less samples were dropped before the mean instead of being
counted as 0.0, so eval/{ds} became an average over survivors and rose
whenever anything failed.
- eval/{ds}-none_reward_ratio became structurally 0, making the "treating as
0.0 for metrics" warning unreachable.
- A per-sample try/except plus "raise when every sample failed" turned an
empty eval dataset (len(tasks) == 0) into a hard RuntimeError.
Restores eval_rollout_single_dataset and log_eval_rollout_data to main's
behavior, and drops the two tests that covered the removed handling. Nothing
else in the PR depends on the failed_samples key.
Tolerating a partially-dead eval fleet is worth doing, but it needs its own
change with the metric discontinuity called out, not a silent rider.
|
I found one mismatch between the shared-engine eval contract in this PR and the current head (
(
"miles.rollout.fully_async_rollout.FullyAsyncRolloutFn",
"miles.rollout.inference_rollout.inference_rollout_common.InferenceRolloutFn",
)
The bounded fix is to resolve |
- imports to module top (checkpoint_eval, hf_export); state the reason on the two that must stay local (megatron.bridge optional dep, inference_rollout_eval import cycle) - hf_export: absolute import for update_weight.common; functools.cache replaces the hand-rolled _hf_bridge_cache global - keep _resolve_eval_datasets protected: the rename to public had no caller outside arguments.py - drop looks_like_hf_checkpoint: public and unused - args.eval_hf_dir / args.eval_datasets are always set, so read them directly instead of getattr-with-default - keyword args for resolve_checkpoint_eval_fn / FleetEvalFn's ambiguous params
The dispatcher exported snapshots and the manager deleted them, and the manager only reached its GC line on the success path. Every other outcome - export_failed and busy on the driver, ckpt_missing and EvalSkip inside the manager, a crashed eval task - returned without anyone deleting anything, so each one leaked a model-sized directory into --eval-hf-dir. With the documented /dev/shm staging that is a node-down failure after a stretch of unhealthy evals. One party creates and the same party deletes: the dispatcher carries the dir it exported alongside the pending ref and retires it in _settle's finally, which is now the single path every outcome funnels through (_reap_finished, backpressure, and drain all call it). Only dirs the dispatcher exported are eligible, so --save-hf checkpoints, --hf-checkpoint and any caller-supplied dir are structurally out of reach rather than excluded by a path check. Drops from the manager: _eval_consumed_snapshots, _gc_eval_snapshots, and the shutil/pathlib imports. Drops the --eval-keep-snapshots >= --eval-max-in-flight assert from both places it was written: the ring only ever held retired snapshots, so the pending-snapshot-GC'd case it guarded could not happen, as _gc_eval_snapshots' own docstring said. _reap_finished becomes async and awaits the ref like the other two call sites instead of ray.get, so retirement has exactly one implementation.
The slot takes a rollout fn, which generates against engines the framework hands it, or a CheckpointEvalFn subclass, which gets the snapshot path and owns weight delivery, endpoint and generation itself. The help described only the first, so the scope difference was invisible from the CLI - you had to know the type dispatch existed. Also fixes the missing space that ran 'function.' into 'If'.
checkpoint_eval.py's own docstring stopped matching its contents when the fleet left the CheckpointEvalFn contract: it still said FleetEvalFn was the contract's in-job implementation. The file held two unrelated things - a 36-line user-facing contract and 99 lines of engine plumbing - and someone writing a backend had to read past _pin_fleet, _mark_unreachable_engines and _wait_router_ready to reach the part that concerns them. miles/ray/rollout/ is where it belongs by what it touches: RolloutServer.recover, wait_all_engines_alive, engine actor handles, the eval router's address. It sat in miles/rollout/ only because it was born as a CheckpointEvalFn. The file is named after the class, as rollout_server.py / server_group.py / eval_dispatch.py are. checkpoint_eval.py is now 98 lines and reads as one thing: the contract, EvalSkip, retarget_args (public for external backends), and the posture predicates. eval_fleet.py imports EvalSkip and retarget_args from it; nothing goes back the other way. The three fleet tests move to tests/fast/ray/rollout/test_eval_fleet.py alongside the module, using that directory's conftest make_args.
…cate The posture is a pure function of two args, constant for the run, read by the driver's dispatcher and by RolloutManager - which never import each other, so the predicate had to live in a module both could reach and ended up in checkpoint_eval.py next to the contract it has nothing to do with. miles already has the shape for this: miles_validate_args computes ft_components, eval_datasets and the rollout function paths and stores them on args. The posture joins them, so "which module owns this predicate" stops being a question - it is an arg, and args live on args. This also undoes a coupling from the commit that introduced the predicate: eval_dispatch.py had to import checkpoint_eval, which drags in base_types -> data_source -> data -> chat_template_utils -> sglang. It is back to ray and stdlib, and its harness no longer needs a stub to run. is_checkpoint_eval_fn stays in checkpoint_eval.py - it tests a type defined there, and validation still uses it for the two-backends-at-once check.
… eval tp SGLang validates tp_size % (dp_size * attn_cp_size) == 0 and ep_size * moe_dp_size == tp_size. The eval fleet takes its tp from --eval-num-gpus-per-engine but inherited dp/pp/ep/attn_cp from the rollout engines, so a rollout tuned at tp=8 ep=8 with a 1-GPU eval engine produced tp=1 ep=8 - an engine that fails ServerArgs validation at boot. The docs promised the fleet is 'configured exactly like the engines you already tuned', and TP is the one thing that is not inherited, which is exactly what breaks the four that are coupled to it. _EVAL_SKIPPED_SERVER_ARGS already reasoned about tp_size this way and did not carry the reasoning across. When the two TPs match, inheritance is unchanged. When they differ, the four fall back to 1 with a log line, still overridable by --eval-sglang-*.
export_hf_model_direct is collective, but its only failure point - the empty weight_map assert and the index write - runs on rank 0. When it fired, rank 0 unwound while every other rank sat in torch.distributed.barrier() until the NCCL watchdog killed them, and the driver meanwhile saw a fast asyncio.gather failure, logged skipped_export_failed and kept training against wedged actors. The barrier moves into a finally so all ranks always reach it; the marker stays after it, so a failed export still leaves the directory unmarked and the eval point skips as ckpt_missing. The bridge branch raises after its barrier, so ranks diverge but none hang, and the actor stays in sync for later collectives - left alone.
The marker was touched at the end of each export branch but a pre-existing one was never cleared, so a re-export into a path that already had it - a resumed run, or --save-hf writing the same rollout_id again - left the stale marker vouching for half-written shards when the export failed midway. The --eval-hf-dir path survived that because the dispatcher rmtrees on export failure; the --save-hf reuse path did not, since save_model calls save_hf_model with raise_on_error=False and swallows the exception. Both branches now clear the marker before writing, and the single touch moves to the end of save_hf_model - after the LoRA adapter, which used to be written after the marker already claimed the export was done. A failed export or a failed adapter returns without marking, so the eval point skips as ckpt_missing.
The fleet branch checked six things; the CheckpointEvalFn branch checked one, though both drive the same _eval_checkpoint path. The refactor-flag check is the one that mattered: with MILES_EXPERIMENTAL_ROLLOUT_REFACTOR unset, RolloutManager takes the legacy branch and eval_generate_rollout is the class object, so call_rollout_function calls TheClass(eval_input) - constructing a CheckpointEvalFn with a RolloutFnEvalInput - and the job dies at the first eval point after full setup. Same for the reuse-mode modulo check: an external backend on --save-hf with a non-multiple --eval-interval got ckpt_missing on most points instead of an error at startup. The shared checks now hang off args.eval_uses_snapshots, which is exactly the condition that routes to _eval_checkpoint. What stays keyed to --eval-num-gpus is what is genuinely fleet-only: the tp divisibility, and the conflict with a CheckpointEvalFn path (flipped to a negative assert so it sits with its siblings).
CheckpointEvalFn's docstring still pointed at 'FleetEvalFn below', which is not below, is not named that, and is deliberately not a CheckpointEvalFn. The fleet section of the guide said the fleet 'runs the standard eval datasets'; the fleet only delivers weights and the configured eval fn generates, which is what lets custom eval fns work on it unchanged. It also called the whole step fire-and-forget - the export is a collective the training loop waits on, and backpressure can add an eval duration on top, both of which the docs now say.
args.eval_uses_snapshots was computed near the other derived args, ~500 lines before _resolve_rollout_functions assigns args.eval_function_path, so the predicate read an unresolved field. Every sane config gave the same answer - an unset --eval-function-path cannot resolve to a CheckpointEvalFn - but the dependency was inverted and the eval validation I just added inherited it. The derivation moves into _resolve_rollout_functions itself, next to the assignment it depends on, and the two validations that read either field move below the call. Reading them earlier is now a visible ordering error rather than a silently-correct accident.
checkpoint_eval.py opened with the two-posture taxonomy and then spent a paragraph on what the eval fleet is not - framing that only makes sense to someone who watched the refactor. It is the file you open to write your own eval backend, so it now says that first: subclass, point --eval-function-path at it, get a directory and return results. Same edit to the rest: EvalFleet defined by what it is rather than by not being a CheckpointEvalFn, EvalDispatcher by what it does rather than by which bug its ownership rule closed, _retire by the invariant rather than the leak it prevents.
…else probes EvalFleet reached past the server layer to ray.kill engines itself. With --use-fault-tolerance that raced RolloutHealthMonitor, which probes the same engines (RolloutManager builds one per server group, eval included) and calls stop_engines on failure - two probers killing and marking the same actors, then both triggering recover. The probe moves to RolloutServer.probe_and_mark_dead, next to recover() and wait_all_engines_alive() which it exists to feed, and the fleet only runs it when no monitor is watching - which is what its own docstring said it was for. eval_fleet.py no longer imports ray at all.
args.eval_uses_snapshots was copied into self._uses_snapshots and self._snapshot_eval, so the same value went by three names that do not grep to each other. Both classes already hold args; read it there.
The manager inferred 'this is the pre-training baseline' from hf_dir equalling args.hf_checkpoint - a value comparison standing in for intent, agreed on across a Ray call. Pointing --eval-hf-dir at the base checkpoint, or ever running the baseline from a different path, would have changed behaviour silently. The dispatcher knows: a caller-supplied hf_dir is an existing checkpoint, its own exports and --save-hf reuse are not. It passes require_marker.
Thanks for the comment! This issue has been fixed in b0aa5e5 commit |
Two resolutions, one of them not a textual conflict: _compute_server_args: main added the MILES_SGLANG_DUMMY_LOAD block where this branch used to merge sglang_overrides, and this branch had moved that merge to the end so a per-group override wins over every args-derived default. Kept main's block in place; the merge stays last, so an override of load_format still beats the env var. model.py: git merged cleanly but the result did not import. This branch dropped `from pathlib import Path` when save_hf_model moved to hf_export.py, and main's new inkling LoRA loading and _has_loadable_ckpt both use Path. Restored the import.
An inherited eval path is the rollout fn serving eval itself, never a CheckpointEvalFn, but is_checkpoint_eval_fn resolved it anyway - importing user-supplied rollout modules on the driver at validation time (test_keeps_user_supplied_rollout_fn_and_data_source caught this with its my.custom.rollout_fn path). Only an explicit --eval-function-path is resolved now; the fleet-conflict assert moves next to the derivation it shares the check with. Fixtures that hand-build Namespaces gain the derived field: the rollout conftest pins eval_uses_snapshots=False, test_checkpoint_eval's make_args defaults it True, and make_manager writes it on args (the field the manager actually reads) instead of the dead _uses_snapshots attribute.
The dispatcher now tells the manager whether a snapshot needs its .complete marker; the fake's eval signature predates that and rejected the call. Record the flag and assert the split: exports and --save-hf reuse require it, a caller-supplied hf_dir (the pre-training baseline) never wrote one.
Both eval e2e cases run train_async.py --fully-async, but neither declared the fully-async label, so run-ci-fully-async selected only test_qwen3_30B_A3B/ test_fully_async.py and skipped the two that added fully-async eval. The 0.5B case also runs --eval-num-gpus 1 without declaring eval, which left run-ci-eval selecting a single 2400s 8-GPU H200 job when a 400s H100 one covers the same postures. Labels are additive here - a test runs if any of its labels is included - so this only widens selection.
Snapshot GC moved from RolloutManager to EvalDispatcher, so _retire started reading args.eval_keep_snapshots, which make_dispatcher never set - every dispatcher test that settles a pending ref died on AttributeError. save_hf was missing the same way; only the reuse-mode test set it, so nothing had failed on it yet.
|
LGTM for the code changes! |
Conflict: fully_async_rollout.py gained upstream's eval support (#1740) next to this branch's aclose(); both are kept.
test_eval_without_fleet_pauses_producer arrived from #1740 after this branch updated the other generate stubs, and the merge took both sides cleanly because they touch different regions of the file.
Motivation
Fully-async rollout has no evaluation story: the producer never stops, so there is no quiet window to evaluate in, and running eval on the training engines is structurally broken — eval requests get aborted by every weight update, and no well-defined weight version is measured. Heavy eval sets also steal training inference capacity even in sync mode.
Design
Eval has exactly two postures, split by where the weights come from:
FullyAsyncRolloutFnserves it itself, pausing new producer submissions for the duration (in-flight requests finish and buffer — a gate, not a retract).update_weights_from_disk(hf_dir, weight_version=str(rollout_id)).args.eval_uses_snapshotsis the single discriminant, derived from args in_resolve_rollout_functionsand read by both the driver's dispatcher andRolloutManager— which never import each other.Two ways to fill the second posture
They are mutually exclusive (validated), and they are different kinds of thing:
The in-job eval fleet (
--eval-num-gpus N) delivers weights and nothing else.EvalFleet.pin(dir, version) -> GenerateStateprobes every engine, revives dead ones, loads the snapshot, and confirms each engine reports the expected version before returning the state to generate against. Becausepinreturns the state, generating before pinning is not expressible. Your--eval-function-pathfn then generates against that state exactly as it would against the training engines, so custom eval fns work on the fleet unchanged.A black box (
--eval-function-pathat aCheckpointEvalFnsubclass) takes it from the directory onward:Directory in, results out, anything in between — a non-sglang service implements this by submitting
checkpoint_dirto its API. The fleet is deliberately not one of these; it is internal plumbing, not a user-facing contract.examples/fully_async/external_eval_fn.pyis the reference implementation: it launches its own sglang server on user-named GPUs (MILES_EXTERNAL_EVAL_GPUS) or attaches to one (MILES_EXTERNAL_EVAL_URL).Because the fn runs in-job, all eval config (datasets, sampling, rm, lora) comes from the real training args — nothing is hand-copied.
Engine configuration
The eval fleet inherits every
--sglang-*setting from the rollout engines; override any single field with--eval-sglang-*(argparse.SUPPRESSdefaults, so unset really means inherit; booleans take a--no-form). Not inheritable: TP comes from--eval-num-gpus-per-engine, which also places the engines, anddp/pp/ep/attn_cpfall back to 1 when the eval TP differs from the rollout TP — SGLang ties them totp_size, so inheriting them across a different TP produces an engine that fails its own validation at boot.Semantics
eval/lag_stepsreports how late). The export is not fire-and-forget — it is a collective the training loop waits on,eval/export_time_secondsper point; reuse mode has no export at all.weight_versionis verified after every load;eval/{ds}/weight_version/mean == eval/stepandmixed_version_ratio == 0prove each point measured exactly the intended weights.--eval-keep-snapshots + --eval-max-in-flightmodel-sized dirs.eval/skipped_{busy,export_failed,ckpt_missing,unhealthy,pin_violation,crashed}). Overflow is two orthogonal knobs (--eval-max-in-flight N×--eval-overflow-policy backpressure|skip), and the final eval point is never skipped.--pause-generation-mode in_placeandretract(retractadditionally requires [sglang-miles] Fix flush_cache() no-op after pause_generation in retract sgl-project/sglang#31962 + [Fix] retract-mode flush_cache no-op crash #1750).What's in the PR
Core:
miles/rollout/checkpoint_eval.py— the black-box contract:CheckpointEvalFn,EvalSkip,is_checkpoint_eval_fn, andretarget_args(public for external backends)miles/ray/rollout/eval_fleet.py—EvalFleet.pin: health probe, load, per-engine version read-back, router probemiles/ray/rollout/eval_dispatch.py— driver-sideEvalDispatcher: bounded in-flight dispatch, overflow policy, snapshot export and retirement, drain-before-disposemiles/ray/rollout/rollout_manager.py—_eval_checkpoint, one call site for both postures: pin first when there is a fleet, then calleval_generate_rolloutmiles/rollout/fully_async_rollout.py—_call_eval: generate on the injected state, or pause the producer and use its ownmiles/backends/megatron_utils/hf_export.py—export_hf_model_directgoes through miles' direct megatron→HF converters, so export coverage always matches weight-sync coverage;save_hf_modelmoved here frommodel.py, restoring the 1,000-line file budgetexamples/fully_async/external_eval_fn.py+run_qwen3_5_4b_fully_async_eval.py— both backends behind one flag (--eval-backend fleet|external)docs/user-guide/fully-async.md, CLI reference)Tests:
tests/e2e/megatron/test_qwen3_4b_fully_async_eval.pycovers all three postures as short Qwen3-4B GRPO runs (suitestage-c-8-gpu-h200, labelsmegatron/eval; applyrun-ci-evalto run it on a PR), plus fast tests for the contract, fleet pin/recovery, dispatcher policies and snapshot ownership, the manager snapshot path, and the--eval-sglang-*inheritance mechanism.Review history
The branch was reviewed and reworked in place; the design above is the reworked one. What changed and why:
FleetEvalFn→EvalFleet, out of theCheckpointEvalFncontract and intomiles/ray/rollout/. The fleet is not a black box — it must hand generation back to the configured eval fn — and forcing it into the contract produced theinnerconstructor argument no other implementation had, a "privileged, not via the CLI flag" caveat, and--eval-num-gpussilently beating--eval-function-path.resolve_checkpoint_eval_fndisappeared with it.--eval-hf-dir— a node-down failure with the documented/dev/shmstaging.resolve_rollout_function_pathscomputed the eval path before the--fully-asyncoverride, so eval resolved to a secondInferenceRolloutFnandFullyAsyncRolloutFn._call_evalwas never reached. The GPU verification below therefore did not exercise the pause, despite reporting it.--eval-sglang-*replaced "the fleet is whatever rollout is"; thesglang_overridesmerge in_compute_server_argsmoved after the args-derived branches so a per-group override actually wins. That also fixes existing--sglang-configper-group overrides ofdtype/ LoRA / disaggregation keys, which previously lost to those branches — worth a look from anyone running PD configs.torch.distributed.barrier()until the NCCL watchdog); the.completemarker is cleared before a re-export and written after the LoRA adapter; both snapshot postures now get the same validation, including the class-based-API check whose absence crashed the black-box posture at the first eval point.--eval-model-path(nothing set it, and the boot weights are overwritten before the first eval); the--eval-keep-snapshots >= --eval-max-in-flightassert (the GC ring only ever held retired snapshots).Verification
CI on
fc2a4bf, the first commit:PR Testrun green with the eval e2e actually executing:test_qwen3_4b_fully_async_eval.pyPASS instage-c-8-gpu-h200(23 min) — shared / fleet / external each completed as a separate successful ray job. Note: without arun-ci-<label>PR label the GPU stages select nothing, so a labelless green does not exercise this test.tests/fast3934 passed / 20 skipped / 0 failed.GPU matrix (8×H200, Qwen3.5-4B, dapo-math training, gsm8k eval), also on
fc2a4bf:lag_steps ≤ 11, training uninterruptedskipobserved and attributedrecover(), and the next eval lands pinned — training untouchedThese runs predate the rework and need repeating on the head commit. In particular the shared-engine numbers were produced by code in which the producer pause could not run, so that posture is effectively unverified.
🤖 Generated with Claude Code