refactor(fsdp)(1/5): metrics buffer and train metrics integration#70
Open
Rockdu wants to merge 3 commits into
Open
refactor(fsdp)(1/5): metrics buffer and train metrics integration#70Rockdu wants to merge 3 commits into
Rockdu wants to merge 3 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
MetricBufferwhose schema declares each metric's reduction op (MEAN/MAX/REPLICATED) once, replacing thedefaultdictof per-microbatch tensors that was uniformly averaged.MEANmetrics record a local sum plus the item count behind it; the flush divides globally summed numerators by globally summed counts.MAXmetrics (model_output_max_abs_diff,model_output_rel_max) are reduced with an actualReduceOp.MAX.gather_objectover the whole world to two deviceall_reduces overdp_group, accumulating in float64.Why
The old path had three defects:
*_max_*. A rank that saw an outlier had it diluted by1/world_size, which defeats the purpose of a max metric — it hid exactly the train-vs-rollout divergence those metrics exist to catch.gather_objectpickled a dict per rank through gloo on every optimizer step and reduced on the Python side.On the group choice. The buffer reduces over
dp_group, not the world. SP peers in Miles-diffusion share samples (rollout data is indexed bydp_rank) and the sequence-parallel plan all-gathers the model output at the boundary hook before the loss, so SP peers hold identical values. To be precise, the reported numbers would be unchanged over the world group — a duplicate cancels out of both a sum/count and a max — so this is about not draggingsp_sizex the ranks into every collective, not about correctness.Validation
Run on an H200 devbox using the project image
rockdu/miles_diffusion:dev-cu129-sglang-main-20260719, CPU-only (CUDA_VISIBLE_DEVICES="").Added the 4-rank test covers what single-rank tests cannot: that a mean weights items rather than ranks, that a max survives when only one rank sees the outlier, that a metric only one rank records still reduces correctly, and that a subgroup ignores ranks outside it (the way DP ignores SP peers).
Checklist
pre-commit run --all-filespassespytest -xis green —tests/fastis 125 passed with one pre-existing unrelated failure (see Validation)python3 train.py --helpstill parses — n/a, no flags changedci-sglang-repo: sgl-project/sglang
ci-sglang-pr: #32420