refactor(rl): centralize training entrypoints around env.sh - #17
Conversation
- add shared RL buffer server and slime generator launch scripts - move geo3k_vl runtime knobs into env.sh - remove geo3k_vl-specific launcher wrappers - update RL README files with env-driven startup flow
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (10)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesRL runtime and evaluation changes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Env as env.sh
participant Generator as run_slime_generator.sh
participant Ray
participant Train as TRAIN_ENTRYPOINT
participant Buffer as run_buffer_server.sh
participant Server as buffer_server.py
Env->>Generator: load runtime configuration
Generator->>Ray: start Ray head and submit training job
Ray->>Train: execute composed training arguments
Env->>Buffer: load runtime configuration
Buffer->>Server: start buffer server process
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rl/run_buffer_server.sh (1)
9-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the duplicated bootstrap into a shared sourced helper. Both launchers duplicate the
--envargument parsing/env-loading (L9–37) and therequire_dir/require_file(+is_truein the generator) helpers. This duplication already caused the divergent.current_runbehavior flagged above; consolidating reduces future drift.
rl/run_buffer_server.sh#L9-L55: move arg parsing and therequire_*helpers into a commonrl/_launcher_common.shandsourceit.rl/run_slime_generator.sh#L9-L62: source the same helper for arg parsing,require_*, andis_true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rl/run_buffer_server.sh` around lines 9 - 55, The launcher bootstrap logic is duplicated across both scripts and must be centralized. Create and source rl/_launcher_common.sh from rl/run_buffer_server.sh lines 9-55 and rl/run_slime_generator.sh lines 9-62, moving shared --env parsing, environment loading, require_dir, and require_file there; also move is_true from rl/run_slime_generator.sh into the helper. Remove the duplicated implementations from both launchers while preserving their existing argument validation and environment behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rl/examples/geo3k_vl/env.sh`:
- Line 23: Rename the exported variable from AIEVOBOC_MULTIPLIER to
AIEVOBOX_MULTIPLIER, preserving its existing default value and
environment-variable fallback behavior.
In `@rl/README.md`:
- Line 15: Document the PYTHON_BIN and RAY_BIN defaults or explicitly point
readers to the example env.sh files in rl/README.md lines 15-15 and 95-122, and
apply the same update in rl/README_CN.md lines 15-15 and 95-122 so both language
versions explain where these launcher binaries are configured.
In `@rl/run_slime_generator.sh`:
- Line 279: Update the DAPO_filter entry used to build RUNTIME_ENV_JSON so it
safely handles an unset variable under set -u, matching the neighboring
environment keys’ defaulting behavior; otherwise ensure DAPO_filter is reliably
exported before this block.
- Around line 218-232: Update the WANDB_TEAM check and
WANDB_ALWAYS_USE_TRAIN_STEP condition in the WANDB_ARGS construction to use
empty/default-safe expansions under set -u. Preserve the existing optional team
and train-step flag behavior when these variables are unset or empty, while
retaining their current behavior when configured.
---
Nitpick comments:
In `@rl/run_buffer_server.sh`:
- Around line 9-55: The launcher bootstrap logic is duplicated across both
scripts and must be centralized. Create and source rl/_launcher_common.sh from
rl/run_buffer_server.sh lines 9-55 and rl/run_slime_generator.sh lines 9-62,
moving shared --env parsing, environment loading, require_dir, and require_file
there; also move is_true from rl/run_slime_generator.sh into the helper. Remove
the duplicated implementations from both launchers while preserving their
existing argument validation and environment behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1910b7ae-f82f-41ed-a929-b99210594c8b
📒 Files selected for processing (9)
env/geo3k/geo3k_rule_eval.yamlrl/README.mdrl/README_CN.mdrl/buffer_server.pyrl/examples/geo3k_vl/env.shrl/examples/geo3k_vl/run_buffer_server.shrl/examples/geo3k_vl/run_slime_generator.shrl/run_buffer_server.shrl/run_slime_generator.sh
💤 Files with no reviewable changes (4)
- rl/examples/geo3k_vl/run_slime_generator.sh
- env/geo3k/geo3k_rule_eval.yaml
- rl/examples/geo3k_vl/run_buffer_server.sh
- rl/buffer_server.py
| export AIEVOBOX_MESSAGE_CUT="${AIEVOBOX_MESSAGE_CUT:-0}" | ||
| export AIEVOBOX_ENABLE_EVALUATION="${AIEVOBOX_ENABLE_EVALUATION:-1}" | ||
| export AIEVOBOX_POOL_SIZE="${AIEVOBOX_POOL_SIZE:-16}" | ||
| export AIEVOBOC_MULTIPLIER="${AIEVOBOC_MULTIPLIER:-1.2}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --hidden -g '!*.lock' 'AIEVOBO[CX]_MULTIPLIER'Repository: AI45Lab/SAfactory
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- env.sh around line 23 ---\n'
sed -n '1,80p' rl/examples/geo3k_vl/env.sh
printf '\n--- search for AIEVO* variables ---\n'
rg -n --hidden -g '!*.lock' 'AIEVO[A-Z_]*' rl || true
printf '\n--- search for BOX/BOC variants in repo ---\n'
rg -n --hidden -g '!*.lock' 'AIEVOBO[CX]_MULTIPLIER|AIEVOBOX_MULTIPLIER|AIEVOBOC_MULTIPLIER' . || trueRepository: AI45Lab/SAfactory
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n rl/examples/geo3k_vl/env.sh | sed -n '1,120p'Repository: AI45Lab/SAfactory
Length of output: 7121
Rename this variable to AIEVOBOX_MULTIPLIER.
AIEVOBOC_MULTIPLIER is inconsistent with the surrounding AIEVOBOX_* names and exports the wrong environment variable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rl/examples/geo3k_vl/env.sh` at line 23, Rename the exported variable from
AIEVOBOC_MULTIPLIER to AIEVOBOX_MULTIPLIER, preserving its existing default
value and environment-variable fallback behavior.
| | `rl/slime_generator.py` | Slime rollout function. It starts the LLM proxy, fetches trajectory groups, builds masks/rewards, and returns training samples. | | ||
|
|
||
| ### 2. Start Slime training (Terminal 1) | ||
| Slime, Megatron-LM, and SGLang must be installed separately in the runtime used by `PYTHON_BIN` and `RAY_BIN`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant README sections with line numbers.
for f in rl/README.md rl/README_CN.md; do
echo "===== $f ====="
nl -ba "$f" | sed -n '1,170p'
echo
done
echo "===== variable references in rl/ ====="
rg -n --no-heading '\b(PYTHON_BIN|RAY_BIN|NUM_ROLLOUT|ACTOR_NUM_NODES|TP_SIZE|epoch|EPOCH|ROLLOUT_GROUP_BATCH_SIZE|RL_GROUP_SIZE|RL_GLOBAL_BATCH_SIZE)\b' rlRepository: AI45Lab/SAfactory
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all occurrences of the suspected variables and related runtime config names.
rg -n --no-heading '\b(PYTHON_BIN|RAY_BIN|NUM_ROLLOUT|ACTOR_NUM_NODES|TP_SIZE|NUM_EPOCHS|EPOCHS|epoch)\b' .Repository: AI45Lab/SAfactory
Length of output: 5370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in rl/README.md rl/README_CN.md; do
echo "===== $f (1-70) ====="
sed -n '1,70p' "$f"
echo
doneRepository: AI45Lab/SAfactory
Length of output: 4632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in rl/README.md rl/README_CN.md docs/rl-training.md docs/rl-training_CN.md docs/configuration.md docs/configuration_CN.md; do
echo "===== $f ====="
sed -n '35,115p' "$f"
echo
doneRepository: AI45Lab/SAfactory
Length of output: 22198
Document PYTHON_BIN and RAY_BIN or point to the example env.sh. The launcher note depends on those binaries, but the README itself only says each example uses rl/examples/<env>/env.sh; adding the binary defaults there would close the gap in both English and Chinese.
📍 Affects 2 files
rl/README.md#L15-L15(this comment)rl/README.md#L95-L122rl/README_CN.md#L15-L15rl/README_CN.md#L95-L122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rl/README.md` at line 15, Document the PYTHON_BIN and RAY_BIN defaults or
explicitly point readers to the example env.sh files in rl/README.md lines 15-15
and 95-122, and apply the same update in rl/README_CN.md lines 15-15 and 95-122
so both language versions explain where these launcher binaries are configured.
| WANDB_ARGS=() | ||
| if is_true "${USE_WANDB}"; then | ||
| WANDB_ARGS=( | ||
| --use-wandb | ||
| --wandb-project "${WANDB_PROJECT}" | ||
| --wandb-group "${WANDB_GROUP}" | ||
| --wandb-dir "${WANDB_DIR}" | ||
| ) | ||
| if [[ -n "${WANDB_TEAM}" ]]; then | ||
| WANDB_ARGS+=(--wandb-team "${WANDB_TEAM}") | ||
| fi | ||
| if is_true "${WANDB_ALWAYS_USE_TRAIN_STEP}"; then | ||
| WANDB_ARGS+=(--wandb-always-use-train-step) | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
set -u crash risk: WANDB_TEAM / WANDB_ALWAYS_USE_TRAIN_STEP lack :- defaults.
With set -u (Line 3), when USE_WANDB is truthy but WANDB_TEAM (Line 226) or WANDB_ALWAYS_USE_TRAIN_STEP (Line 229) is unset, the bare ${WANDB_TEAM} / ${WANDB_ALWAYS_USE_TRAIN_STEP} expansion aborts the script with "unbound variable". The [[ -n ... ]] guard on WANDB_TEAM implies it is intended to be optionally empty, so it can legitimately be unset. Sibling flags (e.g. USE_OPD L200, SGLANG_ENABLE_MIXED_CHUNK L254) already use :- defaults.
Proposed fix
- if [[ -n "${WANDB_TEAM}" ]]; then
+ if [[ -n "${WANDB_TEAM:-}" ]]; then
WANDB_ARGS+=(--wandb-team "${WANDB_TEAM}")
fi
- if is_true "${WANDB_ALWAYS_USE_TRAIN_STEP}"; then
+ if is_true "${WANDB_ALWAYS_USE_TRAIN_STEP:-false}"; then
WANDB_ARGS+=(--wandb-always-use-train-step)
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| WANDB_ARGS=() | |
| if is_true "${USE_WANDB}"; then | |
| WANDB_ARGS=( | |
| --use-wandb | |
| --wandb-project "${WANDB_PROJECT}" | |
| --wandb-group "${WANDB_GROUP}" | |
| --wandb-dir "${WANDB_DIR}" | |
| ) | |
| if [[ -n "${WANDB_TEAM}" ]]; then | |
| WANDB_ARGS+=(--wandb-team "${WANDB_TEAM}") | |
| fi | |
| if is_true "${WANDB_ALWAYS_USE_TRAIN_STEP}"; then | |
| WANDB_ARGS+=(--wandb-always-use-train-step) | |
| fi | |
| fi | |
| WANDB_ARGS=() | |
| if is_true "${USE_WANDB}"; then | |
| WANDB_ARGS=( | |
| --use-wandb | |
| --wandb-project "${WANDB_PROJECT}" | |
| --wandb-group "${WANDB_GROUP}" | |
| --wandb-dir "${WANDB_DIR}" | |
| ) | |
| if [[ -n "${WANDB_TEAM:-}" ]]; then | |
| WANDB_ARGS+=(--wandb-team "${WANDB_TEAM}") | |
| fi | |
| if is_true "${WANDB_ALWAYS_USE_TRAIN_STEP:-false}"; then | |
| WANDB_ARGS+=(--wandb-always-use-train-step) | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rl/run_slime_generator.sh` around lines 218 - 232, Update the WANDB_TEAM
check and WANDB_ALWAYS_USE_TRAIN_STEP condition in the WANDB_ARGS construction
to use empty/default-safe expansions under set -u. Preserve the existing
optional team and train-step flag behavior when these variables are unset or
empty, while retaining their current behavior when configured.
| \"ROLLOUT_BUFFER_URL\": \"${ROLLOUT_BUFFER_URL}\",\ | ||
| \"SLIME_ROLLBUF_RESTART_TRAINING\": \"${SLIME_ROLLBUF_RESTART_TRAINING}\",\ | ||
| \"RL_OFF_BY_N\": \"${RL_OFF_BY_N}\",\ | ||
| \"DAPO_filter\": \"${DAPO_filter}\",\ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
DAPO_filter referenced without a default under set -u.
${DAPO_filter} has no :- fallback, unlike neighboring keys such as AIEVOBOX_DEBUG_CACHE_PROCESSOR_COMPARE (L281) and TEACHER_URL (L282). If env.sh/MODEL_SCRIPT does not define it, building RUNTIME_ENV_JSON aborts with "unbound variable" (Shellcheck SC2154). Add a default or confirm it is always exported before this point.
Proposed fix
- \"DAPO_filter\": \"${DAPO_filter}\",\
+ \"DAPO_filter\": \"${DAPO_filter:-}\",\📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| \"DAPO_filter\": \"${DAPO_filter}\",\ | |
| \"DAPO_filter\": \"${DAPO_filter:-}\",\ |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 279-279: DAPO_filter is referenced but not assigned.
(SC2154)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rl/run_slime_generator.sh` at line 279, Update the DAPO_filter entry used to
build RUNTIME_ENV_JSON so it safely handles an unset variable under set -u,
matching the neighboring environment keys’ defaulting behavior; otherwise ensure
DAPO_filter is reliably exported before this block.
Source: Linters/SAST tools
Summary by CodeRabbit
New Features
Documentation
Refactor