Skip to content

feat(megatron): support graduated precision boundaries - #980

Draft
gphuang wants to merge 5 commits into
mainfrom
feat/220/graduated-precision-boundary
Draft

feat(megatron): support graduated precision boundaries#980
gphuang wants to merge 5 commits into
mainfrom
feat/220/graduated-precision-boundary

Conversation

@gphuang

@gphuang gphuang commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Supports Task 9 Option 8 for Issue #220.

  • Add an optional outer precision override inside Flux sensitive boundaries.
  • Preserve the existing boundary as the MXFP4 exclusion region, then select outer BF16 before inner tensorwise FP8.
  • Validate that outer counts are non-negative and stay within their enclosing boundary.
  • Forward and log the new fields through FluxPretrainTrainer.

Defaults remain zero, so existing recipes keep their current routing.

Test plan

  • 14 targeted Flux config/layer-spec tests pass in the pinned Primus image
  • Exact 57-block routing proof: BF16 {0,56}, FP8 {1–3,53–55}, MXFP4 {4–52}
  • Python compile and whitespace checks pass
  • Option 8 model-build smoke after review and Probe 3 verdict

Allow an outer precision override inside Flux sensitive boundaries so recipes can keep the outermost blocks in BF16 while routing adjacent blocks through tensorwise FP8.
@gphuang

gphuang commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Result: agent review blocked before runtime smoke

Blocked. The intended Option 8 recipe routes correctly, but the framework API accepts configurations that can silently collapse the graduated layout.

  • get_flux_layer_spec() creates the inner/outer backends only for automatic local-MXFP4 selection. Explicit backends, TE, absent FP4, or unavailable providers silently route every block through one backend. Graduated routing must reject unsupported paths.
  • tw_fp8 can inherit a blockwise/MXFP8 recipe, and bf16 can select native linears under FP16 parameters. Validate that the configured names enforce tensorwise FP8 and BF16 semantics.
  • The Primus unit test checks only 57 QKV slots. Add the complete 247-slot census plus unsupported-configuration tests.

The complete base-to-head diff, surrounding config/provider paths, existing tests, and CI were reviewed. CI is green, but its Torch unit job was skipped. Rerun agent review after these findings are fixed; the PR is not ready for runtime smoke.

@gphuang gphuang left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up: re-review confirms API and routing block

Re-reviewed exact head c1e75cf across API behavior, routing, and tests. Findings are unchanged from the previous review: unsupported backend paths silently collapse graduated routing; tw_fp8 and bf16 do not enforce their stated precision semantics; and tests cover only 57 QKV slots rather than all 247 linear slots. The relevant Torch CI job was skipped.

The PR remains draft. Dependent recipe review and the 10-step smoke remain gated until these blockers are fixed.

Reject unsupported precision and backend combinations before model construction, and cover the complete Flux routing contract with negative-path and model-build tests.
@gphuang

gphuang commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Result: agent review approved after fixes

Review found that unsupported backend and precision combinations could silently fall back to one backend, while the routing test covered only one linear per block. Fixed in 1ef2bf6f: routing now rejects unsupported backend, provider, dtype, recipe, precision-role, and layer-count states. Tests cover all 247 declared linear slots and a mixed-precision model build.

Two independent re-reviews approved the updated diff. The PR stays draft while CI and the pinned GPU test remain open.

Guangpu Huang added 3 commits August 13, 2026 07:55
Apply the repository's Black formatting so the reviewed routing fix passes the lint gate.
Merge the native TE-MXFP4 and Flux baseline updates without changing the reviewed local-spec Option 8 routing contract.
Keep post-construction config mutations fail closed instead of silently falling back to homogeneous MXFP4 routing.
@gphuang

gphuang commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Result: reconciled graduated-routing review approved

Merged Primus main at abc46648 into the draft branch and preserved the exact Option 8 layout: BF16 {0,56}, tensorwise FP8 {1–3,53–55}, MXFP4 {4–52}.

Independent review found one post-construction fail-open path: disabling sensitive routing while retaining boundary counts silently selected homogeneous MXFP4. Commit f8cc4e16 now rejects that state at config construction and layer-spec selection, with regression coverage.

All required CI checks and static validation pass. PR 980 remains draft; the 10-step GPU smoke is still pending compute.

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