feat(verl): GRPO + Megatron/FSDP LoRA recipe on EKS with Hydra config and eval pipeline

[mvinci12]

Purpose

Adds a GRPO (Group Relative Policy Optimization) test case for verl with verifiable math and
code rewards, running on Ray/KubeRay over EKS. Covers both the Megatron (via Megatron-Bridge)
and FSDP backends, LoRA from 7B up to 235B MoE, and full fine-tuning.

Opened as a draft for placement and scope feedback before review.

Changes

New directory 3.test_cases/pytorch/verl/grpo-megatron-lora/ (85 files):

• conf/ — Hydra config groups (backend, cluster, data, lora, model, profiling,
  sandbox, tracking). This is the source of truth for all training parameters.
• scripts/ — submit_training.py entry point (Hydra → verl CLI → ray job submit), LoRA
  adapter merge with a logit-parity gate, evaluation drivers, and unit tests.
• data/ — dataset prep, difficulty filtering, multi-dataset mixing, validation-holdout builder.
• models/ — HuggingFace → FSx model staging helpers.
• kubernetes/ — vLLM eval server, lm-evaluation-harness jobs with custom pass@k task
  definitions, sandbox-fusion code-reward service, FSx utility pod.
• lustre/ — FSx for Lustre PV / PVC / StorageClass templates.
• docs/ — cluster topology, configuration reference, eval walkthrough, measured results,
  profiling, troubleshooting.
• Dockerfile + build-push.sh — training image (EFA + Megatron-Bridge) and ECR push.

Nothing is installed locally: verl is installed on the Ray nodes at job submission time via
scripts/runtime_env.yaml. Training parameters live in conf/; env_vars carries
infrastructure only (AWS, Kubernetes, secrets, NCCL) and is gitignored, with
env_vars.example as the tracked template.

All external dependencies are pinned — no latest:

Dependency	Pin
verl	v0.8.0
megatron-core	0.18.0
Megatron-Bridge	v0.5.0
Ray	2.53.0
vLLM	0.20.2
base image	verlai/verl:vllm020.dev2
sandbox-fusion	volcengine/sandbox-fusion:server-20250609

Test Plan

Environment:

• AWS Service: Amazon EKS (validated on SageMaker HyperPod EKS; any EKS cluster with GPU
  nodes, KubeRay and a shared filesystem works)
• Instance type: p6-b200.48xlarge (NVIDIA B200, 8×183 GB per node)
• Number of nodes: 6 (48 B200 GPUs); a 1-node config is included for quick validation
• Storage: FSx for Lustre via PVC
• KubeRay: v1.3.0
• Model: Qwen3-235B-A22B, GRPO + LoRA (rank 128 / alpha 256), Megatron backend

Test commands:

cp env_vars.example env_vars && vim env_vars   # infra only, not training params
source env_vars
./build-push.sh

envsubst < lustre/storageclass.yaml            | kubectl apply -f -
envsubst < lustre/pv.yaml                      | kubectl apply -f -
envsubst < lustre/pvc.yaml                     | kubectl apply -f -
envsubst < kubernetes/sandbox-fusion.yaml      | kubectl apply -f -
envsubst < kubernetes/fsx-utils.yaml           | kubectl apply -f -

./data/submit_data_prep.sh --datasets eurus apps taco codecontests \
    --output-dir /fsx/data/verl/data
./models/submit_download.sh Qwen/Qwen3-8B /fsx/data/verl/models/Qwen3-8B

kubectl port-forward -n "$KUBE_NAMESPACE" svc/<ray-head-svc> 8265:8265
export RAY_ADDRESS="http://localhost:8265"

python3 scripts/submit_training.py --cfg job                    # dry run
python3 scripts/submit_training.py cluster=p6-b200-1node tracking=console
python3 scripts/submit_training.py backend=megatron model=qwen3-235b

Repo checks:

ruff check --line-length 120 .          # clean
python3 -m py_compile $(find . -name '*.py')
for s in $(find . -name '*.sh'); do bash -n "$s"; done
python3 -m pytest scripts/test_reward_routing.py scripts/test_merge_provenance.py \
                  scripts/test_lmeval_utils.py

Test Results

Four training runs plus the measurement work around them; full detail and the negative
results are in docs/results.md.

Public, contamination-cleared, deterministic math benchmarks (greedy decoding, so no sampling
variance) — both trained arms beat the untrained base on GSM8K:

Arm	GSM8K (n=1319, 5-shot, strict-match)	MATH-500 scoreable subset (n=472)
base	0.8469	0.5212
attention-only	0.8719 — +0.0250 (2.83σ, p=0.006)	0.5297 — +0.0085 (0.52σ, null)
expert-FFN	0.8704 — +0.0235 (2.60σ, p=0.012)	0.5551 — +0.0339 (2.11σ, p=0.048)

The pre-registered primary (base → expert-FFN step-150, GSM8K strict-match, win = ≥ +0.02 at
≥ 1.96σ) passes at +0.0235 / 2.60σ. Both arms clear the ~2.50σ Bonferroni bar on GSM8K. On
code, MBPP pass@4 is +0.0300 (2.85σ).

Reported alongside, since the docs state it plainly: the Run-4 hypothesis that moving LoRA to
the expert FFNs beats attention-only placement is not supported — 6/6 matched internal
nulls and a well-powered MBPP null, at +11.4% step time (7.10σ) and a 105.7 GB adapter.
docs/results.md also records that ~93% of the base → step-150 gain landed by step 50.

Directory Structure

3.test_cases/
└── pytorch/
    └── verl/
        └── grpo-megatron-lora/
            ├── Dockerfile
            ├── build-push.sh
            ├── README.md
            ├── AGENTS.md
            ├── env_vars.example
            ├── conf/            # Hydra config groups (training params)
            ├── scripts/         # submit_training.py, merge + parity gate, eval, tests
            ├── data/            # prep, filtering, mixing, val holdout
            ├── models/          # HF -> FSx staging
            ├── kubernetes/      # vLLM eval, lm-eval jobs, sandbox-fusion, FSx utils
            ├── lustre/          # FSx PV / PVC / StorageClass
            └── docs/            # cluster, configuration, eval, results, profiling, troubleshooting

This is a Kubernetes-only test case, so there is no slurm/ subdirectory.

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).
• A README is included or updated with prerequisites, instructions, and known issues.
• New test cases follow the expected directory structure.

[KeitaW]

Review Batch 1/6 — Scope, size and what doesn't need to ship

Placement: this sits beside verl/kubernetes/rlvr, which is already GRPO-on-EKS with verl + Ray

3.test_cases/pytorch/verl/ currently uses a verl/<platform>/<recipe>/ layout —
verl/kubernetes/rlvr/ and verl/hyperpod-eks/rlvr/. This PR adds verl/grpo-megatron-lora/ at
the platform level, so the tree ends up with two different nesting depths under one framework.

The overlap is more than cosmetic. verl/kubernetes/rlvr/README.md already describes GRPO (and
DAPO) with verl on an EKS/HyperPod-EKS Ray cluster, and verl/kubernetes/rlvr/recipe/ already
ships run_grpo_configurable.sh and run_qwen3-235b_megatron_96gb.sh — a Qwen3-235B Megatron
recipe. Two sibling directories teaching the same workload on the same platform with different
scaffolding is the outcome CONTRIBUTING.md's self-containment guidance is trying to avoid, and
it doubles the maintenance surface for the next verl bump.

Could you say which of these you intend?

• Merge into the existing recipe — add the Megatron/LoRA + Hydra path under
  verl/kubernetes/rlvr/, reusing its setup/ (KubeRay install, RayCluster, env_vars).
• New sibling recipe at the same depth — verl/kubernetes/grpo-megatron-lora/, with the
  README stating plainly what it does that rlvr/ does not (LoRA at 235B MoE, the eval pipeline).

Either is fine; the current placement is the one that isn't.

Cluster and shared-storage provisioning belongs in 1.architectures, not in a test case

A test case consumes a cluster; it does not build one. CONTRIBUTING.md permits the dependency
explicitly — "Dependencies are accepted for the network architecture (VPC templates) and if they
are within the repository" — and the established test cases follow it: FSDP/kubernetes/README.md:10
and dreamzero/README.md:74 state the cluster prerequisite and link 1.architectures;
megatron/nemo1.0/EKS/README.md:8 does the same for FSx.

Please drop both:

• lustre/{storageclass,pv,pvc}.yaml — FSx provisioning, owned by
  1.architectures/7.sagemaker-hyperpod-eks/terraform-modules/hyperpod-eks-tf/modules/fsx_lustre.
• conf/cluster/p6-b200-{1,6}node.yaml (and the cluster config group) — hardware description.
  The handful of values submit_training.py actually reads (num_nodes, gpus_per_node, the
  offload flags, gpu_memory_utilization, agent_num_workers, fsx_home) fold into
  conf/config.yaml as ordinary training parameters.

Then state the requirement in the README Prerequisites — node count, instance type, EFA, and an
FSx for Lustre PVC — and link 1.architectures. Nothing more than that.

Six near-identical model-download scripts

models/download_qwen{3_8b,3_235b,3_30b_a3b,3_coder_next,25_72b,25_coder_7b}.sh are 102 lines
each and pairwise identical apart from two variables:

$ diff models/download_qwen3_8b.sh models/download_qwen3_235b.sh
5c5   # Model Download Script for Qwen/Qwen3-8B      -> .../Qwen3-235B-A22B
19,20c19,20
< HF_MODEL_ID="Qwen/Qwen3-8B"                        -> "Qwen/Qwen3-235B-A22B"
< MODEL_NAME="Qwen3-8B"                              -> "Qwen3-235B-A22B"

That is ~510 lines where ~110 would do, and six places to fix every future bug (three of the
findings below apply to all six at once). models/submit_download.sh <HF_ID> <DEST> is already
the generic entry point, and it is the one the README quickstart uses. Please keep one
parameterised script and drop the five duplicates, or reduce them to a table of model IDs.

[KeitaW]

Files that don't need to be in the repo

A handful of committed files carry no consumer. Each is individually small; together they are a
meaningful slice of the 85.

• LICENSE (this file) — no other directory under 3.test_cases/ ships its own LICENSE
  (find 3.test_cases -name LICENSE returns nothing on main). The repo-root LICENSE already
  covers the tree, and the per-file SPDX-License-Identifier: MIT-0 headers carry the per-file
  statement. Please drop it.
• docs/parallelism-strategies.svg — 54 KB / 670 lines, and grep -rn parallelism-strategies
  across the contribution matches exactly one line: the directory listing inside AGENTS.md. No
  README or doc embeds it. Either reference it from docs/configuration.md where the parallelism
  discussion lives, or drop it.
• kubernetes/lmeval-tasks/bigcodebench_p4.yaml — 59 lines, and AGENTS.md:237 says of it:
  "authored but NOT USABLE." No script references it. A committed artifact whose own docs declare
  it non-functional is a trap for the next reader; the finding (lm-eval's reliability_guard()
  is the wrong executor for BigCodeBench) is well worth keeping in docs/results.md, but the YAML
  isn't.
• kubernetes/eval-mlflow-log-job.yaml — hardcodes --steps 50,350,750,
  --baseline qwen3-235b-base, --run-prefix qwen3-235b- and output filenames, requires an
  undocumented MLFLOW_RUN_ID, and runs /fsx/data/verl/eval_scripts/compare_eval_results.py,
  a path nothing in this PR stages. It is one operator's run, not a reusable asset. [confirmed]
• lustre/{storageclass,pv,pvc}.yaml and conf/cluster/*.yaml — cluster and filesystem
  provisioning, which is 1.architectures' layer (see the layering note in this batch's summary).
  Drop both; state the cluster and FSx requirement in the README Prerequisites instead.
• AGENTS.md — 310 lines. The only two AGENTS.md files on main are under
  1.architectures/7.sagemaker-hyperpod-eks/; there is no precedent under 3.test_cases/, and
  its content substantially restates README.md + docs/. If you want to keep it, it needs to be
  maintained as a third copy of the same facts — and it already isn't (see the .gitignore drift
  in Batch 5).

[mvinci12]

All dropped in d06b7f3.

• LICENSE — confirmed your find: nothing else under 3.test_cases/ ships one.
• docs/parallelism-strategies.svg — kept, and now actually referenced: embedded in
  docs/configuration.md under "Parallelism Tuning Guide" with alt text. You were right
  that its only reference was the AGENTS.md listing, which is also gone.
• kubernetes/lmeval-tasks/bigcodebench_p4.yaml — dropped. The finding stays in
  docs/results.md, reworded from "authored but NOT yet usable" to "attempted and found
  not usable with this harness", so it no longer implies a shippable artifact.
• kubernetes/eval-mlflow-log-job.yaml — dropped. You were right that nothing applied it;
  I also confirmed it had no envsubst allowlist, so an unset MLFLOW_RUN_ID would have
  rendered a bare --mlflow-run-id and failed argument parsing.
• lustre/ and conf/cluster/ — dropped; see the summary comment.
• AGENTS.md — dropped. Its unique content was migrated first (conventions to
  docs/configuration.md, self-test gates to the README). Worth noting the standalone
  repo this was vendored from gitignores its own copy, so dropping it here is consistent.

[KeitaW]

Config keys that nothing reads

sandbox.max_concurrent is self-labelled # NOT WIRED, and the 40 lines of comment above it
explain at length that it does nothing. Shipping a knob that a reader will reasonably try to turn,
with a comment that says turning it does nothing, is worse than not shipping it — the real knob
(SANDBOX_MAX_CONCURRENT_PER_WORKER, read in custom_reward_fn.py:298) should be the only one
documented.

Suggested change

	max_concurrent: 256 # NOT WIRED — see above
	# Sandbox concurrency is bounded per RewardLoopWorker process by
	# SANDBOX_MAX_CONCURRENT_PER_WORKER (see scripts/custom_reward_fn.py); there is
	# no verl CLI knob for it, so nothing is plumbed from here.

The same class of key appears elsewhere; each is a one-line delete:

Key	Where	Consumer
ray.runtime_env	conf/config.yaml:80	none — submit_training.py:806 hardcodes script_dir / "runtime_env.yaml" [confirmed]
gradient_checkpointing, use_remove_padding, forward_prefetch	conf/backend/fsdp.yaml:11-13	none — submit_training.py:155-156 hardcodes the first two True and never emits the third [confirmed]
load_format	conf/lora/enabled.yaml:8	none — submit_training.py:187 hardcodes safetensors [confirmed]
num_efa_per_node, head_instance_type	conf/cluster/*.yaml	none — moot if the cluster group is dropped, see Batch 1
VLLM_EXECUTE_MODEL_TIMEOUT_SECONDS	env_vars.example:96, marked REQUIRED	none — absent from deploy_vllm_eval.sh's SUBST_VARS and from vllm-eval.yaml [confirmed]
S3_BUCKET_NAME, RAY_memory_usage_threshold, NCCL_IB_DISABLE, MLFLOW_TRACKING_NAME	env_vars.example	none

The VLLM_EXECUTE_MODEL_TIMEOUT_SECONDS one is the costly member of the set: the comment says it
is required because "a 105 GB adapter load far exceeds vLLM's 300 s default and the first request
returns HTTP 500," and the value is silently discarded, so the documented failure still happens.

Relatedly, env_vars.example:118-124 says the NCCL block "propagate[s] via the shell environment
to Ray workers at runtime." It doesn't: env_vars is sourced on the submit host, and
scripts/runtime_env.yaml carries only a pip: list — the only env vars that reach the workers
are the four +ray_kwargs.ray_init.runtime_env.env_vars.* entries in submit_training.py.
NCCL_NET_GDR_LEVEL happens to work because the Dockerfile also bakes it; NCCL_DEBUG and
NCCL_IB_DISABLE do not reach a worker at all.

[mvinci12]

All deleted in d06b7f3, and each site now carries one line saying where the real
behaviour lives. sandbox.max_concurrent, ray.runtime_env,
gradient_checkpointing/use_remove_padding/forward_prefetch, load_format,
num_efa_per_node/head_instance_type (with the whole group). I also removed
VLLM_NODE_INSTANCE_TYPE and RAY_memory_usage_threshold, which have no consumer either;
I kept S3_BUCKET_NAME, NCCL_IB_DISABLE and MLFLOW_TRACKING_NAME because the docs do
reference them.

The two you singled out as costly I wired rather than deleted, since both are
documented as REQUIRED for a real failure:

• VLLM_EXECUTE_MODEL_TIMEOUT_SECONDS now has an env: entry in vllm-eval.yaml and is
  in deploy_vllm_eval.sh's SUBST_VARS.
• VLLM_ENFORCE_EAGER had the same defect one layer deeper, which I don't think you saw:
  it was in SUBST_VARS, but appeared in the manifest only as
  ${VLLM_ENFORCE_EAGER:-false} with no env: entry — and envsubst does not expand the
  ${VAR:-default} form, so it was evaluated in-container where the variable was unset and
  the documented escape hatch always resolved to false. Also wired.

Your closing point about the NCCL block was right and is corrected: env_vars.example now
says these do not reach the workers, names the four
+ray_kwargs.ray_init.runtime_env.env_vars.* overrides that do, and explains that
NCCL_NET_GDR_LEVEL worked only because the Dockerfile bakes it.

[KeitaW]

Operator run-diary in shipped source

custom_reward_fn.py is 564 lines, 238 of them comments (42%), and the block starting here is
~80 lines of dated incident narrative — "THE MODEL ABOVE IS WRONG", restart rates per pod per
hour, error densities per 1k log lines, timestamps from specific runs. conf/sandbox/enabled.yaml
carries a second copy of the same narrative, and conf/config.yaml:37-58, conf/data/mixed-hard.yaml
and conf/data/mixed-valplus.yaml carry more.

The conclusions are genuinely valuable and I'd hate to lose them. But a reader opening a reward
function to understand routing has to scroll past a debugging journal to reach compute_score,
and a lot of it references artifacts they can't see. docs/results.md is already the right home
and is already excellent — please move the narrative there and leave the invariant behind:

Suggested change

	# 2026-08-06: THE MODEL ABOVE IS WRONG. It bounds REQUESTS, not EXECUTIONS.
	# This semaphore bounds in-flight REQUESTS, not executions. One request fans out to
	# ~39 sandbox executions, and nothing bounds those — so lowering this reduces the blast
	# radius of a pod death but does NOT fix sandbox OOM. See docs/results.md for the
	# measurements behind that conclusion.

[mvinci12]

Moved in d06b7f3, using your suggested invariant almost verbatim. docs/results.md gains
"Sandbox concurrency and OOM (unresolved)" holding the retracted first model, the ~39
test-cases-per-request measurement, both ruled-out hypotheses, and the negative result at
semaphore 5 including the higher restart rate. The ~40-line duplicate in
conf/sandbox/enabled.yaml went with it (66 → 37 lines).

I took the narrower option on the other three files you cite — conf/config.yaml:37-58 and
the two conf/data/ files keep their narrative. The token-budget block justifies the value
on the next line, and the contamination audit is load-bearing exactly where the dataset is
selected. Happy to move those too if you'd rather; I stopped at the ones whose conclusion is
"do not turn this knob", which is the pair that genuinely did not belong next to code.

[KeitaW]

Review Batch 2/6 — Correctness: the documented quickstart does not run as written

[KeitaW]

custom_reward_fn.py is loaded from a path nothing puts it at

Must fix. This line points verl at {cfg.cluster.fsx_home}/custom_reward_fn.py =
/fsx/data/verl/custom_reward_fn.py. The only thing that places the file anywhere is
Dockerfile:155, which copies it to /workspace/custom_reward_fn.py — a different path, on a
different filesystem. custom_reward_fn.py:28 documents the Docker path
(custom_reward_function.path=/workspace/custom_reward_fn.py), contradicting this line, and
AGENTS.md:263-264 says the FSx copy is "synced via FSx DRA from S3" — a mechanism this PR
neither creates nor documents.

grep -rn custom_reward_fn across the contribution finds exactly one thing that stages it, and it
is eval-only: submit_val_eval_k8s.sh:99 kubectl cps it into the fsx-utils pod.

So a reader following the README quickstart — where sandbox: enabled is the default in
conf/config.yaml:22 — submits training with verl pointed at a file that does not exist on FSx.
Two clean fixes:

Suggested change

	f"reward.custom_reward_function.path={cfg.cluster.fsx_home}/custom_reward_fn.py",
	f"reward.custom_reward_function.path=/workspace/custom_reward_fn.py",

(the file is already baked into the image the Ray workers run), or keep the FSx path and add an
explicit staging step to the README quickstart and to data/submit_data_prep.sh. The image path
is the simpler one and removes a whole class of "which copy is running?" ambiguity.

[mvinci12]

Fixed in e6b3b87, taking your first option: the path is now
/workspace/custom_reward_fn.py, the copy the Dockerfile bakes into the image the Ray
workers run. You were right that this made the default quickstart submit against a
nonexistent file — sandbox: enabled is the default, so it was not an edge case. The
comment now says explicitly not to point this at FSx and why, and the FSx-DRA claim that
contradicted it died with AGENTS.md.

[KeitaW]

KUBE_NAMESPACE is honoured by some manifests and ignored by the sandbox

Must fix. fsx-utils.yaml:18, vllm-eval.yaml:30, lmeval-job.yaml:25, valeval-job.yaml:20
and lustre/pvc.yaml:7 all set namespace: ${KUBE_NAMESPACE}. The sandbox-fusion Deployment
(here) and its Service (:143) set no namespace at all, and the README's apply command has no
-n, so they land in whatever namespace the current kubectl context points at. [confirmed]

Meanwhile conf/sandbox/enabled.yaml:6 hardcodes the resolved DNS name:

url: http://sandbox-fusion.default.svc.cluster.local:8080/run_code

With KUBE_NAMESPACE=verl (or any non-default value, which is the whole reason the variable
exists), the PVC and workload pods go to verl, the sandbox Deployment goes wherever the kubectl
context points, and every reward call still resolves sandbox-fusion.default.svc. If the context's
namespace is anything other than default, that name has no Service behind it and
preflight_sandbox_check aborts the submission with "sandbox is unhealthy" against a URL that was
never going to resolve. Even in the case where the sandbox does land in default and the URL
happens to work cross-namespace, the coupling is invisible: the variable that is supposed to move
the whole deployment moves everything except the sandbox. conf/cluster/*.yaml:21 hardcodes
namespace: default as well.

Suggested change

	name: sandbox-fusion
	name: sandbox-fusion
	namespace: ${KUBE_NAMESPACE}

…on both the Deployment and the Service, plus:

# conf/sandbox/enabled.yaml
  url: http://sandbox-fusion.${oc.env:KUBE_NAMESPACE,default}.svc.cluster.local:8080/run_code

[mvinci12]

Fixed in e6b3b87, both halves. namespace: ${KUBE_NAMESPACE} on the Deployment and the
Service, and conf/sandbox/enabled.yaml now interpolates
${oc.env:KUBE_NAMESPACE,default} into the FQDN — line 60 of that same file was already
doing it correctly for the preflight namespace, which made the inconsistency clear.
Verified: KUBE_NAMESPACE=verl resolves to
http://sandbox-fusion.verl.svc.cluster.local:8080/run_code. The conf/cluster/*.yaml
hardcoded namespace: default you also mention went away with the group.

docs/troubleshooting.md no longer tells the reader to keep the two in sync by hand; it
now states the one thing that still must hold — export the variable in both the envsubst
shell and the submit shell.

[KeitaW]

FSDP silently overrides ppo_mini_batch_size after the config sets it

_build_shared_overrides emits actor_rollout_ref.actor.ppo_mini_batch_size from
model.ppo_mini_batch_size falling back to training.ppo_mini_batch_size
(submit_training.py:82-83), and then this line emits the same key again with a literal 24.
Both land on the verl command line, and Hydra applies repeated dotted overrides in order, so the
last one wins — verified live against hydra-core 1.3.5 / omegaconf 2.3.1 on 2026-08-18:

$ python app.py actor.ppo_mini_batch_size=16 actor.ppo_mini_batch_size=24
RESULT ppo_mini_batch_size = 24

So on the default backend, training.ppo_mini_batch_size (conf/config.yaml:61) and any
per-model ppo_mini_batch_size are dead, and print_config_summary (:562-567) prints
"MiniBatch: N (model override…)" for a value the job will not use. The 24 is also derived for
48 GPUs by its own comment, but it is applied unchanged under cluster=p6-b200-1node. [confirmed]

Computing one value and emitting it once removes both problems:

Suggested change

	"actor_rollout_ref.actor.ppo_mini_batch_size=24",
	# FSDP normalizes ppo_mini_batch_size per-GPU as (mini_batch * rollout.n) // world_size.
	# Emitted only here for FSDP; _build_shared_overrides emits the Megatron value.
	f"actor_rollout_ref.actor.ppo_mini_batch_size="
	f"{OmegaConf.select(cfg.model, 'ppo_mini_batch_size', default=cfg.training.ppo_mini_batch_size)}",

…with the shared builder emitting the key only for the Megatron path, and a divisibility assert
((mini * n) // world_size % micro == 0) so a bad combination fails at submit instead of step 3.

[mvinci12]

Fixed in e6b3b87. Emitted exactly once now, from the shared builder, for both backends.

Rather than keep a bypass, I chased why the literal existed: the global default of 16 really
was invalid for FSDP at 48 GPUs. I enumerated the arithmetic across all six models on both
backends at 1 and 6 nodes; 24 satisfies every shape, so that is the global default and
the literal is gone. Your "applied unchanged under 1-node" point is also fixed by this —
at 8 GPUs the value now normalizes correctly.

print_config_summary was worse than you described: not only ungated by backend, it printed
nothing at all when the model had no override, so the effective value was never shown. It
now always prints the emitted value, its source, and the per-group figure verl derives.

I took the divisibility assert too, and it earned its keep immediately — see the summary
comment for the qwen25-72b / qwen3-coder-next finding it surfaced.

[KeitaW]

The pod anti-affinity selectors match no KubeRay pod

Both anti-affinity terms here (and vllm-eval.yaml:78) select on ray-node-type. KubeRay labels
Ray pods ray.io/node-type — the prefixed form
(Ray docs: KubeRay + Prometheus,
verified live 2026-08-18: "ray.io/node-type: head / ray.io/node-type: worker"). The sibling
recipe in this same tree uses it too —
3.test_cases/pytorch/verl/kubernetes/rlvr/setup/load_data_grpo.sh:9:
kubectl get pods -l ray.io/node-type=head — as does this PR's own
scripts/scale_ray_workers.sh:93 for the neighbouring label (-l ray.io/cluster=${RAY_CLUSTER}).
An unprefixed ray-node-type matches nothing, so both "preferred" terms score zero on every node
and the scheduler places the pods as if no anti-affinity existed. [confirmed]

That is consistent with what the comment four lines above records: "During the 2026-07-10 235B run,
both replicas shared one node with the Ray head and crashed together under scoring load." The
anti-affinity added in response was inert.

Suggested change

	ray-node-type: head
	ray.io/node-type: head

Same change at vllm-eval.yaml:78-80 (key: ray.io/node-type). Worth confirming against your own
cluster with kubectl get pod <ray-head-pod> --show-labels before merging, since the intended
separation for a privileged sandbox arguably wants a required term rather than a preferred one.

[mvinci12]

Fixed in e6b3b87: ray.io/node-type in both sandbox-fusion.yaml and
vllm-eval.yaml:78. Your reading is right, and the comment now records it — the
anti-affinity added in response to the 2026-07-10 co-location crash was inert.

On preferred vs required: I left both preferred. Your reasoning for required is
sound, but I can't verify node counts on the target cluster from here, and a required term
that cannot schedule fails worse than a soft one. Instead docs/cluster.md gains a "Blast
radius" section that tabulates what is enforced against what is not, names a NetworkPolicy
and a dedicated tainted node group as the two additions worth making on a shared cluster,
and includes the --show-labels command you suggested for confirming the selector matches.

[KeitaW]

Resume preflight reports "fresh run" when the check itself fails

preflight_resume_check exists, by its own docstring, because "a run gets restarted on top of a
checkpoint that was trained against a broken reward signal without anyone noticing." But the
kubectl exec at :766 runs cat ... 2>/dev/null || true with check=False, and the
TimeoutExpired handler at :772-773 sets step = "". Every failure mode — pod missing, wrong
namespace, kubectl not authenticated, exec timeout — produces an empty step, which lands on this
line and prints "starting from the base model (fresh run)". [confirmed]

The job is then submitted with resume_mode=auto, so if a tracker file does exist, verl resumes
from it — the exact scenario the guard was written to make visible, now reported as its opposite.

Suggested change

	print(" --> no tracker file found: starting from the base model (fresh run)")
	if step:
	...
	elif probe_failed:
	print(" --> WARNING: could not read the tracker file (kubectl exec failed).")
	print(" resume_mode=auto may still resume from an existing checkpoint.")
	print(" Re-run with backend.resume_mode=disable for a guaranteed fresh start.")
	else:
	print(" --> no tracker file found: starting from the base model (fresh run)")

…where probe_failed is set from proc.returncode != 0 or the timeout branch.

[mvinci12]

Fixed in e6b3b87 along the lines you suggested. probe_failed is set from
proc.returncode != 0 or the timeout, and the third branch prints the kubectl error, says
plainly that this is not the same as "no checkpoint", and that resume_mode=auto may still
resume. Your framing — the guard reported its own failure as its opposite — is exactly what
made this worth fixing rather than tolerating.

One thing beyond the comment: only TimeoutExpired was caught in both preflights, so a
missing kubectl binary raised a bare traceback. Both now handle
FileNotFoundError/OSError, and the sandbox preflight exits with an actionable message.

[KeitaW]

Review Batch 3/6 — Evaluation integrity

docs/results.md is the strongest part of this contribution (see Batch 6), which is exactly why
the measurement plumbing underneath it deserves this much attention.

[KeitaW]

The two val-split paths generate at different token budgets

Must fix. submit_val_eval_k8s.sh:72 sets VAL_MAX_TOKENS=24576 and passes it through.
submit_val_eval.sh — the Ray path, and the one README.md and AGENTS.md document — never
passes --max-tokens at all, so it takes this default of 10240. [confirmed]

Training generates at max_response_length: 24576, and conf/config.yaml:37-51 records in detail
what happens below that ceiling: at 16384, ~22% of every batch was a forced zero because a
truncated response has no \boxed{} and does not compile, and corr(response_length, score) = -0.457.
At 10240 the censoring is worse. So the two "equivalent" val-split evaluations are not comparable
with each other, and neither of them is comparable with in-training validation.

Suggested change

	"--max-tokens", type=int, default=10240, help="Generation max_tokens"
	"--max-tokens", type=int, default=24576,
	help="Generation max_tokens. Must match training's max_prompt_length + "
	"max_response_length budget (conf/config.yaml) or scores are not comparable.",

…and pass --max-tokens "${VAL_MAX_TOKENS:-24576}" explicitly from submit_val_eval.sh:117 as
well, so the value is visible in both submitters rather than inherited from a default.

[mvinci12]

Fixed in 30a3b25, both parts of your suggestion. Default raised to 24576 with the help
text naming the training budget, and submit_val_eval.sh now passes
--max-tokens "${VAL_MAX_TOKENS}" explicitly so the figure is visible in both submitters
rather than inherited. You were right that the Ray path — the documented one — was silently
running at 10240, below the 16384 the k8s script's own comment measured as a ~6× artifact
on apps.

[KeitaW]

compare_eval_results.py collapses task variants and manufactures zeros

Two problems in the loader, both of which produce a plausible-looking table from bad inputs.

Task names are prefix-matched. base_task = next((t for t in LMEVAL_TASKS if task.startswith(t)), None)
maps humaneval, humaneval_p4 and humaneval_p10 all onto the key humaneval_pass_at_1. This
PR ships exactly those variants (kubernetes/lmeval-tasks/humaneval_p4.yaml,
humaneval_p10.yaml, mbpp_p1/p4/p10.yaml), so running two of them in one lm-eval invocation —
which LMEVAL_TASKS accepts as a comma list — silently keeps whichever the JSON iterates last.
[confirmed]

Missing statistics become 0.0. At :110,
float(stats.get("mean_score", 0.0)) turns an absent field into a real-looking zero, which then
renders as a large negative delta against the baseline. At :79 and :104, a missing or
malformed results file returns {}, and the tool exits 0 with an empty table — verified by Codex
on 2026-08-18 by running the script against two nonexistent runs: it printed a complete markdown
report with empty tables and no error. [confirmed]

Suggested change

	# match task even if lm-eval used a variant name (humaneval_greedy etc.)
	# Exact task-id match: humaneval / humaneval_p4 / humaneval_p10 are distinct
	# benchmarks and must not collapse onto one metric key.
	base_task = task if task in LMEVAL_TASKS or task.rsplit("_", 1)[0] in LMEVAL_TASKS else None

…plus raising (rather than returning {}) when a requested run's results.json is absent, and
dropping the 0.0 default so an absent mean_score is reported as missing.

While you're here: _load_lmeval_metrics explicitly skips every *_stderr key (:87). The PR
description and docs/results.md quote σ and p-values throughout, so a reader cannot reproduce
the significance claims from the committed tooling. Surfacing stderr alongside each metric would
close that gap cheaply.

[mvinci12]

All four in 30a3b25.

• Exact task-id match, with every shipped variant named in LMEVAL_TASKS. I used exact
  membership rather than your rsplit suggestion so that a future mbpp_p20 fails loudly
  as unknown instead of silently folding into mbpp.
• The 0.0 default is gone; an absent mean_score is reported missing. Same for
  count/errors.
• All four {} returns raise, and main() turns that into a non-zero exit naming the paths
  it looked in. Your Codex repro — two nonexistent runs — now exits 1 instead of printing a
  complete markdown report.
• stderr is surfaced alongside each metric. You were right that the σ and p-values
  docs/results.md quotes were not reproducible from the committed tooling.

Verified against fixtures: humaneval and humaneval_p4 in one results.json survive as
separate keys, stderr appears, and a val_split.json missing mean_score for one source no
longer manufactures a zero for it.

[KeitaW]

deploy_vllm_eval.sh defaults are the values env_vars.example says will fail

env_vars.example:82-88 marks two values REQUIRED and says exactly what happens otherwise:

• VLLM_MAX_MODEL_LEN=32768 — "must match LMEVAL_MAX_LENGTH … If the server allows less than the
  client requests, requests fail."
• VLLM_GPU_MEM_UTIL=0.95 — "REQUIRED at 0.95 for large adapters. At 0.85 the KV cache loses
  ~23 GiB and the server CrashLoopBackOffs with 'No available memory for the cache blocks'."

This script's fallbacks are 12288 and 0.85. Since env_vars is gitignored and sourcing it is
optional (:31), anyone who exports VLLM_MODEL_PATH/VLLM_SERVED_NAME and runs the script gets
both documented failures. 12288 is also below training's own 2048 + 24576 = 26624. [confirmed]

Suggested change

	export VLLM_MAX_MODEL_LEN="${VLLM_MAX_MODEL_LEN:-12288}"
	export VLLM_MAX_MODEL_LEN="${VLLM_MAX_MODEL_LEN:-32768}"

…and VLLM_GPU_MEM_UTIL="${VLLM_GPU_MEM_UTIL:-0.95}" at :49.

Two more in the same script: VLLM_ENFORCE_EAGER is exported at :60 but is not in SUBST_VARS
(:93) and not read by vllm-eval.yaml, so the documented VLLM_ENFORCE_EAGER=true escape hatch
does nothing; and REGISTRY is required at :38, before the --delete branch at :63, so the
documented teardown ./scripts/deploy_vllm_eval.sh --delete fails in a clean shell and leaves an
8-GPU deployment running. Moving the delete branch above the deploy-only validation fixes the
second. [confirmed]

[mvinci12]

All three in 30a3b25. VLLM_MAX_MODEL_LEN → 32768 and VLLM_GPU_MEM_UTIL → 0.95, each
with the documented failure recorded at the default. Your point that env_vars is optional
at :31 is what made these load-bearing rather than cosmetic.

The --delete ordering is fixed — the teardown branch now runs before any deploy-only
validation, since it needs only the namespace. Verified with REGISTRY unset: it tears down
and exits 0. Raised rollout status --timeout from 20m to 100m at the same time, since 20m
was under the startup budget the script itself documents.

VLLM_ENFORCE_EAGER is now genuinely wired — see my reply on the dead-keys comment for why
it was inert in a slightly different way than "not in SUBST_VARS".

[KeitaW]

The "wait for vLLM" loops cannot fail

The loop above (:75-80, and the identical one at valeval-job.yaml:68-72) polls
/v1/models 120 times at 15 s. If every probe fails, it falls out after 30 minutes and execution
continues straight into lm_eval, which then runs against a dead endpoint under set -euo pipefail
— burning the retry budget and failing with an asyncio timeout rather than at the readiness gate.
[confirmed]

Suggested change

	done
	if curl -sf "${MODELS_URL}" >/dev/null; then
	echo "[lmeval] vLLM is up"; break
	fi
	sleep 15
	if [ "$i" -eq 120 ]; then
	echo "[lmeval] ERROR: vLLM never became ready at ${MODELS_URL}" >&2; exit 1
	fi
	done

[mvinci12]

Fixed in 30a3b25 with your suggested shape, in both lmeval-job.yaml and
valeval-job.yaml:68-72. Both now exit 1 on the final attempt with the URL in the
message, so the failure lands at the readiness gate instead of as an asyncio timeout after
the retry budget is spent.

[KeitaW]

The vLLM rollout wait is shorter than the startup it is waiting for

This startupProbe budgets ~90 minutes for the documented ~60-minute 235B weight load, which is
right. But the Deployment sets no progressDeadlineSeconds, and the Kubernetes default is 600 s
(Deployment API reference),
so kubectl rollout status in deploy_vllm_eval.sh:98 reports ProgressDeadlineExceeded about
ten minutes in — while the pod is starting exactly as designed. The script's own --timeout=20m
is also under the startup budget it documents at :97 ("may take 10-15 min"). [confirmed]

Suggested change

	failureThreshold: 180 # 180 * 30s = 90 min (235B MoE at TP=8: ~60min weight load + engine/KV-cache init; ~32s/shard is vLLM per-shard TP-split overhead, not FSx I/O which measures 760MB/s)
	spec:
	progressDeadlineSeconds: 7200

…on the Deployment (:35), and --timeout=100m on the rollout status call.

[mvinci12]

Fixed in 30a3b25: progressDeadlineSeconds: 7200 on the Deployment and --timeout=100m
on the rollout status call. The comment records that the K8s default of 600s was reporting
ProgressDeadlineExceeded while the pod was starting exactly as designed, and that this
should stay ≥ the startupProbe budget.

[KeitaW]

The merge provenance gate is off by default

The comment at :77-83 states the failure precisely: DATASET defaults to mixed-code-math-v2,
"so merge_adapter.sh 100 with default arguments merges the WRONG experiment's adapter, and the
non-zero gate, the delta gate and check_merge_parity.py all pass — none of them look at WHICH
adapter it is."

Having diagnosed that, the script warns and proceeds. A gate that is off unless you already know
to turn it on protects nobody — and check_merge_parity.py is described in README.md as the
thing to run "before spending eval GPU-hours." Reading rank/alpha out of the adapter's own
adapter_config.json and asserting against the config that produced it would make this automatic;
short of that, requiring an explicit opt-out is the minimum:

Suggested change

	if [ ${#EXPECT_ARGS[@]} -eq 0 ]; then
	if [ ${#EXPECT_ARGS[@]} -eq 0 ] && [ "${ALLOW_UNVERIFIED_ADAPTER:-0}" != "1" ]; then
	echo "ERROR: EXPECT_LORA_RANK unset -- nothing verifies WHICH adapter is merged." >&2
	echo " DATASET currently resolves to '${DATASET}'." >&2
	echo " Set EXPECT_LORA_RANK (and EXPECT_LORA_ALPHA), or ALLOW_UNVERIFIED_ADAPTER=1." >&2
	exit 1
	fi

[mvinci12]

Fixed in 30a3b25, fail-closed with your suggested ALLOW_UNVERIFIED_ADAPTER=1 opt-out.
Your framing settled it: a gate that is off unless you already know to turn it on protects
nobody, and the README points at check_merge_parity.py as the thing to run before spending
eval GPU-hours. The error prints both escape routes. I updated the documented invocations in
the README and docs/eval-pipeline.md so they still work.

I did not do the adapter_config.json version you preferred — reading rank/alpha from the
adapter and asserting against the producing config is better, and I'd rather not hand-roll
it without a checkpoint to test against. The explicit gate is the minimum you named.

[KeitaW]

Review Batch 4/6 — Security and reproducibility

No .dockerignore, so env_vars travels in the build context

build-push.sh:44-49 builds with the test-case root as the context, and that directory is where
env_vars (tokens, MLflow ARN) lives. .gitignore does not apply to Docker contexts, so the file
is sent to the daemon — and to a remote builder if one is configured. Nothing COPYs it into a
layer today, but a future COPY . . would bake it. A four-line file removes the whole class:
[confirmed]

# .dockerignore
env_vars
.git/
outputs/
profiling/

53 of the 85 new files are missing the MIT-0 SPDX header

Every .py and .sh file in this PR carries the header correctly. The YAML, Markdown, Dockerfile,
.gitignore and env_vars.example files do not — 53 files in total, including all 24 conf/*.yaml,
all 10 kubernetes/*.yaml, all 3 lustre/*.yaml, scripts/runtime_env.yaml, the Dockerfile,
README.md, AGENTS.md and every file under docs/.

Comment-prefixed header lines don't affect YAML, Dockerfile or Markdown parsing, and this is
consistently a merge gate on this repo. A two-line prepend covers all of them:

# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
# SPDX-License-Identifier: MIT-0

Reproduce the list with:

for f in $(git ls-files 3.test_cases/pytorch/verl/grpo-megatron-lora); do
  head -5 "$f" | grep -q 'SPDX-License-Identifier: MIT-0' || echo "$f"
done

[KeitaW]

set -x before sourcing env_vars prints HF_TOKEN in cleartext

Must fix. All six models/download_qwen*.sh set set -xeuo pipefail here and then
source "${SCRIPT_DIR}/../env_vars" four lines down. Bash xtrace applies to commands executed
inside a sourced file, so every export in env_vars — including HF_TOKEN and any AWS
credentials — is echoed to stderr. Verified live, 2026-08-18: [confirmed]

$ cat fake_env_vars      # export HF_TOKEN="hf_SECRETVALUE123"
$ bash -c 'set -xeuo pipefail; source ./fake_env_vars; echo done'
+ source ./fake_env_vars
++ export HF_TOKEN=hf_SECRETVALUE123
++ HF_TOKEN=hf_SECRETVALUE123

These scripts are run through models/run_on_cluster.sh / kubectl exec, so that stderr lands in
pod logs and terminal scrollback.

Suggested change

	set -xeuo pipefail
	set -euo pipefail

If the tracing is useful, set -x after the source (and set +x around any later secret use)
keeps it without the leak.

[mvinci12]

Fixed in d06b7f3, where the six scripts collapsed to one. The consolidated
models/download_model.sh uses set -euo pipefail, with a comment recording why set -x
must not come back: xtrace applies inside the sourced file, and run_on_cluster.sh runs
this via kubectl exec, so the trace lands in pod logs. Your bash -c reproduction was the
clearest part of the review.

Because this landed in the consolidation commit rather than the security one, the same rule
is written into the shell conventions now in docs/configuration.md, so it applies to the
next script too.

[KeitaW]

HF_TOKEN as a literal value in a long-lived Pod spec

envsubst substitutes the real token into env.value, so it is stored in etcd and readable by
anyone with get pod in the namespace — on a pod whose entire job is to sleep infinity, so it
sits there for the life of the cluster. [confirmed]

I know some existing test cases do the same (3.test_cases/pytorch/FSDP/kubernetes/*.yaml), so
this is not a new pattern in the repo — but 3.test_cases/pytorch/dreamzero/kubernetes/libero/secret.example.yaml
is the better precedent and is cheap to follow here:

Suggested change

	value: "${HF_TOKEN}"
	- name: HF_TOKEN
	valueFrom:
	secretKeyRef:
	name: hf-token
	key: HF_TOKEN

…created out of band with kubectl create secret generic hf-token --from-literal=HF_TOKEN=$HF_TOKEN,
documented in the README quickstart alongside the envsubst line.

[mvinci12]

Fixed in 58624d6 following the dreamzero precedent you pointed at: secretKeyRef on a
hf-token Secret, with kubectl create secret generic in the README quickstart and in the
manifest header. Your "on a pod whose entire job is to sleep infinity" framing is what
makes this different from the FSDP/kubernetes precedent — the exposure window is the life
of the cluster.

[KeitaW]

Untrusted model-generated code runs privileged, and again unsandboxed in the eval driver

Two halves of the same concern, and the second is the surprising one.

sandbox-fusion.yaml:94-95 runs privileged: true, with the rationale "SandboxFusion requires
privileged mode for cgroup-based process isolation." Fair enough as a starting point, but a
privileged container executing model-written programs is a node-level escape path, and the pod has
no NetworkPolicy, no automountServiceAccountToken: false, and only preferred anti-affinity
from the Ray head — which, per the previous batch, currently matches nothing at all. So the
untrusted-code executor can schedule onto the node hosting the training control plane. A short
paragraph in docs/cluster.md on the intended blast radius (dedicated tainted node, no service
account token, restricted egress) would make the trade-off reviewable rather than implicit —
README.md currently presents the sandbox purely as a convenience. [confirmed]

The second half is this line. The lm-eval driver runs HF_ALLOW_CODE_EVAL=1 +
--confirm_run_unsafe_code, which executes generated programs in the driver pod itself — an
ordinary pod with network access, a default service-account token, and the full FSx PVC mounted
read-write at /fsx (:149-150), where the checkpoints and every eval result live. That is
strictly less isolated than the sandbox this contribution deploys for the same purpose during
training. At minimum: automountServiceAccountToken: false, and mount FSx read-only with a
separate writable path for OUT_DIR. [confirmed]

[mvinci12]

Addressed in 58624d6. You were right that the second half is the surprising one, and it
was the more serious of the two.

• The lm-eval driver now runs automountServiceAccountToken: false, with /fsx mounted
  read-only and a single narrow read-write mount at /eval-out
  (subPath: data/verl/eval_results) for its own output, OUT_DIR repointed there. So
  generated code can no longer reach the checkpoints or other runs' results.
• docs/cluster.md gains "Blast radius: two places model-generated code executes", written
  as a table of what is set against what is not — automountServiceAccountToken: false, no
  FSx mount and preferred anti-affinity on one side; no NetworkPolicy and no dedicated
  tainted node on the other. It names those last two as the additions worth making on a
  shared cluster, and notes the anti-affinity was inert until the label prefix was fixed.

I did not add the NetworkPolicy or the taint. Both depend on the cluster's existing
policy and node layout, and shipping a guess would be worse than documenting the gap
precisely — but say the word if you'd rather this PR carried a default NetworkPolicy.

[KeitaW]

Unpinned installs at pod start contradict the checklist

The PR checklist ticks "External dependencies are pinned to a specific version or tag (no
latest)", and CONTRIBUTING.md states the rule: "If your scripts depend on external software
(libs, frameworks, containers…) then fix the versions via a tag or commit ID to ensure
reproducibility." scripts/runtime_env.yaml and the Dockerfile honour it well. The
pip-at-startup paths do not: [confirmed]

Location	Unpinned
kubernetes/lmeval-job.yaml:60 (this line)	torch
kubernetes/valeval-job.yaml:58-59	torch, aiohttp, tqdm, pandas, pyarrow
kubernetes/fsx-utils.yaml:42	datasets, huggingface_hub
kubernetes/eval-mlflow-log-job.yaml:41	sagemaker-mlflow, boto3
data/submit_data_prep.sh:70	numpy, pyarrow, huggingface_hub
scripts/merge_adapter.sh:140-141	peft>=0.11, safetensors>=0.4 (ranges, not pins)

Suggested change

	pip install --index-url https://download.pytorch.org/whl/cpu torch >/work/pip-torch.log 2>&1 || { tail -30 /work/pip-torch.log; exit 1; }
	pip install --index-url https://download.pytorch.org/whl/cpu "torch==<the CPU torch you validated>" >/work/pip-torch.log 2>&1 || { tail -30 /work/pip-torch.log; exit 1; }

The driver images
(python:3.11-slim, python:3.12-slim, and LMEVAL_IMAGE's default) are floating tags too —
patch-level tags or digests would settle those.

Also: imagePullPolicy: Always on already-pinned images at sandbox-fusion.yaml:85 and
vllm-eval.yaml:88. It re-pulls on every restart and breaks air-gapped clusters; IfNotPresent
is the right value once the tag is pinned.

[mvinci12]

Fixed in 58624d6 across all six locations plus the driver images. Rather than invent
numbers I went looking for what was actually installed, and found one machine-generated
record: the results.json lm-eval wrote inside the driver pod on the reported code runs.
It reports torch 2.13.0+cpu, transformers 4.49.0, numpy 2.4.6, Python 3.11.15.

Those are == pins now. Everything else is a bounded range in the style
runtime_env.yaml uses, because no run recorded the resolved version — every install
redirected to a /work/pip.log on an emptyDir. Each block says which of the two it is, so
a reader can tell a measurement from a guess. Full detail in the summary comment, including
the coverage caveat.

Driver images are pinned to python:3.11.15-slim, the patch tag python:3.11-slim
resolved to for those runs — which also removes the 3.11/3.12 drift fsx-utils had against
the driver Jobs. imagePullPolicy: Always → IfNotPresent on both already-pinned images.

[KeitaW]

trust_remote_code without a risk note

Three dataset configs set "trust_remote_code": True (:47, :53, :59), and
kubernetes/vllm-eval.yaml:150 passes --trust-remote-code. Both execute Python fetched from the
Hub at load time. It is very likely necessary here (APPS/TACO ship loading scripts), so this is a
documentation ask rather than a behaviour change — a one-line comment naming why each one needs
it, so the next reader doesn't have to decide whether it was deliberate:

Suggested change

	"trust_remote_code": True,
	# trust_remote_code: APPS ships a dataset loading script; this executes code
	# fetched from the Hub at load time. Pin `revision` below to make that auditable.
	"trust_remote_code": True,

Relatedly, none of the snapshot_download calls (models/download_*.sh:38, download_model.py)
pass a revision, so they track a mutable branch — same reproducibility rule as the pip pins
above. [confirmed]

[mvinci12]

Documented in 58624d6, and your instinct that it is "very likely necessary" turns out to
be half right in an interesting way. There is a note above the registry saying these
execute Hub-fetched Python, that the flag is set only for the three datasets that shipped a
loading script, and that it is inert on the pinned datasets>=3.0 — which removed
loading-script support entirely. That removal is exactly the condition the existing
"scripts are no longer supported" fallback handles. So the flags are only doing anything
if someone pins an older datasets, and the comment says so rather than implying they are
required. codecontests=False is annotated as parquet-native.

--trust-remote-code on the vLLM server is documented at the invocation, noting that what
is trusted is whatever was staged and shard-verified rather than fetched at serve time. And
on your revision point: snapshot_download now takes one — HF_REVISION in the shell
path, --revision in the Python path — both warning when unset that the download is
tracking a mutable branch.

While placing that comment I found it initially inside a backslash-continued shell command,
where a # line would have swallowed the continuation. That is what prompted the new
bash -n check over shell embedded in manifests, which is now part of the gate suite.

[KeitaW]

Review Batch 5/6 — Smaller drift and edge cases

Each of these is a one- or two-line fix; grouped so they're quick to work through.

Fallback paths that re-introduce the bug they guard against

custom_reward_fn.py:399-402 wraps from verl.utils.reward_score import sandbox_fusion in
try/except ImportError: return None, and :416-420 returns None on any unexpected return
shape. Both route the caller back to default_compute_score at :542 — which is the code path the
module header spends 45 lines explaining is broken ("metadata dropped on the floor … a test case
that failed because the sandbox was DOWN counts exactly like one the model got wrong"). So the
guard's success case is a silent return to the silently-deflating scorer, and the operator sees
neither an error nor a [SANDBOX-UNRECOVERED] line. [confirmed]

runtime_env.yaml:18 pins verl to exactly v0.8.0, so there is no version to degrade toward; the
header's "Compatible with verl main (0.8.0.dev, commit b7249af) and v0.7.0" is describing a
compatibility surface the pin removes. Importing at module scope and letting an unexpected shape
raise turns a silent measurement artifact into a loud failure at submit time.

Same shape at eval_val_split.py:36-38 (tqdm import guard selecting a second execution path) —
add tqdm to the runtime env and import it directly. [confirmed]

Two small submitter issues

• models/submit_download.sh:61 flattens the remaining arguments with EXTRA_ARGS="$*" and expands
  them unquoted at :95, so a path containing a space splits and a glob character expands on the
  submit host. EXTRA_ARGS=("$@") … "${EXTRA_ARGS[@]}" is the fix. The same script is also the only
  Ray submitter that does not pass RAY_HEADERS, so it cannot reach an authenticated dashboard.
  [confirmed]
• env_vars.example:68 exports RAY_ADDRESS unconditionally. data/submit_data_prep.sh,
  models/submit_download.sh and scripts/submit_val_eval.sh source that file before applying
  their ${RAY_ADDRESS:-…} default, so a caller's deliberate port-forward override is already lost
  — which is exactly why merge_adapter.sh:32-49 carries a bespoke save-and-restore workaround.
  Making the template conditional (export RAY_ADDRESS="${RAY_ADDRESS:-http://localhost:8265}")
  fixes the root cause and lets that workaround go. [confirmed]
• data/submit_data_prep.sh:56 blocks on an interactive read -r -p "Continue anyway? [y/N]" when
  the dashboard is unreachable, which hangs any non-interactive use. Gating it on [ -t 0 ] (or an
  --assume-yes flag) keeps the prompt for humans without wedging automation.

backend= typo silently selects FSDP

build_verl_overrides (submit_training.py:427-430) dispatches on cfg.backend.name == "megatron"
and sends everything else down the FSDP path. A mistyped backend name therefore submits a real job
with the wrong parallelism rather than failing. An explicit elif cfg.backend.name == "fsdp": … else: raise SystemExit(...) costs two lines. [confirmed]

[KeitaW]

fsx-utils is Ready before its dependencies are installed

The comment at :34-35 says "The pod is ready once pip install completes (~30-60s)", and :9
tells the reader to kubectl wait --for=condition=Ready. With no readinessProbe, Ready flips
as soon as the container process starts — so a script that execs in right after the wait can find
datasets missing. The | tail -1 on this line also discards the pip exit status (set -e without
pipefail), so a failed install looks like success. [confirmed]

Suggested change

	pip install --no-cache-dir datasets huggingface_hub 2>&1 | tail -1
	pip install --no-cache-dir "datasets==<pin>" "huggingface_hub==<pin>" "safetensors==<pin>" > /tmp/pip.log 2>&1 || { tail -20 /tmp/pip.log; exit 1; }
	touch /tmp/ready

…plus readinessProbe: {exec: {command: [test, -f, /tmp/ready]}}. Adding safetensors here also
fixes the next finding.

[mvinci12]

Fixed in 58624d6 with your suggested shape: the install writes /tmp/ready and a
readinessProbe execs test -f /tmp/ready, so kubectl wait --for=condition=Ready
now means what line 9 claims. The | tail -1 is gone, so a failed install no longer looks
like success under set -e without pipefail. I took your parenthetical too — safetensors
is in the install list, which fixes the next finding at its root.

[KeitaW]

The safetensors integrity gate silently skips on the documented path

The verify block exists to catch "truncated .safetensors files that pass the config.json check
but fail at training time." When the import is unavailable it prints a warning and sys.exit(0) —
a pass. The fsx-utils pod (the documented place to run these) installs only datasets and
huggingface_hub, neither of which requires safetensors, so on the recommended path the gate
never runs and always reports success. [confirmed]

It is also a defensive import guard, which this repo avoids by policy: the environment is known, so
importing directly and letting an absent dependency fail loudly is both simpler and correct.

Suggested change

	print("WARNING: safetensors package not installed, skipping integrity check")
	from safetensors import safe_open

(top-level, alongside the other imports in the heredoc), with safetensors added to the
fsx-utils install list.

[mvinci12]

Fixed in d06b7f3. The consolidated models/download_model.sh imports safetensors at top
level, and safetensors was added to the fsx-utils install list, so on the documented path
the gate now actually runs. Your point about the repo's own policy on defensive import
guards is well taken — it also applies to two more sites, which is the next comment.

download_model.py:78-82 had the identical fail-open; that path now depends on the same
install list.

[KeitaW]

submit_lmeval.sh header contradicts its own defaults

Three mismatches between the usage block and the code below it:

Header says	Code sets
LMEVAL_IMAGE default vllm/vllm-openai:v0.20.2 (:30)	python:3.11-slim (:92)
LMEVAL_INSTANCE_TYPE default m5.8xlarge (:32)	m5.xlarge (:94)
"Runs HumanEval + MBPP (pass@1, pass@10)" (:7)	default LMEVAL_GEN_KWARGS is greedy with no num_samples, so pass@1 only

kubernetes/lmeval-job.yaml:17-19 repeats the first one ("Image: published vllm/vllm-openai").

Suggested change

	# LMEVAL_IMAGE driver image (default vllm/vllm-openai:v0.20.2)
	# LMEVAL_IMAGE driver image (default python:3.11-slim; CPU torch + lm-eval
	# are pip-installed at start, see kubernetes/lmeval-job.yaml)

[mvinci12]

All three fixed in b96489c, plus the two propagations. LMEVAL_IMAGE documented as
python:3.11.15-slim with a note that CPU torch and lm-eval are installed at pod start;
LMEVAL_INSTANCE_TYPE as m5.xlarge; and the pass@10 claim replaced with the actual
opt-in invocation, since LMEVAL_METADATA defaults empty and pass@10 needs a sample count.
lmeval-job.yaml:17-19 repeated the image claim and docs/eval-pipeline.md repeated it
twice more — all corrected.

One more in the same block that you didn't list: line 134 described the default as "greedy"
while line 137 sets temperature=0.2, i.e. sampling. Corrected.

[KeitaW]

valeval-job can be scheduled onto a node that cannot fit it

This nodeSelectorTerms list accepts m5.xlarge, which has 4 vCPU
(EC2 general purpose instance types),
while the container requests cpu: "8" and memory: "16Gi" at :103-105 — more memory than an
m5.xlarge has in total once kubelet overhead is subtracted. That branch of the selector can never
schedule, so the Job sits Pending if it is the only match. [confirmed]

Suggested change

	values: ["p6-b200.48xlarge", "ml.p6-b200.48xlarge", "m5.xlarge", "m5.8xlarge"]
	values: ["p6-b200.48xlarge", "ml.p6-b200.48xlarge", "m5.8xlarge"]

[mvinci12]

Fixed in b96489c, exactly as suggested — m5.xlarge removed, leaving the two B200 forms
and m5.8xlarge. The comment records the arithmetic: 4 vCPU against cpu: "8" and less
total memory than the 16Gi request.

[KeitaW]

check_merge_parity.py docstring points at a script and a flag that don't exist

The copy-pasteable command in the docstring passes --max-new-tokens 0, which parse_args
(:43-58) does not define — argparse exits 2 with "unrecognized arguments". :5 also credits the
merged model to scripts/merge_megatron_lora_ckpt.py, which is not in this PR; the merger is
scripts/merge_adapter.py.

Suggested change

	--max-new-tokens 0 --num-prompts 4
	--num-prompts 4

…and Confirms the merged HF model produced by scripts/merge_adapter.py is at :5.

[mvinci12]

Both fixed in b96489c. merge_megatron_lora_ckpt.py → merge_adapter.py, and
--max-new-tokens 0 deleted rather than implemented, since the checks are logit-only and
never generate. Your observation that argparse prints this same docstring as its
description is what makes it more than cosmetic — --help was emitting a command its own
parser rejects. Verified both directions: the old flag is rejected, the corrected command is
accepted.

[KeitaW]

Stale version claims and an internal contradiction in AGENTS.md

This comment credits the image with "vLLM 0.12.0 with the Blackwell PDL patch (see Dockerfile)".
The Dockerfile now builds on vllm020.dev2 (vLLM 0.20.2) and says at :129-133 that the sed patch
was replaced by the VLLM_LORA_DISABLE_PDL=1 env var.

Suggested change

	# - vLLM 0.12.0 with the Blackwell PDL patch (see Dockerfile)
	# - vLLM 0.20.2 with the Blackwell PDL workaround (VLLM_LORA_DISABLE_PDL, see Dockerfile)

AGENTS.md:192-193 also claims ".gitignore contains env_vars and nothing else" — the committed
.gitignore has ten entries. A third copy of the project's facts drifts from the other two
quickly; it is part of why Batch 1 suggests dropping the file.

[mvinci12]

Fixed in b96489c: the header now reads vLLM 0.20.2 with the VLLM_LORA_DISABLE_PDL
workaround, matching Dockerfile:129-134.

The AGENTS.md:192-193 .gitignore claim resolved itself — the file is gone. But your
closing point stands on its own merits and is the reason I dropped it rather than fixing it:
a third copy of the same facts drifts, and it already had.

[KeitaW]

Review Batch 6/6 — Things that look great, and sources

Things that look great

• docs/results.md is the best evaluation write-up I have reviewed in this repo. A
  pre-registered primary endpoint with a stated win condition (≥ +0.02 at ≥ 1.96σ), a Bonferroni
  bar, and a headline result that reports its own hypothesis as unsupported ("Expert-FFN
  placement — the Run-4 hypothesis — is NOT SUPPORTED. 6/6 matched internal nulls") is genuinely
  rare. So is naming the confound in the same paragraph as the claim it weakens. Insight #1 —
  "internal in-distribution MATH moved +0.0784; public math moved roughly 3× less … never +0.0784
  unqualified" — is the kind of thing most write-ups quietly omit.
• Validating a benchmark's own gold answers through your scorer before spending GPU (insight
  #2) caught two bad instruments at zero cost, and the MATH-500 handling is exactly right: keep the
  paired deltas, restrict absolutes to the n=472 scoreable subset, and say why.
• The train/test overlap audit in conf/data/mixed-valplus.yaml:23-45 — prompt-hashing the val
  split against train, finding 74.8% CODE contamination (apps 93.3%), and demoting CODE to a
  secondary "train-set fit" readout as a result — is a measurement decision most projects would
  never make against their own headline number. It also retro-explains the internal-CODE vs
  HumanEval gap, which is a satisfying piece of work.
• scripts/test_reward_routing.py's negative control. Check 2 asserts that numina_cn_k12_h2
  is not scored as numina_olympiads — proving the suffix-strip is load-bearing rather than just
  asserting the happy path. Codex ran the suite on 2026-08-18: 41/41 pass, no GPU, no network.
  Pairing an offline test with the data change it protects is the right instinct.
• NCCL_SOCKET_IFNAME=^docker,lo,veth (Dockerfile:76) is the repo's canonical exclusion form —
  matching 3.test_cases/megatron/megatron-bridge/Dockerfile:265 and five siblings — rather than
  the eth0/eni positive selection that breaks on EFA instances. Consistently the most-flagged
  issue on this repo, and this PR gets it right without being asked.
• The TCP-only livenessProbe on sandbox-fusion (:121-127), with the death-spiral reasoning
  written down: an HTTP liveness check that kills an overloaded-but-working pod cascades into empty
  Service endpoints and zeroed code rewards. Keeping readiness on HTTP so a slow pod sheds load,
  while liveness only catches a truly wedged process, is the correct split and the comment explains
  why the obvious configuration is wrong.
• The pinning discipline in Dockerfile and scripts/runtime_env.yaml — every pin carries the
  constraint that forced it (transformers>=5.8.1,<5.9.0 reconciling Megatron-Bridge v0.5.0 against
  vLLM 0.20.2's exclusions; Ray downgraded to 2.53.0 to match the KubeRay head; megatron-core==0.18.0
  for megatron.core._rank_utils.safe_get_world_size). Batch 4's finding is that the pod-startup
  installs don't match this standard — the image layer sets the bar.
• scale_ray_workers.sh's indexed JSON patch, including patching maxReplicas alongside
  replicas/minReplicas because the autoscaler clamps to it and a group left at maxReplicas=0
  silently refuses to scale back up. That is a real KubeRay behaviour that costs an hour to
  discover, and resolving the group index by name rather than assuming [0] is the careful form.
• The OTel opt-out annotations on sandbox-fusion (:46-54) with the measurement attached — a
  bare print(42) taking 10.4 s and timing out, zeroing every code reward. Encoding an
  environment-specific footgun as configuration plus one comment is the ideal outcome for that
  class of bug.

Sources

Repo precedent and files cited above:

• CONTRIBUTING.md — pinning rule, KISS/self-containment, directory conventions
• 3.test_cases/pytorch/verl/kubernetes/rlvr/ and .../hyperpod-eks/rlvr/ — existing verl GRPO-on-EKS recipes and the verl/<platform>/<recipe>/ layout
• 3.test_cases/megatron/megatron-bridge/Dockerfile:265 (+5 siblings) — canonical NCCL_SOCKET_IFNAME exclusion list
• 3.test_cases/pytorch/dreamzero/kubernetes/libero/secret.example.yaml — HF_TOKEN via Secret
• 3.test_cases/pytorch/FSDP/kubernetes/*.yaml — the plaintext-HF_TOKEN precedent this review does not treat as a blocker

Official documentation:

• Kubernetes — Deployment progressDeadlineSeconds (default 600 s)
• AWS — EC2 general purpose instance types (m5.xlarge = 4 vCPU)
• KubeRay — Ray pod labels (ray.io/node-type)

Checks run against this branch, 2026-08-18:

• verified live — Hydra 1.3.5 / OmegaConf 2.3.1: a repeated dotted CLI override is applied last-wins, not rejected (actor.ppo_mini_batch_size=16 … =24 → 24)
• verified live — set -xeuo pipefail followed by source env_vars echoes every export in the sourced file, including HF_TOKEN, to stderr
• verified live — grep -rn parallelism-strategies over the contribution matches only AGENTS.md:53; no doc embeds the SVG
• verified live — 53 of 85 new files lack the SPDX-License-Identifier: MIT-0 header (git ls-files + head -5 scan)
• verified live — compare_eval_results.py run against two nonexistent runs exits 0 and prints a complete markdown report with empty tables
• verified live — scripts/test_reward_routing.py: 41/41 checks pass offline
• verified live — grep for VLLM_EXECUTE_MODEL_TIMEOUT_SECONDS, S3_BUCKET_NAME, RAY_memory_usage_threshold, NCCL_IB_DISABLE, num_efa_per_node, head_instance_type, forward_prefetch finds zero consumers outside their own definitions

[KeitaW]

Thank you! Few comments.

[mvinci12]

Thank you — this was a useful review, and the two [confirmed] must-fixes were
real: the quickstart genuinely did not run as written. I reproduced every finding before
changing anything, and everything is addressed. Five commits, one per batch, plus a
follow-up for something the verification turned up.

Your five structural asks

• Placement — took the second option: git mv to
  verl/kubernetes/grpo-megatron-lora/, so the tree keeps one nesting depth under verl/.
  The README now states plainly what this adds over kubernetes/rlvr (Hydra config
  surface, LoRA to 235B MoE on Megatron-Bridge, the eval pipeline, the sandboxed
  code-reward path) and links rlvr for the shorter GRPO/DAPO path and its KubeRay setup.
• lustre/ and the cluster group — both dropped. The FSx PVC is now a stated
  prerequisite linking 1.architectures/7.sagemaker-hyperpod-eks and its fsx_lustre
  module. The seven values submit_training.py actually read became compute.* keys in
  conf/config.yaml; I renamed it from cluster because your objection was substantive,
  not just about the group mechanism. Four keys nothing read went with it.
• Six download scripts — one parameterised models/download_model.sh remains.
  run_on_cluster.sh now forwards arguments (printf %q-quoted); it used to shift them
  away, so a parameterised script would have received none.
• Files with no consumer — LICENSE, AGENTS.md, bigcodebench_p4.yaml and
  eval-mlflow-log-job.yaml are gone. AGENTS.md's genuinely unique content was migrated
  first: shell/Dockerfile conventions into docs/configuration.md, the self-test gates
  into the README — including the grep -c PASS overcount, which is worth keeping.
• The run-diary — the sandbox-concurrency narrative moved to docs/results.md as
  "Sandbox concurrency and OOM (unresolved)", keeping the retracted first model, both
  ruled-out hypotheses and the negative result at semaphore 5. Both source sites keep the
  one-line invariant. custom_reward_fn.py drops from 42% comments to ~30%.

Two things worth flagging, because they go beyond what you asked

1. Your ppo_mini_batch_size finding had a second victim. Emitting the key once
   exposed that qwen25-72b and qwen3-coder-next (both TP=4/PP=4 → dp=3) give
   effective=21 against per_dp=128, so 128 % 21 != 0 — the same crash the
   qwen3-30b-a3b override exists to prevent, on two models conf/config.yaml and the
   README both document as supported. The hardcoded 24 had been masking it on FSDP. I
   enumerated the arithmetic across all six models on both backends: 24 as the global
   default is valid everywhere at both 1 and 6 nodes, so that is now the default. I could
   not test these on hardware, so the real deliverable is
   _validate_batch_divisibility(), which fails at submit with the numbers printed
   instead of several minutes into step 3.

2. My own SPDX fix introduced a bug, and my gate did not catch it. Prepending the
   header pushed # @package _global_ to line 3 in the 12 group files that use it. Hydra
   only honours that directive on line 1, so those groups silently re-nested one level and
   cfg.profiling.tool would have raised on every run. --cfg job --resolve exited 0
   throughout, because it only composes and prints the config and never calls main()'s
   body — the same "a check that cannot fail is not a check" shape you flagged in three
   other places. Fixed in 7287ccc, and the gate replaced with one that composes each
   combination and actually runs print_config_summary() and build_verl_overrides(),
   asserting ppo_mini_batch_size is emitted exactly once. 42 combinations now go through
   the real code path.

On pinning, one deliberate non-uniformity. You suggested
torch==<the CPU torch you validated>. I could not determine that from the repo, so I went
looking: the results.json written by lm-eval inside the driver pod on the reported code
runs records torch 2.13.0+cpu, transformers 4.49.0, numpy 2.4.6, Python 3.11.15.
Those are pinned exactly, and the driver images to python:3.11.15-slim — the patch tag
python:3.11-slim resolved to at the time. For everything else no run recorded the resolved
version (every install redirected to a /work/pip.log on an emptyDir), so those are
bounded ranges in the style runtime_env.yaml already uses, and each block says which of
the two it is. I would rather ship a range than an == that looks like a measurement and
is not. Two caveats: only 5 of 12 eval runs saved a results.json, and the three MATH arms
ran lm-eval 0.4.9.2 with torch/transformers unrecorded.

Two suggestions I did not take as written

• The sandbox anti-affinity: fixed the label to ray.io/node-type, but left both terms
  preferred rather than promoting to required. You were right that a privileged executor
  argues for required, but I cannot verify node counts on the target cluster and a
  required term that cannot schedule fails worse than a soft one. docs/cluster.md now
  has a "Blast radius" section tabulating what is and is not enforced, naming a
  NetworkPolicy and a tainted node group as the two additions worth making, and giving
  the --show-labels command to confirm the selector matches.
• --max-new-tokens in check_merge_parity.py: deleted from the docstring rather than
  implemented. The checks are logit-only and never generate, so the flag had no meaning.

Verification (all offline, no cluster, no GPU): ruff clean under
--select E,F,UP,B,I,G; py_compile on 15 Python files; bash -n on 12 shell scripts
and on the 4 shell programs embedded in manifest command/args blocks — that last
check is new and it caught a comment that would have swallowed a line continuation in the
vLLM invocation; 34 YAML files parse; SVG well-formed; all three self-test gates ALL GREEN
(41/41 reward routing after the raise-instead-of-degrade change); 42 config combinations
through build_verl_overrides; SPDX scan reports 0 missing; 0 @package directives
displaced. Behaviour I checked by execution rather than inspection: compare_eval_results.py
against two nonexistent runs now exits 1 instead of printing a full report;
deploy_vllm_eval.sh --delete works with REGISTRY unset; submit_data_prep.sh exits
cleanly with no TTY and proceeds with --assume-yes; an exported RAY_ADDRESS survives
sourcing env_vars.example; KUBE_NAMESPACE=verl propagates into the sandbox URL; and the
old --max-new-tokens is rejected by argparse while the corrected command is accepted.

I also re-checked that everything in Batch 6 survived the refactor — the
NCCL_SOCKET_IFNAME exclusion form, the TCP-liveness/HTTP-readiness split and its
reasoning, the scale_ray_workers.sh maxReplicas patch, the OTel opt-out annotations, the
runtime_env.yaml pins with their constraints, the test_reward_routing.py negative
control, the contamination audit, and docs/results.md's pre-registered primary. 15 checks,
all intact.

[KeitaW]

Review Batch 1/4 — New defects introduced by the fixes

Round 1's headline was that the documented quickstart did not run. That specific cause is fixed.
The consolidation work introduced a new one at the next step, and the new validator invalidated
three documented commands nobody re-ran. Both are the same shape as the thing they replaced, which
is worth saying plainly: the fixes were good, the surface the fixes changed did not get re-walked.

[KeitaW]

The quickstart's model-download command fails with exit 2

Must fix. Collapsing the six download_qwen*.sh scripts was right, but the Quickstart still
passes the old two-argument signature. submit_download.sh:60-65 takes MODEL="$1", shifts, and
forwards the rest verbatim to download_model.py — which defines --model, --output-dir,
--revision and --skip-verify, and no positional argument. Reproduced against this branch,
2026-08-19: [confirmed]

$ python3 models/download_model.py --model Qwen/Qwen3-8B /fsx/data/verl/models/Qwen3-8B
usage: download_model.py [-h] --model MODEL [--output-dir OUTPUT_DIR] [--revision REVISION] [--skip-verify]
download_model.py: error: unrecognized arguments: /fsx/data/verl/models/Qwen3-8B
$ echo $?
2

Step 4 of the Quickstart therefore fails. The same two-argument signature is repeated in
docs/configuration.md:861 and models/download_model.sh:11.

Suggested change

	./models/submit_download.sh Qwen/Qwen3-8B /fsx/data/verl/models/Qwen3-8B
	./models/submit_download.sh Qwen/Qwen3-8B

download_model.py already defaults the destination to /fsx/data/verl/models/<name>; pass
--output-dir if you want a different base. Worth fixing all three call sites in one go —
grep -rn 'submit_download.sh' . finds them.

[KeitaW]

Three documented commands now hard-abort at the new validator

_validate_batch_divisibility() is a good addition and its error messages are genuinely helpful.
But adding a gate makes every previously-printable command a claim to re-check, and three worked
examples in this file now abort before submitting. I computed each from the shipped config
(train_batch_size, n_responses_per_prompt=4, ppo_micro_batch_size_per_gpu=2, 48 GPUs,
qwen25-72b at TP=4/PP=4/CP=1 → dp=3): [confirmed]

Doc	Command	Validator result
:293 (this block)	backend=megatron model=qwen25-72b … train_batch_size=256	dp=3, effective=32, per_dp=341 → 341 % 32 = 21 → ABORT
:358	backend=megatron model=qwen25-72b lora=disabled … train_batch_size=128	per_dp=170 → 170 % 32 = 10 → ABORT
:659	training.ppo_mini_batch_size=64 training.train_batch_size=256 (FSDP)	effective = 64·4//48 = 5 → 5 % 2 = 1 → ABORT

At dp=3 a train_batch_size divisible by 3 keeps per_dp a multiple of effective — 96 (the
default), 192 or 384 all work with ppo_mini_batch_size=24. For the FSDP example, 24 or 48 satisfy
effective % micro == 0.

Suggested change

	python3 scripts/submit_training.py \
	python3 scripts/submit_training.py \

(the numbers below this line are what need changing — training.train_batch_size=192 works at dp=3)

The general point for the next change like this: a validator that rejects a configuration is also a
statement about every example in the docs, and docs/configuration.md:507-511 documents the
validator without re-walking the sections above it.

[KeitaW]

Review Batch 2/4 — Fixes applied to one side of a pair

Three cases where a correction landed on one file or one caller and its sibling kept the old
behaviour. These are the easiest kind to miss in a large revision and the easiest to close.

[KeitaW]

HF_REVISION cannot be set on the only path documented to use it

download_model.sh:24-25 introduces HF_REVISION as the reproducibility pin — "Unset means the
mutable main branch, which is not reproducible" — and its own header (:9-14) says it runs
only via run_on_cluster.sh. But this generator writes the remote env_vars with exactly two
variables, and HF_REVISION is not one of them: [confirmed]

135:    echo "export RAY_DATA_HOME=\"${RAY_DATA_HOME:-/fsx/data/verl}\""
136:    echo "export HF_TOKEN=\"${HF_TOKEN:-}\""

So HF_REVISION=<sha> ./models/run_on_cluster.sh models/download_model.sh Qwen/Qwen3-8B downloads
main and prints WARNING: HF_REVISION unset every time. kubernetes/vllm-eval.yaml:157 sends the
operator to that knob to make the weights auditable.

Suggested change

	echo "export HF_TOKEN=\"${HF_TOKEN:-}\""
	echo "export HF_TOKEN=\"${HF_TOKEN:-}\""
	echo "export HF_REVISION=\"${HF_REVISION:-}\""

(The Ray path is fine — submit_download.sh forwards extra args, so --revision works there.)

[KeitaW]

The readiness probe's own caller still waits on the wrong condition

fsx-utils.yaml:57-63 gained the readinessProbe on /tmp/ready, with the rationale that
kubectl wait --for=condition=Ready used to return before pip install finished so a script
exec'ing in could find datasets missing. That is exactly right — but this script, which dispatches
those scripts into the pod, gates on .status.phase instead: [confirmed]

86:    -o jsonpath='{.status.phase}' 2>/dev/null) || true
98: if [ "${POD_PHASE}" != "Running" ]; then

Running is true as soon as the container starts, which is the condition the probe was added to
stop trusting. The race the probe closes on the kubectl wait path is still open on the dispatch
path — and this is the path the README uses for model staging.

Suggested change

	if [ "${POD_PHASE}" != "Running" ]; then
	if ! kubectl wait --for=condition=Ready "pod/${FSX_UTILS_POD}" \
	-n "${KUBE_NAMESPACE}" --timeout=300s >/dev/null 2>&1; then

…reporting the pod's phase in the failure message as it does today.

[KeitaW]

valeval-job.yaml did not get the hardening its sibling got

The lm-eval isolation work is solid: lmeval-job.yaml gained automountServiceAccountToken: false
and a read-only /fsx with a narrow writable subPath, and sandbox-fusion.yaml and
fsx-utils.yaml both got the token flag. valeval-job.yaml got neither — measured across the four:
[confirmed]

Manifest	automountServiceAccountToken: false	/fsx read-only
lmeval-job.yaml	yes	yes
valeval-job.yaml	no	no

To be fair to the change: valeval routes code execution to the sandbox service, so the
"local executor" argument that drove the lmeval fix does not literally apply here. But it
pip installs verl from a git URL, runs custom_reward_fn, and is now the one pod in the eval path
holding both a projected API token and read-write access to the whole FSx volume — including the
checkpoints and every other run's results. The same two-line treatment applies verbatim:

Suggested change

	spec:
	spec:
	automountServiceAccountToken: false

…plus the read-only /fsx + writable /eval-out subPath split copied from lmeval-job.yaml:175-180.

[KeitaW]

Review Batch 3/4 — Drift and smaller edges

Smaller items

• README.md:67 — "A 1-node config is included for smoke tests." conf/cluster/p6-b200-1node.yaml
  was deleted along with the group; the 1-node path is now the compute.num_nodes=1 override that
  README.md:149 documents correctly. Reword to match line 149.
• docs/configuration.md:574 and :620 — prose still says "the cluster config group" /
  "cluster.* Hydra paths". The code blocks directly below both use compute.* correctly.
• conf/model/qwen3-235b.yaml:103-107 — the comment defending ppo_mini_batch_size: 16 shows
  "Default (16): 16 * 4 // 4 = 16 → 64 % 16 = 0". 16 is no longer the default (conf/config.yaml
  is 24) and per_dp is 96*4//4 = 96, not 64. The override is still correct — 96 % 16 = 0 — but
  the arithmetic shown is from the pre-rewrite config. The KV-cache block at :52-55 likewise still
  reasons at max_model_len=18432 while the config is now 24576 (+2048 prompt = 26624).
• kubernetes/lmeval-tasks/utils.py:215-280 — _BCB_ENTRY, build_predictions_bcb,
  _bcb_candidate and reference_bcb are orphaned now that bigcodebench_p4.yaml is gone; no task
  YAML or test references them, and docs/results.md:354 says the task is deliberately not shipped.
  ~65 lines that can go with it.
• README.md config table — backend and model read as a free cross-product, but
  model=qwen3-235b backend=fsdp now aborts at the validator (its ppo_mini_batch_size: 16 gives
  effective = 16·4//48 = 1, which fails % micro). conf/backend/fsdp.yaml:4 says "up to ~72B";
  the table doesn't. The abort message is excellent and self-explaining, so this is just a
  discoverability line in the table.
• scripts/submit_training.py — preflight_resume_check still reads its exec pod and namespace
  from cfg.sandbox.preflight, so sandbox=disabled (which has no preflight key) makes every
  submit print "resume target could not be resolved". The probe pod is fsx-utils and has nothing
  to do with the sandbox; a top-level kube: block in conf/config.yaml would decouple them.
• models/run_on_cluster.sh:306-318 — REMOTE_EXIT=$? after the kubectl exec is unreachable
  under the set -euo pipefail at :146: a non-zero exec terminates the script before the
  assignment, so the FAILED: ... exited with code N branch never prints. The exit code still
  propagates, so this costs the diagnostic, not correctness. Pre-existing, but this block was edited
  for argument forwarding. if ! kubectl exec ...; then REMOTE_EXIT=$?; ... restores it.

[KeitaW]

docs/cluster.md's fsx-utils section is stale in three ways, one of which wedges the pod

fsx-utils.yaml:83-87 now takes HF_TOKEN from a non-optional secretKeyRef — the right fix. This
procedure was not updated with it: [confirmed]

• No hf-token Secret step. Without it the pod sits in CreateContainerConfigError and the
  kubectl wait --for=condition=Ready on :257 times out with nothing explaining why. README.md:93
  has the kubectl create secret generic step; this file does not.
• :260 says python:3.12-slim; the manifest pins python:3.11.15-slim (fsx-utils.yaml:42).
• :261 says it mounts fsx-claim; every manifest uses claimName: fsx-lustre — and
  docs/cluster.md:91, in this same file, explains why it is fsx-lustre.

Suggested change

	envsubst < kubernetes/fsx-utils.yaml | kubectl apply -f -
	kubectl create secret generic hf-token \
	-n "${KUBE_NAMESPACE}" --from-literal=HF_TOKEN="${HF_TOKEN}"
	envsubst < kubernetes/fsx-utils.yaml | kubectl apply -f -

models/run_on_cluster.sh:36 and :93 print the same deploy recipe without the Secret step.

[KeitaW]

compare_eval_results.py: the strict machinery has a dead branch and inverts its own docstring

The substance of round 1's finding is fixed — a missing run now exits 1 instead of printing a
plausible empty table, and I confirmed that behaviour. Two leftovers in the plumbing: [confirmed]

The strict=True branches are unreachable. _load_lmeval_metrics:95 and _load_val_split:132
each raise FileNotFoundError under if strict:, but their only caller — _load_run:168 — passes
strict=False unconditionally. The strict: bool = True defaults on :80 and :123 are never
exercised; all the real gating happens at _load_run:172-179. That works, but it leaves two
parameters and two raise branches that no input can reach.

Curve mode does the opposite of what this line promises. It says strict=False is "used by
--curve, where an intermediate step legitimately may not have been evaluated" — but main() calls
load() → _load_run(...) at the strict default for every step, so a curve over
--steps 50,350,750 aborts if 350 has not been evaluated yet. That is the normal mid-run case, and
before this change it rendered as dashes.

Either implement the documented behaviour (pass strict=False for curve steps, keep strict for
--run and --baseline) or drop the promise from the docstring. I'd suggest the former — the
docstring describes the more useful tool.

[KeitaW]

Review Batch 4/4 — Resolution, what stands out, and sources

Resolution scoreboard — 33 of 33

Every finding from round 1 is resolved. I checked each against the tree rather than against the
reply, and all 33 hold up. The structural ones:

Round-1 finding	Verified
Placement / layering	Moved to verl/kubernetes/grpo-megatron-lora/; lustre/ and the cluster group deleted; README Prerequisites table links 1.architectures/7.sagemaker-hyperpod-eks and its fsx_lustre module. All relative links resolve.
Reward fn loaded from a path nothing stages	Now /workspace/custom_reward_fn.py, the image copy
Namespace handling	namespace: ${KUBE_NAMESPACE} on the sandbox Deployment + Service; the URL interpolates ${oc.env:KUBE_NAMESPACE,default}
ppo_mini_batch_size emitted twice	Emitted once from the shared builder; 0 duplicate keys across all model×backend pairs
Inert ray-node-type selector	ray.io/node-type in both manifests, with the reasoning recorded
Resume preflight failing open	probe_failed distinguishes "no tracker" from "probe did not run"
Val-split token budgets diverging	default=24576, and submit_val_eval.sh passes it explicitly
secret-in-xtrace	set -euo pipefail, with a comment explaining why xtrace is deliberately absent
HF_TOKEN in the pod spec	secretKeyRef on a hf-token Secret, per the dreamzero precedent
53 files missing SPDX	0 missing across every tracked file
Unpinned installs	torch==2.13.0+cpu, transformers==4.49.0, python:3.11.15-slim; imagePullPolicy: IfNotPresent everywhere
Files with no consumer	LICENSE, AGENTS.md, bigcodebench_p4.yaml, eval-mlflow-log-job.yaml gone; the SVG is now embedded in docs/configuration.md:506 with descriptive alt text
Six download scripts	One parameterised script; run_on_cluster.sh forwards args printf %q-quoted

And the round-1 positives all survived the refactor — NCCL_SOCKET_IFNAME=^docker,lo,veth, the
TCP-liveness/HTTP-readiness split, scale_ray_workers.sh's maxReplicas patch, the OTel opt-out
annotations, the reward-routing negative control, the contamination audit, and docs/results.md's
pre-registered primary.

Things that look great

• You found a bug in your own fix and reported it against yourself. Prepending the SPDX header
  pushed # @package _global_ off line 1 in 12 group files, silently re-nesting those groups.
  Catching that, naming why the existing gate missed it — --cfg job --resolve composes and prints
  but never runs main()'s body — and then replacing the gate with one that runs
  build_verl_overrides() across 42 combinations is a better outcome than the original fix. It is
  also the same "a check that cannot fail is not a check" shape from round 1, applied by you to your
  own work. I re-verified: @package _global_ is on line 1 in every file that has it, and present
  on exactly the groups that need it.
• Chasing the ppo_mini_batch_size literal to its cause rather than deleting it. Removing the
  hardcoded 24 exposed that qwen25-72b and qwen3-coder-next (both dp=3) fail at the old default
  of 16. I reproduced your arithmetic independently: effective = 16·4//3 = 21, per_dp = 96·4//3 = 128,
  128 % 21 = 2 — so two models the README documents as supported would have crashed at step 3.
  I also enumerated all 6 models × 2 backends × {1,6} nodes: 24 is valid everywhere it applies,
  and the single remaining non-OK combination (qwen3-235b + FSDP) is the one your validator rejects
  at submit with a message that names the cause and the fix. That is a better deliverable than a
  tested value, and you said so yourself.
• Going after the resolved versions instead of inventing pins. Recovering
  torch 2.13.0+cpu / transformers 4.49.0 / Python 3.11.15 from the results.json written inside
  the driver pod, pinning exactly where a run recorded the value and using bounded ranges where none
  did, and labelling which is which — plus stating that only 5 of 12 runs saved a results.json
  and the three MATH arms recorded nothing. "I would rather ship a range than an == that looks like
  a measurement and is not" is the right instinct.
• bash -n on shell embedded in manifest command/args blocks. A check nobody asks for, and
  it caught a comment that would have swallowed a line continuation in the vLLM invocation. Worth
  keeping in the self-test section permanently.
• Declining two suggestions with reasons. Leaving the sandbox anti-affinity preferred because
  a required term that cannot schedule fails worse — and then writing the "Blast radius" section
  in docs/cluster.md tabulating what is and is not enforced, naming the NetworkPolicy and tainted
  node group as the additions worth making — is a better answer than complying. Same for deleting
  --max-new-tokens from the docstring rather than implementing a flag the checks have no use for.

Sources

• CONTRIBUTING.md — pinning rule; the architecture-dependency allowance
• 3.test_cases/pytorch/FSDP/kubernetes/README.md:10, dreamzero/README.md:74,
  megatron/nemo1.0/EKS/README.md:8 — the cluster/FSx prerequisite-and-link precedent this now follows
• Kubernetes — Deployment progressDeadlineSeconds
• Ray — KubeRay pod labels (ray.io/node-type)

Checks run against 7287ccc, 2026-08-19:

• verified live — download_model.py --model Qwen/Qwen3-8B /fsx/... exits 2, unrecognized arguments
• verified live — the three docs/configuration.md commands abort, arithmetic recomputed from the shipped config
• verified live — all 6 models × 2 backends × {1,6} nodes enumerated against the validator's rules; 24 valid everywhere, qwen3-235b+FSDP the one intended abort
• verified live — @package _global_ on line 1 in every file that has it; 0 files missing SPDX
• verified live — HF_REVISION absent from the env_vars block run_on_cluster.sh generates
• verified live — automountServiceAccountToken/readOnly present in lmeval-job.yaml, absent in valeval-job.yaml
• verified live — --delete branch precedes the REGISTRY:? requirement; KUBE_NAMESPACE set before it
• verified live — every relative Markdown link resolves; zero references to conf/cluster, cfg.cluster., lustre/, or the pre-move path
• verified live (independent second pass) — 48 Hydra combinations compose and build overrides; no duplicate CLI keys; self-tests 41/41, 13/13, 36/36; ruff clean