Repository navigation
Conversation
Port of miles@ba28991834 miles/rollout/base_types.py. Additive class-based rollout-function API (RolloutFnConstructorInput, RolloutFnTrainInput, RolloutFnEvalInput, BaseRolloutFn) so eval/checkpoint plugins and later B7/B9 work can share one contract. Legacy call_rollout_fn unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Port of miles@ba28991834 miles/ray/rollout/eval_dispatch.py. EvalDispatcher (in-flight cap, overflow policy, snapshot GC) hosted on a daemon asyncio thread for the sync driver; export_hf RPC on the Megatron actor/train group; RolloutManager.eval gains hf_dir/export kwargs (rejecting snapshots until the fleet lands) plus report_eval_skip; new --eval-hf-dir/--eval-max-in-flight/ --eval-overflow-policy/--eval-keep-snapshots flags. eval_uses_snapshots stays false until the eval fleet or a CheckpointEvalFn arrives, preserving the blocking shared-engine path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Port of miles@ba28991834 miles/ray/rollout/eval_fleet.py, redesigned against ServingCluster/RolloutServer and vLLM engine RPCs (reload_weights collective_rpc + /weight_info read-back + health_generate probe) instead of the miles inference-controller/SGLangApiClient surface. The vLLM config gains an update_weights=False 'eval' model with its own router; placement appends eval GPU bundles past every other region so trainer offsets never shift (colocate included). Evals run on a driver-owned EvalRunner actor (off the RolloutManager critical path) with pin/version-check semantics and EvalSkip reasons. v1 gap: eval engines are not part of the recoverable serving topology. GPU e2e gate (tests/test_eval_fleet_nonblocking.py) asserts fleet wall time <= 1.10x shared-engine baseline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Port of miles@ba28991834 miles/rollout/checkpoint_eval.py and examples/infra_features/fully_async/external_eval_fn.py. CheckpointEvalFn/EvalSkip/is_checkpoint_eval_fn/retarget_args (vLLM router fields instead of sglang's); EvalRunner hosts CheckpointEvalFn instances with the fleet pin skipped (the fn owns weight delivery) and disposes them on shutdown; EvalSkip's canonical home moves to checkpoint_eval (re-exported from eval_fleet). Validation: checkpoint fns are mutually exclusive with --eval-num-gpus and require a snapshot source. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a non-blocking, snapshot-based evaluation dispatch system to prevent evaluation from stalling the training loop. It adds support for a dedicated eval engine fleet or custom CheckpointEvalFn backends, along with comprehensive tests and documentation. The review feedback highlights several critical robustness improvements: preventing indefinite hangs during teardown in close() by adding a timeout to drain(), cleaning up partial directories on failed HF snapshot exports to avoid disk space leaks, wrapping weight version retrieval in a try-except block to handle unresponsive engines gracefully, and catching request exceptions during weight info polling in the external vLLM evaluation function.
| def drain(self) -> None: | ||
| future = asyncio.run_coroutine_threadsafe(self._dispatcher.drain(), self._loop) | ||
| future.result() | ||
|
|
||
| def close(self) -> bool: | ||
| """Drain and stop the loop. Returns False when the thread is stuck; in | ||
| that case the eval runner is left alone because it may be mid-RPC.""" | ||
| self.drain() | ||
| self._loop.call_soon_threadsafe(self._loop.stop) | ||
| self._thread.join(timeout=30) | ||
| if self._thread.is_alive(): | ||
| logger.error("Eval dispatch thread did not stop within 30s; leaving the eval runner undisposed.") | ||
| return False | ||
| if self._on_close is not None: | ||
| self._on_close() | ||
| return True |
There was a problem hiding this comment.
The close() method is designed to handle a stuck thread gracefully by joining with a 30-second timeout. However, it calls self.drain() first, which blocks the caller thread indefinitely on future.result() if any evaluation task is hanging. This completely bypasses the thread-joining timeout and can cause the main training thread to hang forever during teardown.
To fix this, we should add an optional timeout parameter to drain() and call it with a 30-second timeout inside close(), catching any timeout or other exceptions so that the thread-joining and cleanup logic can still run.
| def drain(self) -> None: | |
| future = asyncio.run_coroutine_threadsafe(self._dispatcher.drain(), self._loop) | |
| future.result() | |
| def close(self) -> bool: | |
| """Drain and stop the loop. Returns False when the thread is stuck; in | |
| that case the eval runner is left alone because it may be mid-RPC.""" | |
| self.drain() | |
| self._loop.call_soon_threadsafe(self._loop.stop) | |
| self._thread.join(timeout=30) | |
| if self._thread.is_alive(): | |
| logger.error("Eval dispatch thread did not stop within 30s; leaving the eval runner undisposed.") | |
| return False | |
| if self._on_close is not None: | |
| self._on_close() | |
| return True | |
| def drain(self, timeout: float | None = None) -> None: | |
| future = asyncio.run_coroutine_threadsafe(self._dispatcher.drain(), self._loop) | |
| future.result(timeout=timeout) | |
| def close(self) -> bool: | |
| """Drain and stop the loop. Returns False when the thread is stuck; in | |
| that case the eval runner is left alone because it may be mid-RPC.""" | |
| try: | |
| self.drain(timeout=30) | |
| except Exception as e: | |
| logger.error(f"Timeout or error while draining eval dispatcher: {e}") | |
| self._loop.call_soon_threadsafe(self._loop.stop) | |
| self._thread.join(timeout=30) | |
| if self._thread.is_alive(): | |
| logger.error("Eval dispatch thread did not stop within 30s; leaving the eval runner undisposed.") | |
| return False | |
| if self._on_close is not None: | |
| self._on_close() | |
| return True |
| try: | ||
| export_time = await self._export(rollout_id, hf_dir) | ||
| except Exception as e: | ||
| logger.error(f"HF snapshot export for eval {rollout_id} failed: {e}") | ||
| await self.rollout_manager.report_eval_skip(rollout_id, "export_failed") | ||
| return |
There was a problem hiding this comment.
If the HF snapshot export fails during _export(), any partially written files or directories in hf_dir are left behind. Since this directory is never added to self._exported, it will never be cleaned up by the garbage collection loop in _retire(). Over time, repeated export failures can leak disk space, which is especially problematic if eval_hf_dir is configured on a RAM disk (tmpfs).
We should clean up the partial export directory on failure using shutil.rmtree(hf_dir, ignore_errors=True).
| try: | |
| export_time = await self._export(rollout_id, hf_dir) | |
| except Exception as e: | |
| logger.error(f"HF snapshot export for eval {rollout_id} failed: {e}") | |
| await self.rollout_manager.report_eval_skip(rollout_id, "export_failed") | |
| return | |
| try: | |
| export_time = await self._export(rollout_id, hf_dir) | |
| except Exception as e: | |
| logger.error(f"HF snapshot export for eval {rollout_id} failed: {e}") | |
| await self.rollout_manager.report_eval_skip(rollout_id, "export_failed") | |
| shutil.rmtree(hf_dir, ignore_errors=True) | |
| return |
| versions = gather([engine.get_weight_version.remote() for engine in engines]) | ||
| if any(str(v) != str(weight_version) for v in versions): | ||
| logger.error(f"Eval fleet pin violation: versions {versions} != {weight_version}") | ||
| return EvalFleetPin(skip_reason="pin_violation") |
There was a problem hiding this comment.
Unlike the update_weights_from_disk and health_generate calls in pin_eval_fleet(), the get_weight_version gather is not wrapped in a try...except block. If any engine becomes unresponsive or raises an exception during this call, it will crash the entire evaluation task instead of returning a clean EvalFleetPin(skip_reason="unhealthy").
We should wrap this gather in a try...except block to match the robustness of the other fleet operations.
| versions = gather([engine.get_weight_version.remote() for engine in engines]) | |
| if any(str(v) != str(weight_version) for v in versions): | |
| logger.error(f"Eval fleet pin violation: versions {versions} != {weight_version}") | |
| return EvalFleetPin(skip_reason="pin_violation") | |
| try: | |
| versions = gather([engine.get_weight_version.remote() for engine in engines]) | |
| except Exception: | |
| logger.exception("Eval fleet failed to get weight versions") | |
| return EvalFleetPin(skip_reason="unhealthy") | |
| if any(str(v) != str(weight_version) for v in versions): | |
| logger.error(f"Eval fleet pin violation: versions {versions} != {weight_version}") | |
| return EvalFleetPin(skip_reason="pin_violation") |
| while time.time() < deadline: | ||
| served = requests.get(f"{self._url}/weight_info", timeout=30).json().get("weight_version") | ||
| if str(served) == str(weight_version): | ||
| return | ||
| time.sleep(5) | ||
| raise EvalSkip("pin_violation") |
There was a problem hiding this comment.
During weight reloading, the external vLLM server might be temporarily unresponsive or return connection errors. If requests.get() raises a requests.RequestException (such as a connection timeout or connection refused), the loop will immediately crash and fail the evaluation run instead of retrying until the deadline.
We should wrap the requests.get() call in a try...except requests.RequestException: block to ensure robustness during server reloads.
| while time.time() < deadline: | |
| served = requests.get(f"{self._url}/weight_info", timeout=30).json().get("weight_version") | |
| if str(served) == str(weight_version): | |
| return | |
| time.sleep(5) | |
| raise EvalSkip("pin_violation") | |
| while time.time() < deadline: | |
| try: | |
| response = requests.get(f"{self._url}/weight_info", timeout=30) | |
| if response.ok: | |
| served = response.json().get("weight_version") | |
| if str(served) == str(weight_version): | |
| return | |
| except requests.RequestException: | |
| pass | |
| time.sleep(5) | |
| raise EvalSkip("pin_violation") |
Ports miles' eval fleet:
BaseRolloutFn/RolloutFn*Inputcontract types, non-blocking eval dispatch with HF snapshot export, a dedicated eval fleet onServingCluster(--eval-num-gpus), and theCheckpointEvalFncontract with an external-vLLM reference implementation. miles' dead "receiver pools" were not ported.Source:
radixark/miles@ba28991834. Port is additive (new modules + newadd_*_arguments()groups); slime-origin files are not restructured so future[Sync] Mirror SlimePRs stay clean. One commit per sub-PR; review each commit separately.Commits:
Non-blocking timing e2e needs GPU.
Merge order of the miles-port series: F10 → F02 → F06 → F11a → F11b → F05 → F09 → F04 → F03 → F07 → F08 → F01.
train.py/vime/utils/arguments.pyare touched by several branches; later ones are rebased after each merge.🤖 Generated with Claude Code