refactor(rollout): align vllm_rollout.py with slime sglang_rollout.py#178
Conversation
Mirror slime's sglang_rollout.py structure for vllm_rollout.py (984→765 lines, 29→16 defs). Removed vime-only bloat with no slime equivalent: - _base_dataset_prompt_ids, _apply_vllm_routed_experts (40→7 lines inline), _inference_generate_tokens_and_logprobs (30→6 lines inline), _align_engine_tokens_and_logprobs, _warn_if_sampling_filters_may_affect_logprobs, _vllm_meta_from_generate_choice, _decode_vllm_routed_experts, _router_worker_urls, _resume_vllm_workers — all inlined or removed - Stripped verbose docstrings/comments not present in slime Added slime-aligned utilities: - get_rollout_num_engines() in http_utils.py (mirrors slime, replaces inline rollout_num_gpus // rollout_num_gpus_per_engine) - no_stop_trim → include_stop_str_in_output mapping in _build_inference_sampling_params (PR #71 fix) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the vLLM rollout logic by removing several helper functions, inlining token and logprob parsing, and introducing a helper to determine the number of HTTP engines. However, several issues were identified: the .reshape(...) call on rollout_routed_experts is redundant and risks throwing an error if MoE arguments are missing; removing the list-handling check in generate_and_rm breaks its public API and will cause crashes when a list of samples is passed; and using strict=True in zip without the previous alignment helper risks runtime failures on mismatched token and logprob lengths.
| sample.rollout_routed_experts = np.ascontiguousarray(arr.astype(np.int32, copy=True)).reshape( | ||
| len(sample.tokens) - 1, | ||
| args.num_layers, | ||
| args.moe_router_topk, | ||
| ) |
There was a problem hiding this comment.
The .reshape(...) call is redundant because np.load already restores the original shape of the array from the serialized .npy format. Furthermore, accessing args.num_layers and args.moe_router_topk directly is risky as they may not be defined on args (or may be None), which would cause a runtime AttributeError or TypeError if a user runs a dense model or doesn't pass these arguments.
We should remove the .reshape(...) call to keep it simple and safe.
sample.rollout_routed_experts = np.ascontiguousarray(arr.astype(np.int32, copy=True))| # mask previous off-policy generation for partial rollout | ||
| if args.partial_rollout and args.mask_offpolicy_in_partial_rollout and sample.response_length > 0: | ||
| sample.loss_mask = [0] * sample.response_length |
There was a problem hiding this comment.
The removal of the initial isinstance(sample, list) check breaks the public API of generate_and_rm, which is typed to accept Sample | list[Sample]. If a list of samples is passed directly to this function, it will now immediately crash with an AttributeError when trying to access sample.response_length on line 421.
We should restore the recursive list handling at the beginning of the function to maintain backward compatibility and adhere to the function's type signature.
if isinstance(sample, list):
return await asyncio.gather(
*[generate_and_rm(args, s, sampling_params, evaluation=evaluation) for s in sample]
)
# mask previous off-policy generation for partial rollout
if args.partial_rollout and args.mask_offpolicy_in_partial_rollout and sample.response_length > 0:
sample.loss_mask = [0] * sample.response_length| if new_response_tokens: | ||
| meta["output_token_logprobs"] = [ | ||
| [float(lp_val), int(tid)] for lp_val, tid in zip(new_response_log_probs, new_response_tokens, strict=True) | ||
| ] |
There was a problem hiding this comment.
Using strict=True in zip is risky here because the alignment helper _align_engine_tokens_and_logprobs was removed. If there is any discrepancy in the lengths of new_response_log_probs and new_response_tokens returned by the vLLM API, this will raise a ValueError and crash the rollout process.
To make the code more robust against API discrepancies, we should avoid strict=True or handle length mismatches gracefully.
| if new_response_tokens: | |
| meta["output_token_logprobs"] = [ | |
| [float(lp_val), int(tid)] for lp_val, tid in zip(new_response_log_probs, new_response_tokens, strict=True) | |
| ] | |
| if new_response_tokens: | |
| meta["output_token_logprobs"] = [ | |
| [float(lp_val), int(tid)] for lp_val, tid in zip(new_response_log_probs, new_response_tokens) | |
| ] |
vLLM's routed_experts is engine-level (--enable-return-routed-experts), not request-level. The field is ignored by /inference/v1/generate. SGLang's /generate does support it per-request, so this is a correct sglang→vllm translation divergence, not a mirror gap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
update_from_meta_info only reads finish_reason, spec_info, prefix_cache_info, and weight_version — output_token_logprobs is never consumed. Slime's sglang_rollout doesn't construct it either (SGLang returns it natively but update_from_meta_info ignores it there too). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The `e2e-test-unit` CI job runs `pytest tests/unit tests/utils` CPU-only (`# CPU/mock only -> no --gpus`), but the suite did not pass there: collection aborted and 38 tests failed. Causes and fixes (test-only; no source changes): - Stale helper tests (28): PR #178 rewrote vllm_rollout.py to mirror slime's sglang_rollout.py, inlining/removing the granular `_apply_vllm_routed_experts`, `_decode_vllm_routed_experts`, `_inference_generate_tokens_and_logprobs`, `_align_engine_tokens_and_logprobs`, `_vllm_meta_from_generate_choice`, `_router_worker_urls`, `_resume_vllm_workers`, `_base_dataset_prompt_ids`, `_warn_if_sampling_filters_may_affect_logprobs` helpers and the in-generate partial-continuation budget. Those tests called the now-removed private functions; the behavior is already covered by the surviving `test_generate_*` integration tests. Removed the obsolete tests. - Device probe (4): `get_vllm_cli_action_table()` rebuilds via `AsyncEngineArgs.add_cli_args`, which probes the accelerator and raises `RuntimeError: Failed to infer device type` on a GPU-less host. Added an autouse fixture in test_vllm_engine.py that seeds the (already-built) action-table cache so the rebuild is skipped, and mocked `torch.cuda.current_device` in the update_weight connect test. - Import-time sys.modules pollution: the update_weight tests ran `_install_stubs()` at module top level, leaving a fake `vllm` (no `.engine`) in sys.modules that broke COLLECTION of test_vllm_engine.py. Moved stub installation into the module-scoped fixtures with save/restore. The `ray` / `vllm_router` conftest stubs now defer to the real package via `importlib.util.find_spec`, so a bare non-package stub no longer leaks session-wide into sibling test packages (tests/utils). Result: `pytest tests/unit tests/utils` = 162 passed, 0 failed on CPU. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…181) The `e2e-test-unit` CI job runs `pytest tests/unit tests/utils` CPU-only (`# CPU/mock only -> no --gpus`), but the suite did not pass there: collection aborted and 38 tests failed. Causes and fixes (test-only; no source changes): - Stale helper tests (28): PR #178 rewrote vllm_rollout.py to mirror slime's sglang_rollout.py, inlining/removing the granular `_apply_vllm_routed_experts`, `_decode_vllm_routed_experts`, `_inference_generate_tokens_and_logprobs`, `_align_engine_tokens_and_logprobs`, `_vllm_meta_from_generate_choice`, `_router_worker_urls`, `_resume_vllm_workers`, `_base_dataset_prompt_ids`, `_warn_if_sampling_filters_may_affect_logprobs` helpers and the in-generate partial-continuation budget. Those tests called the now-removed private functions; the behavior is already covered by the surviving `test_generate_*` integration tests. Removed the obsolete tests. - Device probe (4): `get_vllm_cli_action_table()` rebuilds via `AsyncEngineArgs.add_cli_args`, which probes the accelerator and raises `RuntimeError: Failed to infer device type` on a GPU-less host. Added an autouse fixture in test_vllm_engine.py that seeds the (already-built) action-table cache so the rebuild is skipped, and mocked `torch.cuda.current_device` in the update_weight connect test. - Import-time sys.modules pollution: the update_weight tests ran `_install_stubs()` at module top level, leaving a fake `vllm` (no `.engine`) in sys.modules that broke COLLECTION of test_vllm_engine.py. Moved stub installation into the module-scoped fixtures with save/restore. The `ray` / `vllm_router` conftest stubs now defer to the real package via `importlib.util.find_spec`, so a bare non-package stub no longer leaks session-wide into sibling test packages (tests/utils). Result: `pytest tests/unit tests/utils` = 162 passed, 0 failed on CPU. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ce (#183) * fix(vllm_rollout): guard routed_experts value, not just key presence PR #178 mirrored slime's `if "routed_experts" in choice:` parse guard, but the vLLM engine's response shape differs from sglang's: sglang OMITS the key when routed experts are not requested (slime only sets `return_routed_experts=True` under `--use-rollout-routing-replay`), whereas the vLLM engine INCLUDES `routed_experts: null` in every choice. The bare `in` check therefore passes with a None value, and `choice["routed_experts"].encode("ascii")` raises `AttributeError: 'NoneType' object has no attribute 'encode'`, crashing `RolloutManager.generate()` for every MoE-model rollout that is not using rollout-routing-replay (e.g. test_qwen3.5_0.8B_gsm8k_short / _async_short). Fix: gate on the value with `choice.get("routed_experts") is not None`, which handles both key-absent and key-present-but-null and matches slime's effective behavior. Found by running the CI suite on `inferactinc/public:vime-latest` @ main: `ray::RolloutManager.generate()` ... vime/rollout/vllm_rollout.py:377 `AttributeError: 'NoneType' object has no attribute 'encode'`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Update vllm_rollout.py Signed-off-by: aoshen02 <aoshen@inferact.ai> --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…rtError) (#184) #178 ("align vllm_rollout.py with slime sglang_rollout.py") removed the private helpers `_align_engine_tokens_and_logprobs`, `_apply_vllm_routed_experts`, `_base_dataset_prompt_ids`, `_inference_generate_tokens_and_logprobs`, and `_vllm_meta_from_generate_choice` from `vime.rollout.vllm_rollout`, inlining their logic into `generate()`. But `vllm_streaming_rollout.py` still imported all five, so `mg_qwen3_4B_streaming` crashed at rollout time with: ImportError: cannot import name '_align_engine_tokens_and_logprobs' from 'vime.rollout.vllm_rollout' Mirror slime here: slime's `sglang_streaming_rollout` imports only `GenerateState` + `_prepare_prompt_ids` and inlines the finalize. This change does the same for vLLM — parse delta token_ids/logprobs, align logprobs to tokens, build meta from the terminal choice+usage, and decode routed_experts — all inlined exactly as the non-streaming `generate()` does (incl. the #183 `routed_experts is not None` guard). The one vLLM-specific bit, the partial- continuation budget helper `_base_dataset_prompt_ids`, is kept local to this file since it has no slime/`vllm_rollout` counterpart. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…181) The `e2e-test-unit` CI job runs `pytest tests/unit tests/utils` CPU-only (`# CPU/mock only -> no --gpus`), but the suite did not pass there: collection aborted and 38 tests failed. Causes and fixes (test-only; no source changes): - Stale helper tests (28): PR #178 rewrote vllm_rollout.py to mirror slime's sglang_rollout.py, inlining/removing the granular `_apply_vllm_routed_experts`, `_decode_vllm_routed_experts`, `_inference_generate_tokens_and_logprobs`, `_align_engine_tokens_and_logprobs`, `_vllm_meta_from_generate_choice`, `_router_worker_urls`, `_resume_vllm_workers`, `_base_dataset_prompt_ids`, `_warn_if_sampling_filters_may_affect_logprobs` helpers and the in-generate partial-continuation budget. Those tests called the now-removed private functions; the behavior is already covered by the surviving `test_generate_*` integration tests. Removed the obsolete tests. - Device probe (4): `get_vllm_cli_action_table()` rebuilds via `AsyncEngineArgs.add_cli_args`, which probes the accelerator and raises `RuntimeError: Failed to infer device type` on a GPU-less host. Added an autouse fixture in test_vllm_engine.py that seeds the (already-built) action-table cache so the rebuild is skipped, and mocked `torch.cuda.current_device` in the update_weight connect test. - Import-time sys.modules pollution: the update_weight tests ran `_install_stubs()` at module top level, leaving a fake `vllm` (no `.engine`) in sys.modules that broke COLLECTION of test_vllm_engine.py. Moved stub installation into the module-scoped fixtures with save/restore. The `ray` / `vllm_router` conftest stubs now defer to the real package via `importlib.util.find_spec`, so a bare non-package stub no longer leaks session-wide into sibling test packages (tests/utils). Result: `pytest tests/unit tests/utils` = 162 passed, 0 failed on CPU. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ce (#183) * fix(vllm_rollout): guard routed_experts value, not just key presence PR #178 mirrored slime's `if "routed_experts" in choice:` parse guard, but the vLLM engine's response shape differs from sglang's: sglang OMITS the key when routed experts are not requested (slime only sets `return_routed_experts=True` under `--use-rollout-routing-replay`), whereas the vLLM engine INCLUDES `routed_experts: null` in every choice. The bare `in` check therefore passes with a None value, and `choice["routed_experts"].encode("ascii")` raises `AttributeError: 'NoneType' object has no attribute 'encode'`, crashing `RolloutManager.generate()` for every MoE-model rollout that is not using rollout-routing-replay (e.g. test_qwen3.5_0.8B_gsm8k_short / _async_short). Fix: gate on the value with `choice.get("routed_experts") is not None`, which handles both key-absent and key-present-but-null and matches slime's effective behavior. Found by running the CI suite on `inferactinc/public:vime-latest` @ main: `ray::RolloutManager.generate()` ... vime/rollout/vllm_rollout.py:377 `AttributeError: 'NoneType' object has no attribute 'encode'`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Update vllm_rollout.py Signed-off-by: aoshen02 <aoshen@inferact.ai> --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…rtError) (#184) #178 ("align vllm_rollout.py with slime sglang_rollout.py") removed the private helpers `_align_engine_tokens_and_logprobs`, `_apply_vllm_routed_experts`, `_base_dataset_prompt_ids`, `_inference_generate_tokens_and_logprobs`, and `_vllm_meta_from_generate_choice` from `vime.rollout.vllm_rollout`, inlining their logic into `generate()`. But `vllm_streaming_rollout.py` still imported all five, so `mg_qwen3_4B_streaming` crashed at rollout time with: ImportError: cannot import name '_align_engine_tokens_and_logprobs' from 'vime.rollout.vllm_rollout' Mirror slime here: slime's `sglang_streaming_rollout` imports only `GenerateState` + `_prepare_prompt_ids` and inlines the finalize. This change does the same for vLLM — parse delta token_ids/logprobs, align logprobs to tokens, build meta from the terminal choice+usage, and decode routed_experts — all inlined exactly as the non-streaming `generate()` does (incl. the #183 `routed_experts is not None` guard). The one vLLM-specific bit, the partial- continuation budget helper `_base_dataset_prompt_ids`, is kept local to this file since it has no slime/`vllm_rollout` counterpart. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Removed functions (no slime equivalent)
Kept (vLLM-specific, no sglang equivalent)
sglang to vllm API divergences (intentionally NOT mirrored)
return_routed_experts: SGLang supports per-request, vLLM is engine-level — removed from payload
routed_experts location: SGLang returns in output meta_info, vLLM returns in output choices[0]
abort: SGLang uses POST /abort_request, vLLM uses POST /pause?mode=abort + POST /resume
multimodal: SGLang sends image_data directly, vLLM needs render+generate two-step
┌────────────────────────┬────────┬────────┬──────┐
│ metrics │ without R3 │ R3 │ diff │
├────────────────────────┼────────┼────────┼──────┤
│ eval/aime24 │ 63.3% │ 63.3% │ same │
├────────────────────────┼────────┼────────┼──────┤
│ logprob_abs_diff step0 │ 0.0214 │ 0.0135 │ -37% │
├────────────────────────┼────────┼────────┼──────┤
│ logprob_abs_diff step1 │ 0.0184 │ 0.0114 │ -38% │
├────────────────────────┼────────┼────────┼──────┤
│ logprob_abs_diff step2 │ 0.0160 │ 0.0097 │ -39% │
└────────────────────────┴────────┴────────┴──────┘