Skip to content

Add a rule file for the python launch and model scripts - #2357

Merged
Zhichenzzz merged 1 commit into
yueming/script-rewritefrom
zhichen/model-script-py-rule
Aug 10, 2026
Merged

Add a rule file for the python launch and model scripts#2357
Zhichenzzz merged 1 commit into
yueming/script-rewritefrom
zhichen/model-script-py-rule

Conversation

@Zhichenzzz

Copy link
Copy Markdown
Contributor

Stacked on #2356 (base is yueming/script-rewrite, review that first). Docs only — no runtime code changes.

#2356 finishes moving scripts/ to python, but the convention that produced it lives only in that PR's description and in the tests. This adds .claude/rules/launch-and-model-scripts.md so the next launcher follows it without archaeology, and registers it in AGENTS.md next to general-code-style.md.

Every rule is taken from the post-#2356 code or from a test that already enforces it, not from preference:

  • Python, not shell. load_model_args resolves scripts/models/<megatron_model_type>.py by file name, so a shell model definition cannot be found at all — and test_shell_script_hygiene.py already rejects a source scripts/models/x.sh in a docker patch. The surviving examples/**/*.sh are named as legacy awaiting a port, so nobody copies one as a template.
  • Model definitions. File name equals the megatron_model_type; one pure model_args(**kwargs) -> str; variants derive via load_sibling_model_args / moe_layer_freq; any environment knob a model script reads has to be in CLEARED_ENV, which test_every_environment_knob_a_model_script_reads_is_frozen enforces.
  • Launcher anatomy. Discovery is scripts/**/run_*.py, so the naming rule is what buys snapshot coverage. Public module-level functions are entrypoints (prepare, execute) and the suite asserts each issues commands, so helpers must be _-prefixed. Shell access goes through command_utils; the self-executing launchers under examples/ are called out as legacy pinned by test_self_executing_launchers.py.
  • Nothing hardcoded about the machine. --model-dir / --data-dir / --output-dir instead of literals, U.repo_base_dir instead of a checkout path, wandb only via get_default_wandb_args, environment read at call time (the num_nodes default_factory), --num-gpus-per-node always passed, --rollout-num-gpus dropped under --colocate, an already-joined cluster expressed with MILES_SCRIPT_EXTERNAL_RAY=1.
  • One launcher per recipe family. Merge near-duplicates behind a frozen _Recipe table keyed by a Literal --model-name; keep separate files when the recipes are different experiments, with run_glm45_355b_a32b_8node.py as the worked example and a stated threshold for when not to merge.
  • Snapshot coverage is the review artifact. Regenerate with MILES_UPDATE_LAUNCH_SCRIPT_SNAPSHOTS=1 pytest tests/manual/launch_scripts and read the diff; launchers must import and run without GPU, checkpoints or network; don't grow the harness denylists without a test that fails once the entry stops being true.
  • Docs follow the launcher. Adding/renaming/deleting a launcher updates the pages that invoke it, including hf download --local-dir destinations.

Testing

pre-commit run --files .claude/rules/launch-and-model-scripts.md AGENTS.md clean. No code paths touched.

@Zhichenzzz
Zhichenzzz force-pushed the zhichen/model-script-py-rule branch from 7d49caf to 10327f7 Compare August 10, 2026 21:27
@Zhichenzzz
Zhichenzzz merged commit 98f568d into yueming/script-rewrite Aug 10, 2026
9 of 16 checks passed
@Zhichenzzz
Zhichenzzz deleted the zhichen/model-script-py-rule branch August 10, 2026 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant