fix: complete #1985 sync — remove TIGHT_HOST_MEMORY + align test params - #218
Merged
Conversation
PR #214 synced #1985's TIGHT_DEVICE_MEMORY removal and batch-size shrinks but missed 10 files that used TIGHT_HOST_MEMORY (a different env-var guard with the same pre-#1985 pattern). Also aligns n-samples-per-prompt (8→4) and num-critic-only-steps (3→2) that were left at pre-#1985 values. Files: test_moonlight_16B_A3B{,_r3}, test_qwen3_{0.6B_parallel_check, 30B_A3B{,_r3}, 4B_ckpt, 4B_ppo{,_disaggregate,_train_critic_only}, 4B_streaming_partial_rollout}, test_qwen2.5_0.5B_ppo_critic_only_short Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request cleans up multiple test files by removing the TIGHT_HOST_MEMORY environment variable and simplifying the command-line arguments to use static values (such as --max-tokens-per-gpu and --use-kl-loss). Additionally, it reduces parameters like --num-critic-only-steps and --n-samples-per-prompt in some test configurations. There are no review comments, so I have no feedback to provide.
#214 and the preceding commit removed TIGHT_DEVICE_MEMORY and TIGHT_HOST_MEMORY definitions but left behind an extra blank line in the header area of 11 test files. Collapse double-blank to single-blank to match slime's spacing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
http_utils.py had get_rollout_num_engines() and run_router improvements that were pre-ported from slime #2016 (post-cutoff 44d29ee). Revert to match slime@44d29ee baseline so the diff stays clean; #2016 will be synced as a whole when its turn comes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ounterpart) This test was vime-specific (created in #49, not from slime) and was never registered in CI. Remove it to keep the test tree aligned with slime@44d29ee. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR #155 synced #1945's DistOptim checkpoint rider but missed the gpu_lock_exec.py change: replaces os.execvp with subprocess.Popen + proper signal forwarding (SIGINT/SIGTERM/SIGHUP) and fd_lock cleanup. This prevents orphaned GPU-holding processes when CI runners are cancelled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove TIGHT_HOST_MEMORY from 10 test files (missed in #214) - Align n-samples-per-prompt (8→4) and num-critic-only-steps (3→2) - Remove leftover blank lines from TIGHT_ variable removal (11 files) - Revert http_utils.py pre-ported #2016 content to slime@44d29ee baseline - Remove test_vllm_generate_endpoint.py (vime-native, no slime counterpart) - Port gpu_lock_exec subprocess signal forwarding (slime #1945) - Align remaining test params (over-sampling-batch-size, max-tokens-per-gpu) - Full test_qwen3_4B_ckpt.py #1945 sync (optimizer placement CLI) - Fix NUM_GPUS=0 placement in 7 CPU test files (was inside decorators) - NamedTemporaryFile multi-line formatting All pre-commit checks pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
youkaichao
approved these changes
Jun 9, 2026
1 task
aoshen02
added a commit
that referenced
this pull request
Jun 10, 2026
PR #218 changed num_rollout from 3→2 and rollout_batch_size from 8→4 but did not update the post-train assertion (expected 3*8=24, actual 2*4=8). Align with slime's upstream fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai>
4 tasks
aoshen02
added a commit
that referenced
this pull request
Jun 11, 2026
…NBUFFERED Traced 3 individual diffs back to their source slime PRs (以点带面) and synced all remaining changes from each: slime #1985 (make tests shorter): - Wrap NamedTemporaryFile across 3 lines in test_vllm_config_mixed_offload_ft.py - Remove extra blank line in test_vllm_config_mixed_offload.py (parameter shortening already synced in vime PR #218) slime #1920 (move fully_async example to main codebase): - Rewrite README.md to match upstream (qwen2.5-0.5B, not qwen3-4b) - Add run-qwen2.5-0.5B-fully_async.sh with proper vLLM translations - Delete run-qwen3-4b-fully_async.sh (upstream removed it) slime #1967 (fix PYTHONBUFFERED typo): - Fix PYTHONBUFFERED=16 → PYTHONUNBUFFERED=1 in 3 scripts: run-glm4.7-30B-A3B.sh, run-glm4.7-355B-A32B.sh, run-minimax-m2.sh (command_utils.py already fixed in prior sync) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai>
CalvinXKY
pushed a commit
that referenced
this pull request
Jun 11, 2026
* sync: complete slime #1920, #1967, #1985 — lint, fully_async, PYTHONUNBUFFERED Traced 3 individual diffs back to their source slime PRs (以点带面) and synced all remaining changes from each: slime #1985 (make tests shorter): - Wrap NamedTemporaryFile across 3 lines in test_vllm_config_mixed_offload_ft.py - Remove extra blank line in test_vllm_config_mixed_offload.py (parameter shortening already synced in vime PR #218) slime #1920 (move fully_async example to main codebase): - Rewrite README.md to match upstream (qwen2.5-0.5B, not qwen3-4b) - Add run-qwen2.5-0.5B-fully_async.sh with proper vLLM translations - Delete run-qwen3-4b-fully_async.sh (upstream removed it) slime #1967 (fix PYTHONBUFFERED typo): - Fix PYTHONBUFFERED=16 → PYTHONUNBUFFERED=1 in 3 scripts: run-glm4.7-30B-A3B.sh, run-glm4.7-355B-A32B.sh, run-minimax-m2.sh (command_utils.py already fixed in prior sync) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * fix: correct §2.4 flag translations in analyze_profile.py --enforce-eager → --vllm-enforce-eager --gpu-memory-utilization → --vllm-gpu-memory-utilization These are diagnostic hint strings, not CLI invocations, but should still use the canonical vime flag names (§2.4 translation rules). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * fix: mirror slime underline + revert group_id comment to rollout_id - analyze_profile.py: match slime's 30-char underline (was 28) - run_qwen36_35b_a3b_swe_8nodes.sh: revert group_id→rollout_id in comment (partial sync of slime #2013, target commit 44d29ee5) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * refactor(arguments): use FlexibleArgumentParser; revert dist.gather_object patch arguments.py (361→346 lines): - Import FlexibleArgumentParser from vllm.utils.argparse_utils; use it in vllm_parse_args() and get_vllm_cli_action_table() so vLLM's deprecated kwarg is handled natively on Python 3.12 without a shim - Remove _ARGPARSE_UNSUPPORTED_KWARGS + _strip_unsupported_argparse_kwargs - Remove import logging / logger (unused) reloadable_process_group.py: - Drop dist.gather_object monkey-patch (added by PR #22, never in slime; callers explicitly use all_gather_object to stay on the patched path) Architecture note: subprocess vllm serve kept intentionally — run_server() is not in vllm.__all__ and changes across minor versions; AReaL uses the same Popen pattern for the same reason. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * ci: revert aoshen02 CI additions; align J2 template with slime Reverts PR #26 (pre-commit gate) + PR #110 (e2e-test-unit). Syncs from slime: opened/reopened trigger types, --pull=always, pip deps (requests ray safetensors), cpu-unittest rename. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * docs(docker): translate Chinese to English in README Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * fix(ci): stub vllm_router in plugin_contracts for cpu-unittest Mirrors slime's with_sglang_router stub pattern: add with_vllm_router kwarg to install_stubs() and pass it from test_plugin_generate_contracts. vllm_rollout.py imports vllm_router at module level; without the stub the cpu-unittest job fails with ModuleNotFoundError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * fix(ci): add missing with_vllm_router=True stub to remaining plugin contracts test_plugin_rollout_contracts and test_plugin_path_loading_contracts both import vllm_rollout (which has bare `import vllm_router` at module level) but were not passing with_vllm_router=True to install_stubs — mirroring the same gap fixed in test_plugin_generate_contracts. Mirrors slime: all three tests pass with_sglang_router=True. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * docs(coding_agent_rl): align generate.py and README.md wording with slime - generate.py: drop backtick-wrapping around /inference/v1/generate in module docstring; collapse to one line matching slime style - README.md: condense rollout-max-*-len paragraph (remove verbose "sampling-params" / "generation length" verbiage, restore `max_tokens` inline like slime's `max_new_tokens` form) - README.md: trim vLLM response-structure detail (choices[0].token_ids / choices[0].logprobs.content[i].logprob) from token-out bullet — matches slime's abstraction level - README.md: add missing 3-line unit-test sentence after provenance paragraph (slime has it, vime was missing it) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * refactor(arguments): remove dist_ckpt_optim_fully_reshardable warning block Not present in slime; drop to maintain mirror parity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * refactor(rollout): remove redundant assertions and finalization re-assignments on_policy_distillation.py: drop two assertions not present in slime (len(plp)==len(sample.tokens) and len(t_log_prob)>=response_length). vllm_streaming_rollout.py: drop 9-line finalization block that re-set sample.tokens/response/response_length/rollout_log_probs/loss_mask after the streaming loop had already written the same values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> * style: apply black formatting to fix pre-commit CI Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: aoshen02 <aoshen@inferact.ai> --------- Signed-off-by: aoshen02 <aoshen@inferact.ai> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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
2 participants
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
PR #214 synced slime #1985's
TIGHT_DEVICE_MEMORYremoval but missed 10 files usingTIGHT_HOST_MEMORY(a different env-var guard with the same pattern). Also aligns parameter values left at pre-#1985 levels:TIGHT_HOST_MEMORYenv-var guard from 10 test files, resolve f-strings to non-tight defaultsn-samples-per-prompt 8→4in 3 files (test_qwen3_0.6B_parallel_check, test_qwen3_4B_ckpt, test_qwen3_4B_ppo_train_critic_only)num-critic-only-steps 3→2in test_qwen2.5_0.5B_ppo_critic_only_short + test_qwen3_4B_ppo_train_critic_onlyAll values now match slime@44d29ee (post-#1985).
Test plan
TIGHT_env vars remain in test files🤖 Generated with Claude Code