Skip to content

Add miles grpo test case - #1225

Open
littlemex wants to merge 7 commits into
awslabs:mainfrom
littlemex:add-miles-grpo-test-case
Open

Add miles grpo test case#1225
littlemex wants to merge 7 commits into
awslabs:mainfrom
littlemex:add-miles-grpo-test-case

Conversation

@littlemex

@littlemex littlemex commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Purpose

Relates to #1224

Adds a GRPO post-training test case for miles, a fork of SLIME for CUDA 13 and NVIDIA Blackwell, as a companion to the sibling 3.test_cases/pytorch/slime. It mirrors slime's layout, recipes, and env-var conventions so the two can be used the same way and compared directly. The full design, the slime-vs-miles comparison with source links, and the validated results live in the test case README; this description does not repeat them.

Changes

Adds 3.test_cases/pytorch/miles/ with the same shape as the sibling slime case:

  • miles.Dockerfile: the radixark/miles base pinned by digest, plus the AWS EFA layer.
  • kubernetes/: RayCluster, an in-cluster BuildKit Job, a data-prep Pod, and a CPU reward-service Deployment, all rendered with envsubst.
  • recipe/: dense and MoE GRPO recipes and their launcher; run-on-cluster.sh submits a recipe from inside the head pod.
  • reward_service/: a FastAPI CPU reward service and its reward_service.Dockerfile, deployed by kubernetes/reward-service.yaml for the disaggregated reward path.
  • scripts/: HF/Megatron checkpoint conversion and AIME-2024 evaluation.
  • requirements.txt, env_vars.*.example, and README.md.

The base-image adaptations, the slime-vs-miles differences with commit-pinned sources, and the known issues are documented in the README.

Test Plan

Environment:

  • AWS Service: Amazon EKS with KubeRay, EFA, and FSx for Lustre
  • Instance type: p5en.48xlarge, eight H200 141 GB GPUs per node
  • Number of nodes: 1 for the dense baseline, 2 for multi-node and MoE configs

Prerequisites and the full step-by-step are in the README Quick Start -- steps 0-6, through reward configuration, before training is launched at step 7.

Test commands:

# After Quick Start steps 0-6. Launch a recipe as a Ray job from inside the head pod:

# Dense Qwen3-4B, colocated -- the minimal check that the GRPO loop closes:
./run-on-cluster.sh --recipe run_grpo_qwen3_4b.sh

# Qwen3-30B-A3B MoE, colocated on 2 nodes; uncomment the ALTERNATE block in env_vars first:
./run-on-cluster.sh --recipe run_grpo_qwen3_30b_a3b.sh

# Follow the job:
ray job logs <submission-id> --address http://localhost:8265 --follow

What this verifies: the full GRPO control loop closes on real hardware -- rollout generation, rule-based reward, Megatron training steps, and Ray orchestration, with multi-node communication over EFA for the 2-node configs. For the dense colocated run, expect SUCCEEDED with rollout/raw_reward > 0 and rollout/repetition_frac = 0.

Test Results

Reward and repetition are the last scalar from the trainer's TensorBoard event files; each config was launched on 2x p5en.48xlarge. They show the loop closes and generation is healthy, not convergence.

Config reward repetition wall time
Qwen3-4B dense, colocated 1 node 0.531 0.0 ~13 min
Qwen3-4B dense, disaggregated 2 nodes 0.523 0.0 ~12 min
GLM-Z1-9B dense, colocated TP2 0.680 0.0 ~13 min
Qwen3-30B-A3B MoE, colocated pure EP, moe_tp=1 0.578 0.0 ~21 min
Qwen3-30B-A3B MoE, colocated pure TP, moe_ep=1 0.531 0.0 ~25 min
Qwen3-30B-A3B MoE, disaggregated pure EP 0.65 0.0 ~18 min
Qwen3-30B-A3B MoE, colocated combined moe_tp=2 x moe_ep=2 0.555 0.0 ~21 min
Qwen3-30B-A3B MoE, combined geometry, FlashInfer fusion on 0.0 0.56 ~20 min, degenerate output
Qwen2.5-72B dense, disaggregated OOM -- did not fit 16x H200

The two limitations -- the 30B MoE combined-geometry degeneration and the 72B memory ceiling -- are in the README Known Issues.

Directory Structure

3.test_cases/
└── pytorch/
    └── miles/
        ├── miles.Dockerfile
        ├── reward_service.Dockerfile
        ├── README.md
        ├── requirements.txt
        ├── env_vars.colocated.example
        ├── env_vars.disaggregated.example
        ├── run-on-cluster.sh
        ├── kubernetes/        # RayCluster, BuildKit Job, data-prep Pod, reward-service
        ├── recipe/            # dense and MoE GRPO recipes + launcher
        ├── reward_service/    # FastAPI CPU reward service
        └── scripts/           # checkpoint conversion, AIME-2024 evaluation

Matches the sibling slime case and the expected 3.test_cases/<framework>/<library>/ layout.

Checklist

  • I have read the contributing guidelines.
  • I am working against the latest main branch.
  • I have searched existing open and recently merged PRs to confirm this is not a duplicate.
  • The contribution is self-contained with documentation and scripts.
  • External dependencies are pinned to a specific version or tag (no latest): base image by digest, EFA installer and GDRCopy by version.
  • A README is included with prerequisites, instructions, and known issues.
  • New test cases follow the expected directory structure, matching the sibling slime case.

@littlemex

littlemex commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

TODO before merge

  • Squash and clean up commit history into logical units
  • Include actual output logs from test case runs (TensorBoard metrics, job completion output)
  • Address upstream issues: SGLang expert-parallel repetition bug blocks 30B MoE validation

@KeitaW
KeitaW marked this pull request as draft August 5, 2026 01:39
@KeitaW

KeitaW commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Thank you! Feel free to open the PR once you clear the remaining TODO items.

Take the upstream radixark/miles image (PyTorch 2.11 / CUDA 13, SGLang and Megatron-LM
prebuilt) by explicit digest and add only the AWS EFA layer. Rebuilding on an NGC base is not
viable (wheel ABI mismatch), so the base is consumed as-is; no floating :latest. A BuildKit Job
builds and pushes the image in-cluster for environments with no local Docker daemon.
Node placement, EFA device count, and worker replica count are env-driven, not hardcoded. The
Ray head co-locates on the GPU pool (CPU_NODE_ROLE defaults to GPU_NODE_ROLE) with a
nvidia.com/gpu toleration and runs num-gpus 0: miles control actors import Megatron even at
num-gpus 0 and need libcuda, so the head must be on a CUDA-capable node. A
karpenter.sh/do-not-disrupt annotation protects the head node from underutilized-consolidation.
Qwen3-4B (dense) and Qwen3-30B-A3B (MoE) GRPO recipes via a shared launcher. The MoE recipe
derives the SGLang rollout geometry; pure EP (moe_tp=1) and pure TP (moe_ep=1) train cleanly,
while combined moe_tp>1 and moe_ep>1 hits a FlashInfer allreduce-fusion bug (drops the moe-tp
reduce), so the recipe adds --sglang-enforce-disable-flashinfer-allreduce-fusion for that
geometry. run-on-cluster.sh optionally launches a recipe from inside the head pod so the operator
needs only kubectl (no local ray CLI, no port-forward); it does not deploy infra.
HF<->Megatron conversion and AIME-2024 evaluation. pass@k keyed per prompt; boxed-answer
extraction counts braces; requests run with bounded concurrency under a generation-sized timeout
so a queued timeout is not scored as an incorrect answer.
A CPU reward-service Deployment and image for the disaggregated overlay, mirroring the sibling
slime test case. Provided as a reference; not exercised in the recorded runs.
README with prerequisites, walkthrough (incl. in-cluster BuildKit and an optional head-pod launch
path), verification status, and known issues. The Ray head co-locates on the GPU pool (num-gpus 0)
because miles control actors import Megatron and need libcuda; a CPU-only head is not safe.
VERIFICATION_LOG records what ran, on what hardware, the metrics, the 30B MoE FlashInfer-fusion
root cause, and why the head must run on a CUDA-capable node.
@littlemex
littlemex force-pushed the add-miles-grpo-test-case branch from c284005 to ec59e11 Compare August 18, 2026 12:06
…nd internal narrative

- Rewrite README.md to mirror the sibling slime test case structure, fold in the
  measurements, remove development/campaign narrative and env-specific details,
  reduce parenthetical asides, and add a slime-vs-miles comparison with
  commit-pinned upstream links.
- Remove docs/ (EFA_2NODE, PORT_NOTES, VERIFICATION_LOG); content is folded into the
  README or dropped as internal. Recipes now reference the README.
- convert_checkpoint.sh: expose the conversion GPU count as CONVERT_NUM_GPUS
  (default 1, still overridable by --num-gpus) instead of a hardcoded value.
- env_vars examples: correct the SAVE_INTERVAL rationale (the end-of-run save
  succeeds; only reload and megatron2hf are untested), clarify node-role labels,
  and drop cluster-provisioner jargon.
@littlemex
littlemex force-pushed the add-miles-grpo-test-case branch from 4a6a333 to fea8451 Compare August 18, 2026 23:16
@littlemex

Copy link
Copy Markdown
Contributor Author

@KeitaW Sorry to bother you when you are busy, but could I ask you to review this? 😃

A few open questions for reviewers, kept out of the PR description on purpose:

  1. B300 validation. This test case is validated on H200 (p5en.48xlarge). Nothing hard-codes a GPU generation and the base image targets sm_103, so p6-b300.48xlarge is expected to work, but it has not been run. Should B300 validation block this PR, or is the "expected-compatible, not yet run" statement in the README acceptable to merge, with B300 tracked as a follow-up?

  2. miles version. This test case pins the radixark/miles image used for the validated runs. miles has since published a v0.1 release. Would you prefer this PR move to v0.1 before merge, or should the version bump and re-validation land as a separate follow-up so this PR ships the working reference first?

  3. Recipe parameterization. The recipes keep many fixed values in TRAIN_ARGS. The intent is a working reference that runs as-is rather than a fully generic launcher, so users edit the recipe for their own runs instead of every value being exposed as an environment variable. Does this match your expectation for a 3.test_cases entry, or would you prefer more of TRAIN_ARGS promoted to environment variables?

@littlemex littlemex changed the title [Don't Merge] Add miles grpo test case Add miles grpo test case Aug 19, 2026
@KeitaW
KeitaW marked this pull request as ready for review August 20, 2026 06:21
@KeitaW
KeitaW self-requested a review August 20, 2026 06:21

@KeitaW KeitaW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Batch 1/4: The three things that stop you

I ran this test case end to end on 2x p6-b300.48xlarge before reviewing it, and the framework side
is genuinely sound: GRPO rollout, deepscaler reward scoring, advantage computation, the Megatron
optimizer step and the colocated CUDA IPC weight sync all worked on Blackwell, which the README lists
as expected-compatible and unverified. Reward moved 0.4922 to 0.5156 to 0.5313 across three rollouts,
train/loss 0.01206, and the AIME eval scored 0.6125. That part is a real result and the recipes are
noticeably more careful than the sibling's.

Three things stopped me before I got there. All three are in the shipped configuration rather than in
miles, and all three are small fixes. A fourth thing I originally reported as a blocker, the
checkpoint save, turned out to be my filesystem and is withdrawn in Batch 2.

Comment on lines +11 to +13
rayStartParams:
dashboard-host: 0.0.0.0
num-gpus: '0'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Ray head cannot do the job the manifest places it on a GPU node to do

raycluster.yaml:76-83 pins the head to the GPU pool with an explicit reason: miles's Ray control
actors import Megatron and transformer_engine at startup even at num-gpus 0, and that import needs
libcuda.so.1. README Known Issue 3 and env_vars.colocated.example:76-82 say the same.

Fifty lines above, the same container sets NVIDIA_VISIBLE_DEVICES: void (:26-27), which makes the
NVIDIA container runtime behave like plain runc and inject no driver on any node. So the head is
pinned to the scarcest node type in the cluster and still has no libcuda. I confirmed that in the
running head pod: no libcuda.so.1 under either /usr/lib64 or /usr/lib/x86_64-linux-gnu,
ctypes.CDLL("libcuda.so.1") raising OSError, and no nvidia-smi.

That is not theoretical. RolloutManager is created with num_cpus=1, num_gpus=0 and no head
affinity (miles/ray/placement_group.py:187-190 in the pinned image), the head advertises CPUs, so
Ray is free to place it there. In my run it did, and the job died 46 seconds after submission with
OSError: libcuda.so.1 raised from transformer_engine/common/__init__.py:360, reached through
megatron.core. The 8-GPU placement group had already formed correctly on the worker.

One line got the shipped recipe running:

Suggested change
rayStartParams:
dashboard-host: 0.0.0.0
num-gpus: '0'
rayStartParams:
dashboard-host: 0.0.0.0
num-gpus: '0'
# Advertise zero CPUs so Ray cannot place control actors here. miles's RolloutManager is
# created with num_cpus=1, num_gpus=0 and imports megatron.core -> transformer_engine, which
# dlopens libcuda.so.1. NVIDIA_VISIBLE_DEVICES=void above means this container has no libcuda
# on any node, GPU node included. The job driver is already pinned to a worker by
# --entrypoint-resources '{"gpu_node": 0.001}'.
num-cpus: '0'

Please treat that as a baseline workaround rather than the whole repair, for three reasons:

  • It does not give the head a driver. It only removes the actors that need one. Any actor requesting
    exactly num_cpus=0 is still exposed, and MultiLoRAController
    (miles/ray/multi_lora/controller.py:59 and :116-122) is both num_cpus=0 and hard-pinned to
    the head
    by NodeAffinitySchedulingStrategy(soft=False). The shipped recipes never create it, so
    the baseline is safe, but the general statement "nothing CUDA-importing can land on the head" is
    not true.
  • miles has a --pin-rollout-manager-to-head flag, and EXTRA_TRAIN_ARGS is advertised as a
    supported extension point. A zero-CPU head makes that combination permanently unschedulable.
  • If the intent really is a CUDA-capable head, the direct fix is the env var, not the CPU count.
    NVIDIA_VISIBLE_DEVICES=none exposes driver capabilities without exposing a GPU device and would
    be worth validating against this import path. What cannot stand is void plus a comment and a
    nodeSelector that both assume a driver.

Whichever way you go, the GPU-pool nodeSelector and the three places that document the opposite
mechanism should change in the same commit, including the two recipe comments that attribute this to
a mooncake import (recipe/run_grpo_qwen3_4b.sh:296-299,
recipe/run_grpo_qwen3_30b_a3b.sh:251-252); mooncake's import is guarded by try/except ImportError
at miles/utils/object_store.py:10-20 and the failure I saw runs through transformer_engine.

Comment on lines +201 to +204
pip install -U "huggingface_hub[cli]"
huggingface-cli download Qwen/Qwen3-4B --local-dir /fsx/models/Qwen3-4B
huggingface-cli download --repo-type dataset zhuzilin/dapo-math-17k --local-dir /fsx/data/dapo-math-17k
huggingface-cli download --repo-type dataset zhuzilin/aime-2024 --local-dir /fsx/data/aime-2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README step 3 downloads nothing

pip install -U "huggingface_hub[cli]" is unpinned, so it installs huggingface_hub 1.x, where
huggingface-cli is a stub that prints a deprecation notice and exits 1 without downloading. Run
verbatim in the pod this PR creates, all three download commands fail and --local-dir is never
created. hf download worked first time and is what my run used. The repo's own
requirements.txt:35 pins huggingface-hub==1.18.0, also 1.x, so pinning as-is would not help.

Suggested change
pip install -U "huggingface_hub[cli]"
huggingface-cli download Qwen/Qwen3-4B --local-dir /fsx/models/Qwen3-4B
huggingface-cli download --repo-type dataset zhuzilin/dapo-math-17k --local-dir /fsx/data/dapo-math-17k
huggingface-cli download --repo-type dataset zhuzilin/aime-2024 --local-dir /fsx/data/aime-2024
pip install -U "huggingface_hub[cli]"
hf download Qwen/Qwen3-4B --local-dir /fsx/models/Qwen3-4B
hf download --repo-type dataset zhuzilin/dapo-math-17k --local-dir /fsx/data/dapo-math-17k
hf download --repo-type dataset zhuzilin/aime-2024 --local-dir /fsx/data/aime-2024

Inherited from ../slime/README.md:276-286, so the sibling has the same problem and is worth fixing
alongside.

Comment on lines +89 to +93
if [ "${COLOCATE}" = "true" ]; then
export WORKER_REPLICAS="${ACTOR_NUM_NODES}"
else
export WORKER_REPLICAS=$(( ( ACTOR_NUM_NODES*ACTOR_GPUS_PER_NODE + ROLLOUT_NUM_GPUS + 7 ) / 8 ))
fi

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the README to select the 30B config under-sizes the cluster

env_vars.colocated.example:89-93 derives WORKER_REPLICAS from ACTOR_NUM_NODES while the dense
defaults still say ACTOR_NUM_NODES=1. The ALTERNATE block that README:142 and :240 tell you to
uncomment sets ACTOR_NUM_NODES=2 at :177-193, about a hundred lines below, and the derivation
never re-runs. Sourcing the file after the documented edit:

ACTOR_NUM_NODES=2 ACTOR_GPUS_PER_NODE=8 ROLLOUT_NUM_GPUS=16  ->  WORKER_REPLICAS=1

so the manifest renders one 8-GPU worker while run_grpo_qwen3_30b_a3b.sh asks the actor for 2 x 8
and the rollout for 16 shared. The placement group cannot be satisfied. I did not launch the 30B
recipe, so the runtime symptom is inferred, but the 4B recipe's own comment
(recipe/run_grpo_qwen3_4b.sh:130-134) describes this shape as a wait rather than an error, and
env_vars.colocated.example:84-88 claims the derivation exists "so the two layouts cannot silently
disagree with the manifest".

Moving the derivation below the ALTERNATE block fixes this exact path. Shipping the MoE settings as a
complete env_vars.moe.example would remove the class of bug, since a late override block is what
makes ordering load-bearing.

Two related gaps, since they are what would have caught it. First, the 30B recipe kept only the
EP/engine divisibility check and dropped the 4B recipe's numeric validation, colocated-equality check
and cluster ceiling. Feeding both recipes the same bad inputs, with no cluster and no image:

perturbation run_grpo_qwen3_4b.sh run_grpo_qwen3_30b_a3b.sh
ACTOR_NUM_NODES=two refuses, "must be a non-negative integer" prints "Actor: two nodes x 8 GPUs" and submits
colocated actor 16 vs rollout 8 refuses submits
request exceeds CLUSTER_GPUS refuses, "more than CLUSTER_GPUS=8" submits
EP_SIZE=0 n/a exits 1 via a raw bash "division by 0", not the recipe's own error

Second, that ceiling guard is inactive in the shipped workflow anyway: neither example env file ever
sets CLUSTER_GPUS, so recipe/run_grpo_qwen3_4b.sh:115 and :135 never fire as configured.
Deriving CLUSTER_GPUS from WORKER_REPLICAS and the per-worker GPU count, after all overrides,
would give you the independent check the comment promises.


@KeitaW KeitaW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Batch 2/4: Checkpointing works; my earlier finding was an environment artifact

I originally raised the checkpoint save as a blocker, having seen it fail twice. I have since
isolated it, and the save path in this PR is fine. Recording the correction here because the
first version of this review was wrong about it and the author should not spend time on it.

The test: one node, one image, one recipe, NUM_ROLLOUT=1 and SAVE_INTERVAL=1 so the save fires
immediately, run back to back about fifteen minutes apart with only the destination changed.

CHECKPOINT_DIR free space reported save_model outcome
node-local disk, 28 TB free ample 8.3 s succeeded, 53 GB written, iter_0000000 and latest_checkpointed_iteration.txt present, Ray job succeeded
FSx for Lustre, 459 GB free, inodes 9% used apparently ample 72.2 s failed after 27 GB with [Errno 28] No space left on device on two ranks and unexpected pos 704 vs 598 on three

So Megatron's async torch_dist writer, miles/backends/megatron_utils/actor.py:653, and the whole
save path work correctly on B300. What failed both times was writing a 53 GB checkpoint to a shared
FSx for Lustre filesystem that was 90% full. The ENOSPC with 459 GB of aggregate free space and 9%
inode usage is the signature of per-OST exhaustion, where one stripe lands on a full target; I did
not confirm that with lfs df, so treat the mechanism as inference rather than measurement.

Consequences for the PR, all of them small:

  • env_vars.colocated.example:126-129 saying the end-of-run save "succeeded on every verification
    run" is consistent with what I now see. No change needed.
  • scripts/convert_checkpoint.sh:2 still cites "README Known Issues (save_model pickle-truncation)",
    an item the README rewrite deleted. That pointer should go, and this is the only checkpoint-related
    change I am asking for.
  • Worth one line in Troubleshooting: a 4B run writes about 53 GB per checkpoint, so CHECKPOINT_DIR
    needs real headroom, and on a shared Lustre filesystem near capacity the save can fail with
    ENOSPC even when df shows hundreds of gigabytes free.

Separately and still open: README step 8 presents megatron2hf as routine while
scripts/convert_checkpoint.sh:2 and env_vars.colocated.example:129 say the round trip is
untested. Those are three distinct states and worth stating separately: HF-to-Megatron input
conversion works (I ran it twice, 8m17s cold and about 2 min with a warm page cache, 7.5 GB written,
so the blanket "not executed on miles" banner is stale), the checkpoint save works, and
Megatron-to-HF back-conversion I still have not run.

@KeitaW KeitaW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Batch 3/4: Portability and correctness

None of these stopped my run, but each of them blocks a path this PR advertises.

run-on-cluster.sh can target the wrong namespace, or the wrong cluster

Two concrete wrong-target defects, not just missing diagnostics:

  • :26 resolves NAMESPACE from the caller's environment or falls back to default, and never
    reads it from ENV_FILE. A user who follows README step 1, edits NAMESPACE in env_vars, and
    then runs ./run-on-cluster.sh as README step 7 shows, searches default.
  • :49 selects the head with .items[0] on ray.io/node-type=head alone. In a namespace holding a
    slime cluster and a miles cluster, or two miles clusters, that can copy files into and submit the
    job to an unrelated head, and the :50-52 precondition then validates the wrong pod happily. The
    selector should include the KubeRay cluster label for miles-ray.

It also validates only the head, never the workers, so a Pending worker passes every precondition and
the recipe prints a banner describing a run that will never start.

The reward service turns failures into valid-looking rewards

For anyone using remote_rm, reward_service/app.py:170-174 converts every handler exception into
HTTP 200 with reward 0.0, and :154-156 reports healthy unconditionally. A persistently broken
scorer therefore trains to completion on a flat zero signal with a healthy-looking Deployment. Please
count and expose scoring errors and, past a threshold, fail /score or mark the pod unready; wiring
failures straight into the liveness probe would be worse, since one deterministic bad input would
crash-loop the pod mid-rollout.

Three more in the same file, all inherited from slime unchanged, all conditional on that path:

  • :51-54 types label as Optional[str], and pydantic 2 rejects an integer rather than coercing
    it. scripts/evaluate.sh:198-201 in this same PR documents that labels can be numeric in the
    JSONL. On the remote_rm path that 422 reaches raise_for_status()
    (miles/rollout/rm_hub/__init__.py:28) and batched_async_rm gathers without return_exceptions
    (:107-108), so one numeric label fails the whole rollout batch rather than one sample.
  • :136-141 treats any REWARD_BACKEND value other than math_verify as reward_model, so a typo
    silently downloads a large classifier and trains on the wrong reward. Accept the two documented
    values and raise otherwise.
  • :121-130 disables both math_verify timeouts because the sync handler runs in a threadpool. That
    removes the timeout rather than replacing it, so a pathological response can hold a worker thread
    indefinitely. /health is also a sync handler, so it does not stay healthy forever as I first
    wrote; it degrades once the threadpool is exhausted. A process pool with a real wall-clock timeout
    is the fix, and it gives you something to count for the paragraph above.

evaluate.sh grades by string equality while claiming otherwise

:8-9 says responses are scored "with the math reward function"; :197-208 extracts the \boxed{}
content and compares strings exactly, so mathematically equivalent answers count as wrong and the
reported accuracy understates the model. math_verify is already a dependency. Inherited from slime,
though miles improved the brace extraction and the numeric-label handling around it.

Also :93-96 polls readiness with curl -s, which exits 0 on a 4xx or 5xx response, so the script
can announce "SGLang server ready" against an unhealthy endpoint. curl -sf fixes it.


@KeitaW KeitaW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Batch 4/4: Documentation against the pinned image, strengths, sources

These are documentation accuracy items. They matter here more than usual, because the whole value of
a reference test case is that the reader can trust what it says about itself.

The documented commit is not the one in the pinned image

README:16 and :306, and every miles-side link in the comparison table, cite radixark/miles
fc04f66, committed 2026-08-18 per the GitHub API. git -C /root/miles log -1 inside the image that
miles.Dockerfile:25 pins by digest reports dec268f7, committed 2026-07-30, which is consistent
with the dev-202607310056 tag recorded alongside. So the table's source links describe code the
reader does not have. The digest line itself cannot prove which commit is inside, so this rests on my
container read rather than on the diff; if you can confirm from your side, either re-pin the digest
or correct the SHA and the links.

Same class, same caveat: the image reports Ray 2.56.1 against raycluster.yaml:90 and README:310
saying 2.55.1, SGLang 0.5.17.dev32+g3fe50ed against README:21 and :308 saying a v0.5.16 build, and
numpy 2.3.5 against a requirements.txt that argues for <2. requirements.txt is explicitly not
installed by the Dockerfile, so nothing is broken today, but its 17-line numpy rationale describes
slime's build and would downgrade a working image if anyone followed it. Retitle it as a reference
for another workflow or align it with what the image ships.

The Validation table cannot be reproduced from what ships

Of the nine rows, only the dense 4B colocated and the 30B pure-EP rows have complete settings that
ship. GLM-Z1-9B has no env block, no recipe and no MODEL_SCRIPT anywhere. The two disaggregated
rows have no shipped COLOCATE=false configuration, and env_vars.disaggregated.example is a
reward-tier overlay, not a GPU topology. The 30B disaggregated row is one the shipped recipe actively
refuses (run_grpo_qwen3_30b_a3b.sh:67-73 exits with "it is UNVERIFIED here"), which reads oddly next
to a validated result. The "fusion on" row cannot be recreated because :93-97 disables the fusion
unconditionally for that geometry. And no row states the NUM_ROLLOUT behind its wall time, which
makes the times uninterpretable given env_vars.colocated.example:122-124 says
train_iters == NUM_ROLLOUT and the shipped default is 100. README:117 also says every row was
launched on 2x p5en while row 1 is the single-node case.

The README rewrite left ten stale references behind

Five different kinds, so they need different fixes:

  • Dead section: env_vars.colocated.example:64 and :175 cite "README Verification Status", deleted.
  • Wrong item number: env_vars.colocated.example:164 and recipe/run_grpo_qwen3_30b_a3b.sh:11,
    :91, :96 cite "Known Issues item 2" for the MoE FlashInfer bug, which is now item 1. :96
    prints that to the operator's console at runtime.
  • Deleted content: scripts/convert_checkpoint.sh:2 and env_vars.disaggregated.example:63 cite
    Known Issues that no longer exist.
  • Stale subject: env_vars.colocated.example:129 cites "README Known Issues" for the untested
    reload; the section exists but no longer covers it.
  • Wrong step number: run-on-cluster.sh:50 says the RayCluster is README step 5; it is step 4.

Smaller things

  • README:182 calls the in-cluster build "a rootless BuildKit Job"; buildkit-job.yaml:22-23 runs the
    ordinary moby/buildkit:v0.18.2 with privileged: true, which a namespace on PodSecurity
    restricted will reject. The credential flow itself works: I created the ECR secret exactly as
    README:186-188 specifies, trailing slash and all, and BuildKit authenticated and pushed.
  • recipe/run_grpo_qwen3_30b_a3b.sh:224-226 warns that a removed --sglang-* flag is "silently
    accepted and does nothing". The parse is strict: miles/utils/arguments.py:2319 calls Megatron's
    parse_args without ignore_unknown_args, so Megatron-LM/megatron/training/arguments.py:99-102
    takes the parser.parse_args() branch and an unknown flag aborts. The advice is good, the
    consequence is inverted.
  • recipe/run_grpo_qwen3_4b.sh:80-85 validates integers with ^[0-9]+$, which admits 08. Bash then
    rejects it as invalid octal inside [[ ]] at :91-92, both tests evaluate false, and the recipe
    submits anyway. It is noisy rather than silent, but it still submits a malformed value, and
    :76-79 states the principle it violates: "A check that cannot run is worse than no check, because
    it reads as a pass." ^(0|[1-9][0-9]*)$ or $((10#${var})) closes it. CLUSTER_GPUS is used in
    arithmetic at :115 and :135 but omitted from that same guard list, so a non-numeric value dies
    with a raw unbound variable.
  • env_vars.colocated.example:17-19 computes AWS_REGION and AWS_ACCOUNT_ID through
    export VAR="$(aws ...)". In bash the exit status of that is export's, not the substitution's,
    so a failure is masked even under set -e. Sourcing the file inside the head pod, which
    run-on-cluster.sh does, printed aws: command not found twice and continued with empty values.
    Harmless today because only REGISTRY depends on them, but it is the wrong shape.
  • miles.Dockerfile:46-48 puts || true on a whole apt-get update && apt-get remove chain, so a
    failed update is swallowed too; :94 passes --no-verify to the EFA installer. Note the obvious
    build-time check does not work, since fi_info -p efa needs an EFA device a build node lacks;
    assert on the installed artifacts under /opt/amazon and keep the provider check for runtime.
  • Both recipes build --runtime-env-json by interpolating ${MODEL_SCRIPT} and ${TENSORBOARD_DIR}
    into a JSON string (run_grpo_qwen3_4b.sh:309-319, run_grpo_qwen3_30b_a3b.sh:262-272), so a
    quote or backslash in a path produces invalid JSON. Inherited from both slime recipes. jq -n --arg
    or python3 -c json.dumps is the safe form.
  • kubernetes/buildkit-job.yaml and kubernetes/raycluster.yaml carry no MIT-0 header; the other new
    source files do. The 30B recipe has one, displaced to :15-16 below its status banner. The README
    has none, but neither does the sibling's, so that one is consistent.
  • GDRCopy is installed at miles.Dockerfile:69-72 and listed in Software Versions, but nothing in the
    manifests declares /dev/gdrdrv. In my pod it was absent and NCCL logged
    NET/OFI Failed to initialize GDRCopy. A host device plugin could inject it without a manifest
    mount, so the accurate statement is that this sample does not declare it. Either document the full
    host-plus-pod setup or drop the userspace install and the version-table row.

Things that look great

  • The recipes are materially more defensive than the sibling's: strict COLOCATE spelling
    validation, integer guards, an engine-divides-pool check and an optional cluster ceiling, each with
    a comment explaining the failure it prevents. slime validates four variables; this validates
    twenty-six. I fed both recipes six bad configurations and the 4B one refused all six by name.
  • scripts/evaluate.sh fixes real bugs relative to slime: an EXIT trap that reaps the SGLang server
    on any exit path, bounded concurrency with a timeout sized for generation rather than for the queue,
    an error-fraction abort so a run that measured the timeout is not reported as accuracy, and a
    brace-counting \boxed{} extractor that handles nesting. Worth porting back into slime.
  • Pinning the base by sha256 digest with a documented digest-resolution recipe
    (miles.Dockerfile:12-26) goes beyond CONTRIBUTING's tag rule, and the stated reason, that the
    upstream deletes dated tags, is the right one.
  • recipe/launcher/grpo_launch.sh and its explanation of why MODEL_ARGS must be expanded inside the
    launcher rather than across the ray job submit boundary will save the next person an afternoon.
  • The EFA_PER_NODE note correctly frames a partial EFA request as a correctness problem rather than
    a throughput one, and correctly says an all-reduce smoke test will not catch it.
  • Dropping patches/ because the base image carries the equivalent patches is the right call and is
    explained.

Sources

  • End-to-end run of this branch at fea84519 on an Amazon EKS 1.35 cluster, 2x p6-b300.48xlarge
    (B300 SXM6, sm_103, driver 580.159.03, 16 vpc.amazonaws.com/efa allocatable per node), FSx for
    Lustre at /fsx. Qwen3-4B dense colocated on 8 GPUs through run-on-cluster.sh. 3.5 node-hours.
    Reviewer-reported, 2026-08-22; those logs no longer exist.
  • Checkpoint-destination isolation, same branch and image, one node, NUM_ROLLOUT=1
    SAVE_INTERVAL=1, two back-to-back runs differing only in CHECKPOINT_DIR: node-local emptyDir
    (28 TB free) succeeded with a 53 GB checkpoint in 8.3 s; FSx for Lustre (459 GB free, 9% inodes)
    failed after 27 GB with [Errno 28] and unexpected pos 704 vs 598 in 72.2 s. 0.8 node-hours.
    Verified live, 2026-08-25; logs at /mnt/fsx/ubuntu/workspace/pr1225-run2/logs/.
  • Image built from miles.Dockerfile at digest sha256:ca0bb593dd6f..., 19,179,530,094 bytes.
    Version and source facts (numpy 2.3.5, ray 2.56.1, sglang 0.5.17.dev32+g3fe50ed,
    /root/miles at dec268f7, fi_info 2.4.0amzn3.0, /usr/local/cuda*/compat absent) read from
    that image, which is still available locally. Verified live, 2026-08-22 and re-verified 2026-08-25.
  • miles source cited by file and line, read from the pinned image rather than upstream main:
    ray/placement_group.py:187-190, ray/multi_lora/controller.py:59 and :116-122,
    utils/ray_utils.py:14-21, utils/misc.py:211-213, train.py:120-124,
    utils/arguments.py:2319 and :2887-2895, rollout/rm_hub/__init__.py:26-29 and :107-108,
    utils/object_store.py:10-20. Verified live, 2026-08-25.
  • Deleted README content from git show ec59e119:3.test_cases/pytorch/miles/README.md
    (lines 158, 177, 536-543, 605-610). Verified live, 2026-08-25.
  • Recipe guard behaviour, executed by driving each recipe through ENV_FILE with one variable
    perturbed at a time, no cluster and no image: seven cases against the 4B recipe, five against the
    30B. Verified live, 2026-08-25.
  • ScoreRequest coercion under pydantic 2.13.4, constructed from the PR's own field declarations.
    Verified live, 2026-08-25.
  • fc04f66 commit date from gh api repos/radixark/miles/commits/fc04f66.... Verified live,
    2026-08-22.

@KeitaW KeitaW left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for the late reply @littlemex . Left few minor comments. Overall LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants