Skip to content

[Feature] Refactor examples/configs filename - #911

Open
jiapingW wants to merge 2 commits into
mainfrom
refactor_file
Open

jiapingW wants to merge 2 commits into
mainfrom
refactor_file

Conversation

@jiapingW

Copy link
Copy Markdown
Collaborator

Motivation

Standardize example configuration filenames by removing redundant online, offline, and disaggregated labels already represented by the directory structure.

Modifications

  • Rename YAML recipes using -[-server-dp][-].yaml.
  • Preserve topology variants and place hardware suffixes such as npu, amd, and h200 last.
  • Update references in documentation, launch scripts, and tests.
  • Update recipe discovery tests to identify configurations by relative path, allowing identical filenames in different directories.

Related Issues

N/A.

Accuracy Test

No model-side changes or accuracy impact expected.

Validation:

  • 158 tests and 563 subtests passed.
  • All renamed YAML configurations retain their original parsed values.
  • Verified 41 referenced YAML paths and documentation links; no stale filename references remain.
  • Updated shell scripts pass bash -n; pre-commit checks pass.

Benchmark & Profiling

Checklist

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 maocheng23 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@FrankLeeeee

Copy link
Copy Markdown
Collaborator

Can we merge this now?

@jiapingW

jiapingW commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Have updated description according to the comment.

This branch has not been deployed

No deployments
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.

3 participants