Repository navigation
Conversation
There was a problem hiding this comment.
Nit: now that the name only says -1server-dp4, could we add a short header comment noting how this differs from its sibling qwen3-8b-dflash.yaml? Besides 4 vs 8 trainer ranks, it also uses report_to: none, the default attention backend (no flex_attention), and drops embedding_key/torch_dtype/log_interval. For example:
# 4-rank trainer variant of qwen3-8b-dflash.yaml (1 external capture server).
# Also differs from the 8-rank recipe: report_to=none, default attention backend.There was a problem hiding this comment.
Same nit as the dflash dp4 file: a header comment would help. This one differs from qwen3-8b-domino.yaml by more than topology: it trains on perfectblend_qwen3-8b_regen.jsonl (vs ShareGPT), and also sets mask_token_id, trust_remote_code, report_to: none, and different save/log intervals. Someone choosing between the two by filename alone wouldn't expect a different dataset.
There was a problem hiding this comment.
Nit: same here, consider a header comment. Compared with qwen3.6-27b-dflash.yaml, this uses max_length: 2048 (vs 4096) and save_interval: 1000, not just 2 vs 8 trainer ranks.
maocheng23
left a comment
There was a problem hiding this comment.
LGTM. I checked that no references to the 78 old filenames remain (also after merging current main), that no new path reuses an old one (so stale --config paths fail loudly instead of loading a different recipe), and that the <N>server-dp<M> suffixes match the configs (dp = trainer ranks). Keeping run_id/output_dir unchanged is a good call: it keeps W&B and output-dir continuity, and the run IDs are still unique.
Two small non-blocking nits are inline: one README sentence about when the topology suffix is used, and header comments on the three -1server-dpN variants that differ from their siblings by more than topology.
| The directory is the source of truth for mode, topology, and service ownership. | ||
| Some filenames retain historical `-online`, `-offline`, or `-disaggregated` | ||
| labels so existing recipe identities and run names remain recognizable. | ||
| Recipe filenames use `<model>-<draft-type>[-<servers>server-dp<ranks>][-<hardware>].yaml`, |
There was a problem hiding this comment.
Nit: in practice the topology suffix only appears where two topologies of the same recipe exist side by side. Most recipes have none whatever their layout, e.g. external/qwen3-8b-dflash.yaml (1 server / 8 ranks), external/qwen3.8-27b-dflash2.yaml (8 server URLs / 8 ranks), managed-local/qwen3.5-4b-mtp-npu.yaml (6 servers / 10 ranks). Maybe add a sentence like: "The -<servers>server-dp<ranks> suffix is added only to tell topology variants of the same recipe apart; other recipes record their topology under deployment only." That way readers don't assume an unsuffixed name implies some particular topology.
|
Can we merge this now? |
|
Have updated description according to the comment. |
Motivation
Standardize example configuration filenames by removing redundant online, offline, and disaggregated labels already represented by the directory structure.
Modifications
Related Issues
N/A.
Accuracy Test
No model-side changes or accuracy impact expected.
Validation:
Benchmark & Profiling
Checklist