Add run-glm47-flash-agentic-async.py launcher - #1019
Conversation
Convenience launcher for GLM-4.7-Flash fully-async agentic training on SWE-bench. Wraps train_async.py with the model-specific Megatron config, the agentic tool-call generate function, and an external Harbor agent server URL so users can spin up a typical 8-node split (1 train + 7 rollout) with a single command.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a script for fully-async agentic training of the GLM-4.7-Flash model using SWE-bench data, supporting disaggregated training and inference across multiple nodes. The reviewer identified several improvement opportunities: a fix for the pgrep pattern in the cleanup function to prevent self-matching, the replacement of a magic number with a named constant for better maintainability, and a correction for a typo in the SGL_DISABLE_TP_MEMORY_IMBALANCE_CHECK environment variable.
| subprocess.run( | ||
| f"pgrep -f '{t}' | {exclude} | xargs -r kill 2>/dev/null || true", | ||
| shell=True, | ||
| ) |
There was a problem hiding this comment.
Using pgrep -f with a pattern that matches the cleanup command itself can cause the intermediate shell process (spawned by subprocess.run) to be killed prematurely, as its command line contains the pattern {t}. A common workaround is to use a bracket expression for the first character of the pattern (e.g., [{t[0]}]{t[1:]}), which prevents the pattern from matching its own process string while still matching the target processes.
| subprocess.run( | |
| f"pgrep -f '{t}' | {exclude} | xargs -r kill 2>/dev/null || true", | |
| shell=True, | |
| ) | |
| pattern = f"[{t[0]}]{t[1:]}" | |
| subprocess.run( | |
| f"pgrep -f '{pattern}' | {exclude} | xargs -r kill 2>/dev/null || true", | |
| shell=True, | |
| ) |
There was a problem hiding this comment.
Fixed in a387b78 — wrapped the first char of each pgrep target in brackets ([s]glang etc.) so the pattern no longer matches the shell subprocess that pgrep runs under.
|
|
||
| def prepare(args: ScriptArgs): | ||
| """Convert HF checkpoint to torch_dist format.""" | ||
| max_convert_nodes = 92 // args.num_gpus_per_node |
There was a problem hiding this comment.
The value 92 is a magic number. It should be defined as a named constant with an explanatory comment to clarify its purpose and improve maintainability.
There was a problem hiding this comment.
Fixed in a387b78 — extracted as module-level constant MAX_CONVERT_GPUS = 92 with a short comment on what the ceiling is for.
| "PYTHONPATH": f"{args.megatron_path}:{SCRIPT_DIR}:{FULLY_ASYNC_DIR}:{miles_root}", | ||
| "MILES_EXPERIMENTAL_ROLLOUT_REFACTOR": "1", | ||
| "NCCL_NVLS_ENABLE": "0", | ||
| "SGL_DISABLE_TP_MEMORY_INBALANCE_CHECK": "false", |
There was a problem hiding this comment.
There is a typo in the environment variable name: SGL_DISABLE_TP_MEMORY_INBALANCE_CHECK should be SGL_DISABLE_TP_MEMORY_IMBALANCE_CHECK. Also, please verify if the value should be "true" to actually disable the check, as "false" is typically the default behavior.
| "SGL_DISABLE_TP_MEMORY_INBALANCE_CHECK": "false", | |
| "SGL_DISABLE_TP_MEMORY_IMBALANCE_CHECK": "false", |
There was a problem hiding this comment.
Not changed — INBALANCE is intentional. The upstream sglang env var name ships the typo (see sglang/python/sglang/srt/environ.py line 556: "SGL_DISABLE_TP_MEMORY_INBALANCE_CHECK"), so renaming it to IMBALANCE here would make sglang ignore the override. Every other launcher in this repo (run-glm47-reasoning.py, run-glm47-reasoning-async.py, miles/ray/rollout.py) uses the same INBALANCE spelling for the same reason.
Adds --num-rollout, --rollout-batch-size, --n-samples-per-prompt, --global-batch-size, --over-sampling-batch-size to the launcher so smoke tests can run with small batches and step counts without editing the script.
Default trace dir is <save_dir>/traces; overridable via --save-traces-dir, or set to "disabled" to skip. Wired through --dump-details so miles writes per-rollout .pt files for both rollout samples and training data.
- Wrap the first char of each pgrep pattern in brackets so the pattern cannot match the shell subprocess's own command line. - Name the 92-GPU ckpt-conversion ceiling as MAX_CONVERT_GPUS with a short comment. Not changed: SGL_DISABLE_TP_MEMORY_INBALANCE_CHECK. sglang upstream (python/sglang/srt/environ.py) ships the env var with that exact spelling, so "correcting" it to IMBALANCE would silently disable the setting.
2 participants
Summary
Adds a convenience launcher for GLM-4.7-Flash fully-async agentic training on SWE-bench at
examples/experimental/swe-agent-v2/run-glm47-flash-agentic-async.py.It wraps
train_async.pywith:--rollout-function-path fully_async_rollout.generate_rollout_fully_async,--custom-generate-function-path miles.rollout.generate_hub.agentic_tool_call.generate, and--agent-server-urlpointing to an external Harbor agent server (e.g. the AWS-hosted one via Tailscale egress).Default split is 1 training node + 7 rollout nodes for an 8-node job; configurable via
--train-num-nodes. The compatibility shim that the agentic generate function relies on landed in #1016 (which is what makes #1008 obsolete and unblocks this launcher onmain).Test plan
python run-glm47-flash-agentic-async.py --num-nodes 8 --agent-server-url http://ts-egress-aws-agent-server:8080