Quote the model args miles inlines into the launch command - #1911
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
4583664 to
cf656e3
Compare
ad36ea8 to
0cf03d3
Compare
cf656e3 to
740e031
Compare
72db26b to
c99f071
Compare
740e031 to
cbc0762
Compare
Failure
Classification: ours, pre-existing, previously maskedNot a flake and not infra — but also not introduced by the rebase.
FixPushed into The harness already freezes env vars, the run id, the PID and generated pseudo-files so a recording cannot depend on the machine. It now freezes the filesystem the same way: while an entrypoint records, only the pytest sandbox is visible and every other path reports absence. That removes the whole class in one place (~20 probe sites across 16 launchers) instead of per-script overrides, and it also fixes a latent non-determinism — a machine that happens to carry Two regression tests cover it, including the "unreadable parent must report absence instead of raising" case. Status: https://github.com/radixark/miles/pull/1911/checks |
c99f071 to
d8234df
Compare
|
| lane | result |
|---|---|
stage-a-cpu (0) / run-cpu |
SUCCESS |
stage-a-cpu (1) / run-cpu |
SUCCESS |
stage-a-cpu (2) / run-cpu |
SUCCESS |
stage-a-cpu (3) / run-cpu |
SUCCESS |
stage-b-cpu / run-cpu |
SUCCESS |
One correction to my previous comment: the first version of that fix hid the checkout as well, which broke load_model_args (AssertionError: no model args script at .../scripts/models/deepseek-v4-flash.py, job). The landed version keeps the checkout visible and hides only paths outside it and the sandbox.
stage-c-8-gpu-h100 (0) failure — not from this PR
Job, runner novita-host4, image radixark/miles:dev. tests/e2e/megatron/test_qwen3_5_35b_a3b_lora_ci.py dies inside a real training step:
RuntimeError: /TransformerEngine/transformer_engine/common/fused_attn/fused_attn_f16_arbitrary_seqlen.cu:934
cuDNN Error: detail::set_attribute(reshape_operation.get_raw_desc(), CUDNN_ATTR_OPERATION_RESHAPE_MODE,
CUDNN_TYPE_RESHAPE_MODE, 1, &cudnn_reshape_mode) failed ... code: CUDNN_STATUS_BAD_PARAM
Two independent reasons this is not the PR's change:
-
The effective argv is unchanged. This PR's entire effect on that launch command is one pair of quotes, which the snapshot pins:
- --moe-layer-freq [1,1,...,1] + --moe-layer-freq '[1,1,...,1]'
Unquoted,
[1,1,...,1]is a bracket glob matching one character from{1, ,}; the launch cwd (/__w/miles/miles) contains no file named1or,, so bash passed it through literally. Megatron receives the identical string either way — the quotes only remove the latent glob hazard. Nothing in that diff can reach a cuDNN fused-attention descriptor. -
Every sibling PR passes the same lane on the same image. The whole M8 chain was rebased onto the same
mainthis morning, and shard 0 of this suite isSUCCESSon #1901, #1902, #1903, #1904, #1905, #1906, #1907, #1908, #1909 and #1910, as well as on unrelated PRs #1912, #1913, #1916, #1919 and #1921. Only this PR's shard 0 is red, and it differs from Replace the model config shell scripts with python #1910 by nothing but those quotes.
Classification: flake / host-specific cuDNN failure, most likely on novita-host4. Next step: let the remaining nine GPU jobs of this round finish (so live siblings are not cancelled), then rerun the failed job and confirm it turns green. I have not touched the code for it.
Status: https://github.com/radixark/miles/pull/1911/checks
RetractionI claimed the sibling PRs "pass the same lane on the same image". That evidence is vacuous and I withdraw it. Only this PR carries the — which I reproduced myself by running that exact command. So those Consequence I should have stated: this lane has never had real coverage anywhere on the M8 chain, so the failure cannot be pinned on the tip commit by elimination — any op in the chain would have been equally untested here. What still stands
Taken together: the launch command is provably unchanged from the Next step unchangedSix I have deliberately not touched the Status: https://github.com/radixark/miles/pull/1911/checks |
Both failures are pre-existing on
|
| test | main nightly 2026-07-28 |
this PR |
|---|---|---|
tests/e2e/megatron/test_qwen3_5_35b_a3b_lora_ci.py |
FAIL exit=1 (stage-c-8-gpu-h100 (0)) |
FAIL (stage-c-8-gpu-h100 (0)) |
tests/e2e/short/test_qwen3_0.6B_fsdp_colocated_2xGPU.py |
timeout after 1800s (stage-c-2-gpu-h200) |
timeout after 1800s (stage-c-2-gpu-h200 (0)) |
The h100 fingerprint is byte-identical on both sides — the same cuDNN assertion at the same source line:
RuntimeError: /TransformerEngine/transformer_engine/common/fused_attn/fused_attn_f16_arbitrary_seqlen.cu:934
cuDNN Error: detail::set_attribute(reshape_operation.get_raw_desc(), CUDNN_ATTR_OPERATION_RESHAPE_MODE,
CUDNN_TYPE_RESHAPE_MODE, 1, &cudnn_reshape_mode) failed ... code: CUDNN_STATUS_BAD_PARAM
The fsdp one is not an assertion at all — it is the 1800s cap on a test declared est_time=300, hit on both sides.
Note these are not random flakes: they reproduce on main. I am therefore not rerunning them — a rerun cannot be greener than main, and it would occupy an 8×H100 runner for an hour to re-learn the same fact. Six of the last eight nightlies on main are red.
Positive evidence for this chain
The one test in these lanes that directly exercises what M8 changed — tests/e2e/megatron/model_scripts/test_glm5_744b_a40b_4layer_r3.py, which runs a model script that op8-11 converted from .sh to .py — passed on this PR, as did tests/e2e/fsdp/test_qwen3_4B_fsdp_true_on_policy.py and both stage-c-8-gpu-h200 shards.
Additionally tests/snapshots/model_args/ has a zero-byte diff from op8-17 (where the .sh-era args were frozen) through this PR's head, so the arguments reaching train.py are provably identical to the pre-M8 baseline.
Current state of this PR
stage-a-cpu ×4, stage-b-cpu, stage-b-2-gpu-h200, stage-c-8-gpu-h200 ×2, stage-c-8-gpu-h100 (1), stage-c-2-gpu-h200 (1), stage-c-4-gpu-h200 (1) all pass. Two stage-c-4-gpu-h200 shards are still queued behind a saturated fleet; the two failures above are the pre-existing main ones.
Per the user's standing instruction I am not filing an issue for the two main failures — flagging them here and to the user instead.
Status: https://github.com/radixark/miles/pull/1911/checks
98b4499 to
d8234df
Compare
The failure
The payload decodes to Root cause: an undeclared cross-repo dependencyop8-9 replaced the temp-file arguments with inline This PR's description declared no dependency pins, so
FixAdded to the PR description, per the mechanism in Since the workflow reads the description from the event payload, a re-run would have replayed the stale body; I pushed an empty commit and immediately reverted it so a fresh Two things for the human to decide
The other two failures are unchanged and still reproduce on Status: https://github.com/radixark/miles/pull/1911/checks |
The
|
| lane | result |
|---|---|
stage-a-cpu ×4, stage-b-cpu |
pass |
stage-b-2-gpu-h200 |
pass |
stage-c-2-gpu-h200 (1) |
pass |
stage-c-4-gpu-h200 (0), (2) |
pass |
stage-c-8-gpu-h100 (1) |
pass |
stage-c-8-gpu-h200 (0), (1) |
pass |
stage-c-4-gpu-h200 (1) |
still running |
stage-c-8-gpu-h100 (0) |
fails — pre-existing on main |
stage-c-2-gpu-h200 (0) |
fails — pre-existing on main |
The two remaining failures are the ones that reproduce identically on main's 2026-07-28 nightly (same cuDNN assertion line; same 1800s timeout on a test declared est_time=300). One extra data point since my last comment: the cuDNN failure moved from novita-host4 to novita-host2 between rounds while shard 1 passed on novita-host4, so it tracks the test rather than the host — consistent with the nightly evidence and against my earlier host-specific guess.
Still needs a human decision
Merge order. tom/refactor-miles-repo-megatron/op8-13 must land in miles-main before or with the miles side, or main breaks the moment op8-9 merges. The ci-megatron-pr: line makes CI correct; it does not make the merge safe. That branch is also one commit behind miles-main (9fc14d826), worth rebasing first.
Per the user's standing instruction, no issues filed for the two main failures — raised here and with the user instead.
Status: https://github.com/radixark/miles/pull/1911/checks
17 pass, 2 fail, 0 outstanding. Every lane is green except the two that reproduce identically on
Both were failing on The merge-order point from my previous comment still stands and needs a human: Status: https://github.com/radixark/miles/pull/1911/checks |
e6decb6 to
d8234df
Compare
The two failures arrived together, with one image rebuild
So Nightly results across that jump:
(The 2026-07-25 nightly is not a data point: every stage Both symptoms — the cuDNN The experimentThis PR is now pinned to the last image that demonstrably passed both lanes: Run 30472852114 resolved This pin is a diagnostic, not a fix, and must come off before merge — it is two days older than Status: https://github.com/radixark/miles/pull/1911/checks |
36a6024 to
d8234df
Compare
Result of the experimentPinned to
So the two failures have different causes
I should also correct myself: I earlier lumped the 2026-07-26 nightly's 2-gpu failures in with this one. They were different tests ( Why this PR cannot be fully greenThe two conditions are mutually exclusive from here:
I have reverted What actually needs doing, by someone who owns these
No issues filed, per the user's standing instruction; raising it here and with the user. Status: https://github.com/radixark/miles/pull/1911/checks |
Final stateRun 30479714644 on Passing: Failure 1 — pre-existing, caused by the shared CI image
Evidence it is the image and not any code under test:
Note the older image is not a usable workaround: it predates the mooncake overlay ( Failure 2 — pre-existing, caused by
|
5cf6e24 to
23a65d9
Compare
d8234df to
40fdcfc
Compare
23a65d9 to
1d07063
Compare
40fdcfc to
d0682a5
Compare
1d07063 to
fbefce1
Compare
d0682a5 to
c95064e
Compare
|
Squashed from: - Fix PYTHONBUFFERED typo in launch scripts and command utils - Fix the same typo in the NPU docker patch - Unbuffer the ray workers, not only the submitting client - Unbuffer the launchers that submit ray jobs of their own
Squashed from: - Add a shell launch script test harness - Make the shell harness report stderr and emit shim stdout correctly - Drop the deprecated huggingface-cli shim - Exercise the shim behaviours the single real script never reaches - Poll the ray cluster the way the real scripts do in the synthetic script - Group the harness tests by what they exercise - Make the harness record commands in fork order and refuse to be unfrozen
…r paths Squashed from: - Fix launch scripts whose model config path could never resolve - Fix two launcher entrypoints that raised before issuing any command - Cover the two regressions this op fixes
… scripts Squashed from: - Derive the miles checkout location instead of hardcoding it - Quote the derived train.py path and pin the invariant - Find the shell scripts without shelling out to git
Squashed from: - Snapshot the external commands of every shell launch script - Apply isort and black to the shell launch script test - Intercept ps so the recordings do not read the host process list - Harden the harness against host state the rollout exposed - Share the snapshot compare-or-update step and stop running each script twice - Keep the generated snapshots under one obvious tests/snapshots tree - Group the launch script tests by subject - Name the shell launcher test after what it covers - Assert the recorded order for every script, including the concurrent ones - Regenerate the concurrent launcher's snapshot in its true command order
ExecuteTrainConfig.num_nodes read SLURM_JOB_NUM_NODES into a class-level
default, so the value was fixed when command_utils was imported. A test that
wants a deterministic launch command cannot undo that with monkeypatch, and a
process that sets the variable after import does not see it either.
A default_factory reads it at construction instead, but dataclass_cli copied
the parameter's declared default straight into the click signature, and for a
factory field that default is dataclasses' _HAS_DEFAULT_FACTORY sentinel,
which click then type-casts:
TypeError: int() argument must be ... not '_HAS_DEFAULT_FACTORY_CLASS'
Every scripts/run_*.py exposes this config through that bridge. Resolve the
factory when the signature is built, the way the argparse bridge already does.
…ript Squashed from: - Snapshot the commands built by every python launch script - Apply isort and black to the python launch script test - Reuse the shell harness sanitizer and snapshot helper - Move the python launcher snapshots into the shared tree too - Share the command recorder with the command_utils tests - Freeze the launcher environment that the snapshots actually depend on - Regenerate the launcher snapshots for the ray runtime unbuffering - Snapshot the config files a launcher generates, not just its commands - Record the generated precision config in the deepseek-v4 snapshots
Squashed from: - Cover the public surface of command_utils with unit tests - Group the command_utils tests by the function under test - Close the gaps that let the command_utils tests pass on broken behaviour - Keep the command_utils tests in one file
Squashed from: - Rename exec_command by the resource its command needs - Point the nvlink and single-node conversion tests at the gpu helper - Re-record the multi-node label the rename changed - Rename the last two exec_command call sites the split missed - Re-record the multi-node label in the rsync_simple test too
Squashed from: - Move the shell exec helpers next to their only consumers - Carry NodeAffinitySchedulingStrategy along with the moved exec helpers - Stop patching command helpers on a module that no longer has them
…yloads Squashed from: - Accept inline base64 payloads for the config file arguments - Pass config documents inline instead of through a temp file - Make the inline config payload reach every consumer and fail loudly - Apply pre-commit import ordering - Regenerate the deepseek-v4 snapshots for the inline config payload
Squashed from: - Snapshot the launchers that build their own command line - Record what the self-executing launchers submit today - Freeze the pid these launchers embed in their cleanup command
…ures Squashed from: - Let a p2p profile's rotary_base reach the model script it configures - Test the model args command run.py actually builds, not a copy of its logic - Flip the p2p snapshots to the rotary base each profile declares
The next ops rewrite all 62 scripts/models/*.sh into python. Once the shell versions are gone there is no source of truth left to prove the rewrite was faithful, so record the argv each of them expands to now. What these golden files pin is agreement with the shell era, not merely agreement with today's behaviour; the rewrite may only change the producer, never these files. They also give the 18 models that no launcher snapshot reaches their first coverage of any kind.
Squashed from: - Expand the model args in python before building the command - Update the launcher snapshots for the inlined model args - Point the command_utils tests at the expanded model args - Freeze the model-args knobs the snapshots now depend on - Skip non-files when scanning the model scripts for environment knobs
Squashed from: - Replace the model config shell scripts with python - Point the run_megatron CLI tests at the model args loader - Preserve the rotary base override in the 16-node profile launcher - Convert the shell model configs to python and load them through one CLI - Apply pre-commit formatting - Keep the environment overrides and the failure path the sourced scripts had - Point the NPU docker patch at the python model definitions - Read each model's environment override where the shell script read it - Match the shell mask when the model is shorter than its dense prefix - Require keyword arguments for the moe layer frequency - Take the model args overrides from the environment the shell already used - Run the model args loader itself instead of a script that only forwards to it - Let the model scripts be plain lists and read their environment at call time - Group the model args utilities by what calls them - Regenerate the shell launcher snapshots for the model args entry point - Expect the TypeError an unknown model args keyword now raises - Drop the override argument the restored environment reading made redundant - Let a model script declare its arguments as one block of text - Concatenate the model argument lines instead of parsing them - Fix the callers that still joined the model args, and pin the contract - Repair the two paths the model args conversion left behind - Treat an explicit zero override as a value, not as a missing argument - Let a model script reach the loader without importing the miles package - Take the golden model args from the python loader instead of the shell - Record the model args lookup in the two concurrent launchers' snapshots - Point the p2p launcher at the loader and let it fail loudly - Regenerate the self-executing launcher snapshots for the python model args
Squashed from: - Quote the model args miles inlines into the launch command - Regenerate the launcher snapshots with the model args quoted - Quote the model args on the NPU path too - Quote the model args the formal-math launcher inlines too - Keep the quoting next to the loader it quotes
fbefce1 to
c77d60a
Compare
c95064e to
79e9a3c
Compare
2 participants
ci-megatron-pr: tom/refactor-miles-repo-megatron/op8-13
Part of #1837