fix(flux): make MLPerf validation measure what it claims to - #1055
fix(flux): make MLPerf validation measure what it claims to#1055jasainio wants to merge 15 commits into
Conversation
Flux evaluation reported a val_loss over 29,696 samples at the dataset's
own timesteps. It was doing neither, and both defects were invisible in the
logs because every number printed was the configured one.
Coverage. Energon shards validation data across dp_size * num_workers but
splits the batch quota across num_workers alone, so the two divisions agree
only when each worker's slice is a whole number of microbatches. They did
not agree in any shipped recipe: the tail of every short worker's slice went
unread, giving 27,776 samples at num_workers 16 and 28,928 at 8, with the
same samples missing on every eval. This adds val_num_workers (defaulting
to 0), builds validation its own WorkerConfig, and asserts
eval_samples % (dp_size * max(1, val_num_workers) * micro_batch_size) == 0
so an unreachable shape fails at startup with the list of counts that work.
The budget is now expressed as eval_samples rather than eval_iters, which
stays correct across batch-size changes, and full_validation reads the true
split size from the dataset index instead of being a no-op.
Timesteps. The MLCommons val Arrow files carry a per-sample int32 timestep;
ingest read only the four tensor columns and dropped it, so every eval ever
run against these shards silently fell back to injecting arange(B) % 8.
That is not a near miss: the published val set pairs each image with one
fixed timestep and orders them so a contiguous batch spans two sigmas, not
eight. Ingest now carries the column into the sidecar, collate validates it
on every sample rather than samples[0], and eval_timestep_source makes the
choice explicit so a missing field is an error for MLPerf instead of a
silent substitution.
is_validation now derives from eval mode alone rather than from the batch
carrying a timestep. The old signal was data-driven, so a training batch
that happened to carry the field silently suppressed CFG dropout.
Noise. per_step_rng_reseed froze the training step counter during eval and
reseeded every microbatch identically, replaying one draw of VAE epsilon and
flow noise across the whole validation set. Both references redraw per
batch. Eval now advances its own counter in a seed range disjoint from
training, derived from (iteration, microbatch) so it survives resume without
checkpointing a counter, and leaves the training stream untouched.
Reporting and cost. consumed_valid_samples counted a flat global batch per
iteration regardless of the real width; it now uses the observed loss
denominator. Three all-reduces and two host round-trips collapse to one
packed fp64 reduction, the missing MLPerf evaluation_frequency event is
logged, and the unconditional per-eval gc.collect()/empty_cache() moves
behind eval_purge_memory, off by default.
Verified against /mnt/dcgpuval/mlperf_flux1: a harness rebuilding the
provider's exact dataloader reproduces 28,928, 27,776 and 5,120 on the
unfixed shapes and 29,696 with 3,712 per timestep after the fix. A full
re-ingest of the val split yields exactly 3,712 per timestep, and driving
both splits through the reader shows the same batch of 64 images carrying
sigmas {0, 0.5} from the real column against all eight from the fallback.
The reduction is split out of primus_evaluate as reduce_eval_losses so the
step that produces the reported number is testable without a process group.
Its tests pin that the loss is a ratio of summed numerator and denominator
rather than a mean of per-rank ratios, that the denominator survives as a
sample count, and that a short or doubled read raises instead of being
reported as the count the configuration intended.
Deployment note: the recipes set eval_timestep_source: dataset, so
evaluation fails loudly until a val split ingested with the timestep column
is in place.
…ngest MLCommons manifests list dataset_info.json and state.json beside the Arrow data. The ingest pipeline fetched the manifest unfiltered, so conversion failed on both and reported every run as having failed files. They also consumed max_files slots, and one sorting ahead of a data file would have shifted every shard index after it. parse_md5_manifest already accepted a suffix_filter; fetch_manifest just never passed one through. Separately, prepare_dataset_if_needed read an absent train_data_path as proof that a run wants a tokenised bookcorpus corpus. Every diffusion recipe sets dataloader_type: external and brings its own Energon pipeline, so that built a dataset the run never opens and demanded HF_TOKEN for a tokenizer it never loads. Adds the val-only ingest config the MLPerf validation split is built from.
The eval_samples patch derives eval_iters during build_args, before Megatron initialises process groups, so args.data_parallel_size did not exist yet. The patch runner logged the AttributeError and carried on, leaving eval_iters at 0: the job evaluated nothing and exited 0. Derive the width from world_size and the parallel sizes instead, and refuse to build validation dataloaders when eval_samples is set but eval_iters is 0, so a swallowed patch failure cannot pass for a clean run. Under skip_train the provider no longer builds a train dataset, which an evaluation-only dataset has no split for. Evaluation reporting now goes through one helper that drops to debug under MLPerf mode, where the submission log reports the loss itself and Megatron's own reporting is already suppressed. Only confirmations are quietened: a coverage shortfall still raises, and the mismatch that context parallelism explains still reports at info.
Black's formatting of the line, which the pre-commit gate enforces and which the preceding commits had not been run through.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| # Microbatch index stride within one evaluation. Must exceed the number of | ||
| # microbatches any single evaluation runs so that consecutive evaluations | ||
| # cannot collide; asserted where the index is built. | ||
| EVAL_RNG_ITERATION_STRIDE = 1 << 20 |
There was a problem hiding this comment.
Not applied: this constant is used, just not in the module that defines it. diffusion_trainer.py imports it and uses it four times to build the eval RNG index (iteration * EVAL_RNG_ITERATION_STRIDE + index) and to assert that consecutive evaluations cannot collide. Deleting it would break that import and remove the guarantee that eval seeds stay disjoint from training seeds.
| global _warned_uncovered_equidistant | ||
| if batch_size % NUM_VALIDATION_TIMESTEPS == 0 or _warned_uncovered_equidistant: | ||
| return | ||
| _warned_uncovered_equidistant = True |
There was a problem hiding this comment.
Not applied: this one is used within the same function. It is the warn-once latch in _warn_if_equidistant_undercovers -- read on line 75 and set on line 77 under global, and patched by two tests. Removing it would turn a once-per-process warning into one per microbatch.
The equal case returns early just above, so the `!= expected` test could never be false. Outdenting its body leaves behaviour identical and stops the guard from reading as though a third case existed.
olehtika
left a comment
olehtika
left a comment
There was a problem hiding this comment.
Reviewed against docs/mlperf/flux_eval_rules_rcp_audit.md (the v6.1 rules audit). A2 (eval RNG), A3 (per-sample timesteps) and A6 (eval budget) all land, and A2/A6 are better than what the audit prescribed — deriving the eval index from (iteration, microbatch) makes it resume-safe without checkpointing a counter, and rejecting eval_samples + eval_iters together closes the failure mode where a wrong value never errors.
Two things I'd flag, both in mlperf_logging_patches.py.
The evaluation_frequency event logs a key the checker doesn't look for
self._event(
key=getattr(self._constants, "EVAL_FREQUENCY", "eval_frequency"),
value=getattr(args, "eval_interval", 0) * self.gbs,
)The comment says the constant name "differs across mlperf_logging releases", but there is no EVAL_FREQUENCY in any release — I checked the installed 4.1.42 and upstream master, and neither mllog/constants.py defines it. The string evaluation_frequency appears only inside the compliance_checker/training_*/closed_flux1.yaml files, never as a constant. So the getattr fallback always fires and the emitted key is eval_frequency, while closed_flux1.yaml requires:
- KEY:
NAME: evaluation_frequency
REQ: EXACTLY_ONEThere's already a correct precedent in-repo: the diffusion backend emits the literal string at primus/backends/diffusion/trainers/base.py:521.
The other two EXACTLY_ONE keys are still absent
opt_learning_rate_warmup_steps and opt_gradient_clip_norm are both EXACTLY_ONE in closed_flux1.yaml and neither is emitted on the Megatron path. Both constants exist (OPT_LR_WARMUP_STEPS, OPT_GRADIENT_CLIP_NORM), and the diffusion backend already emits both (base.py:514 and :520), so this is one line each.
Net effect: with this PR the compliance checker still fails on the same three keys it failed on before it, and every underlying value is already correct — lr_warmup_iters and clip_grad: 1.0 are both right in the recipe, just unlogged.
Two smaller ones, cosmetic only
train_samples is emitted as null rather than 1,099,776, because getattr(args, "train_samples", 1099776) returns None when the attribute exists and is None — the default never fires. This PR's own EVAL_SAMPLES change uses the idiom that handles it (getattr(...) or eval_iters * self.gbs).
gradient_accumulation_steps is max(self.gbs // self.mbs, 1) = 8, ignoring data parallelism; the true micro-batches per step is 1. closed_common.yaml only checks > 0 so it passes, but the diffusion backend uses the real grad_accum_steps at base.py:512. Mirroring base.py:502-521 would close all four at once.
Coverage finding
Separately: the coverage defect this PR fixes (27,776 / 28,928 against a configured 29,696) falsifies §3.2 of the audit, which graded coverage correct on the strength of the Evaluating on 29696 samples log line and the .idx shard count. The .idx count is the dataset size, not what the loop read — exactly the trap described here. I'm updating the audit accordingly. It also means the seven L5 convergence runs in §4.3, including the 0.585314 gate crossing, were measured on 94–97% of the validation set, so those numbers need regenerating once this lands regardless of the A2 re-run.
olehtika
left a comment
olehtika
left a comment
There was a problem hiding this comment.
One more suggestion, on making eval_timestep_source hard to get wrong rather than merely possible to get right.
The field-precedence ordering leaves exactly one unsafe combination
resolve_validation_timesteps checks for the field before consulting the source:
if "timestep" in batch:
val_timesteps = batch["timestep"].float() / NUM_VALIDATION_TIMESTEPS
elif eval_timestep_source == DATASET_TIMESTEPS:
raise ValueError(...)
else:
val_idx = torch.arange(batch_size, device=device) % NUM_VALIDATION_TIMESTEPSThat ordering is the right call and it does most of the work — it means a re-ingested split is
evaluated correctly under either setting, so the two changes can land independently. Enumerated:
| Shards | eval_timestep_source |
Outcome |
|---|---|---|
| re-ingested | dataset |
dataset timesteps — correct |
| re-ingested | equidistant |
dataset timesteps anyway, the field wins — correct |
| pre-fix | dataset |
hard ValueError naming the fix — safe |
| pre-fix | equidistant |
positional injection, silent — the only unsafe cell |
The recipes in this PR set eval_timestep_source: dataset, which closes that cell for them. But
trainer_base.yaml defaults to equidistant for backward compatibility, so the protection rests on
each MLPerf recipe remembering to override it. A recipe copied from a non-MLPerf diffusion config, or
a new one that inherits the default, silently lands back on the injected timesteps — and per this
PR's own description that is indistinguishable from a correct run in the logs.
Suggestion: under mlperf_mode: true, refuse equidistant rather than relying on the recipe.
MLPerf submissions have no legitimate use for injected timesteps, so the combination can be made
unreachable by construction.
Where to put it: beside the existing EvalCoverageError, not in the build_args patch
The obvious home looks like patch_eval_samples, but that would be the wrong place for the reason
this PR already documents in energon_dataset_provider.py:
The patch that turns
eval_samplesintoeval_itersruns inbuild_args, where a failure is
logged and swallowed rather than raised.
An assertion added there would be swallowed exactly as the AttributeError was. The provider-side
EvalCoverageError guard is the pattern that works — a refusal at validation-dataloader-build time
that cannot be silently dropped — and mlperf_mode plus eval_timestep_source are both readable at
that point. Adding the check next to it costs a couple of lines and reuses a mechanism this PR
already established.
Why this is a live risk rather than a hypothetical
On our cluster there are now two complete copies of the MLPerf Flux val split on shared storage, one
carrying the timestep column and one not, with a third directory sharing the mlperf_flux1 name and
containing no shards at all. Until the pre-fix shards are gone everywhere, the bottom row of that
table is reachable by pointing a run at the wrong path — and it is the one combination that produces a
plausible-looking number instead of an error.
Incidentally, reading the annotated shards turned up a detail worth knowing for the test plan: the
per-shard timestep histograms are deliberately non-uniform, e.g. {0: 3712, 1: 1288, 4: 3712, 5: 1288}
for a 10,000-sample shard, because the split is ordered by timestep in blocks. Only the full-split
total is 3,712 each. A useful side effect is that with the real column a coverage under-read shows up
as a skewed histogram, whereas the positional fallback re-derives timesteps from batch position and so
looked uniform however much of the split went unread — which is to say the timestep defect was
concealing the coverage defect this PR fixes alongside it.
…requires Run the Flux MLLOG output through the official training_6.1.0 compliance checker and it fails before it reaches anything about the run itself. Several of these are v6.1 renames that were still being emitted under their v5 spelling, so the keys were present but invisible to the checker. Precision disclosures. v6.1 requires EXACTLY_ONE of lowest_numerical_precision_in_linear, _in_attn and _in_comm, and the logger emitted none. They are read from the environment and startup fails if any is unset, rather than defaulting: the value is a submission claim about what the run did, so inferring it from the config is how a stale claim gets logged after a precision change. The checker validates them against its own enum, which is what makes a wrong value cheap to catch and a missing one expensive. Epoch metadata. BLOCK_START/BLOCK_STOP took first_epoch_num and EVAL_START took epoch_num. v6.1 keys these on samples_count, and Flux is sample-based -- there is one epoch, so the old metadata carried no information either. Gradient accumulation. gbs // mbs counts the accumulation steps of the whole job rather than of one rank, so it over-reported by the data-parallel size on every multi-GPU run. Also emits evaluation_frequency under its v6.1 name rather than the EVAL_FREQUENCY constant, which is absent from the pinned mlperf_logging and silently fell back to the v5 literal; adds the required cache_clear, parallelism, micro_batch_size, config_filename, warmup-steps and clip-norm keys; and makes run_start idempotent, since an eval that precedes the first training batch otherwise emits a second one and the checker rejects the duplicate.
…1 ruleset 6.0.0-rc5 stops at training_5.0.0, so there was no way to check a v6 Flux log against anything: no training_6.0.0 config directory, no closed_flux1.yaml, no rcps_flux1.json. Work on the logging contract was therefore being done against a guess at what the checker wanted. 6.0.0-rc6 adds training_6.0.0 with closed_flux1.yaml and the reference convergence points, which is what makes the rest of this branch verifiable rather than plausible.
run_start has to precede every read of the real dataset, and it did not. It fired from the first training_log call, which is after the first train_step, which is after build_train_valid_test_data_iterators has already constructed the Energon iterators. Everything between those two points -- reading the .nv-meta shard metadata, spawning loader workers, filling the prefetch queue -- happened inside the initialization window and was excluded from the measured time. The warmup steps landed on the wrong side too: they ran on synthetic data inside the first train_step, so compile and FP8 bring-up were being charged to the run. There is no lifecycle hook at the point where the boundary belongs. Primus' phases (setup / build_args / before_train / after_train) all fire before pretrain(), and pretrain() runs setup_model_and_optimizer, then the data builder, then train() with nothing in between. Megatron-LM is a pinned submodule, so the call site cannot be edited either without forking it. mlperf_boundary creates the seam from outside by wrapping three entry points during before_train: pretrain to capture forward_step_func, which is a pretrain() argument and is reachable nowhere else beforehand; setup_model_and_optimizer to capture the model, optimizer and scheduler that warmup needs; and build_train_valid_test_data_iterators, whose first call is the boundary. Firing there runs the pre-run hooks, synchronizes, emits the transition on rank zero, and holds every rank on a second barrier so none of them can start opening shards while rank zero is still writing run_start. The once-guard covers virtual pipelining, which calls the builder per stage. The warmup body is now shared between the boundary and the original train_step hook rather than duplicated. Only MLPerf mode moves to the boundary: development recipes have no clock to protect and keep the behaviour they were tuned against, and the relocated path needs objects that only exist once the capture wrappers have run, so it fails loudly rather than silently skipping warmup if they have not. The old first-training_log trigger stays as a backstop and is now a no-op once the boundary has fired.
Four things in the log were being decided by defaults rather than by the run. The result never reached a file. mllog was left on its default handler, so the records went to stdout interleaved with every other rank's output and with framework noise, and the artifact a checker reads would have been whatever survived a later filtering step. Rank zero now writes MLLOG_OUTPUT_FILE directly. Submission identity defaulted. org fell back to "AMD", division to "closed", platform to "MI355X" and config_filename to "unknown". Those four values decide which division the log is judged in and which recipe a reviewer goes looking for, and a default is exactly how a wrong one gets submitted without anyone noticing. cache_clear defaulted to false in the same way, which let a run that never dropped caches make a claim about the machine it ran on. All of them are now required, and startup fails naming the variable. A run that missed the target produced no run_stop at all -- only convergence emitted one. run_stop is EXACTLY_ONE in the ruleset, so those logs did not parse, and a non-converging run is still part of the campaign the RCP checker compares. It is emitted from after_train now, idempotently, so a converged run does not get a second contradictory record. The block reopened only on the path where the validation loss could be read. When it could not, block_stop had already fired on entry to eval and training carried on outside any block. Precision disclosures are passed through unchanged even when the checker will reject them: training_6.0.0/common.yaml accepts a fixed vocabulary and mxfp6 is not in it, so an MXFP6 run produces a log that has to wait for the format to be approved upstream. Emitting fp8 instead would make it pass by describing the run falsely. It warns instead. The tests now drive the real mlperf_logging end to end -- real mllog writes the file, the real compliance checker reads it back under training_6.0.0 -- so a converged run is verified against the ruleset a submission is judged with rather than against a mock's idea of it. They skip where the package is absent. That is also how two assumptions turned out to be wrong: the checker requires at least one eval_accuracy at or below the target, so no amount of well-formed logging makes an exhausted run pass, and epoch_start needs no matching epoch_stop.
…arget train_iters was 5000, which at GBS 512 is 2560000 samples. The reference convergence points for flux1 at that batch size span 7077888 to 7602176 samples across 20 NVIDIA BF16 runs (rcp_checker training_6.0.0/rcps_flux1.json), so the budget ran out around a third of the way to the earliest of them. Every run was going to exhaust below target no matter how well it trained, and none of them could ever have counted as a result. 16000 steps is 8192000 samples, clearing the slowest reference run with room for a run that converges later than the reference spread. It is a cap and not a target: the run ends when eval_accuracy reaches 0.586 and how many samples that took is the result. lr_decay_iters moves with it -- inert while lr_decay_style is constant, but 4000 next to a 16000-step budget reads as a decay schedule that ends early. The seed also comes from outside now. A campaign is ten runs with ten different seeds, and the value in the log has to be the one the launcher chose rather than the one the file was written with.
The logging patch now fails closed on everything it would otherwise guess, so something has to supply the answers. This is that something, which makes it part of the submission rather than a convenience wrapper: it sets the submission identity, the precision disclosures, the result path and the recipe name, and it drops the page cache and reports cache_clear=false when it lacks the privileges to do so, instead of claiming a cold start it did not get. Each run is checked against the ruleset as soon as it finishes. Checking at the end of the campaign instead would mean finding out after ten runs and several days of compute that none of the logs parse. The campaign driver does ten runs with distinct seeds, then an RCP comparison over the set. Ten is what rcp_checker requires for flux1, not a round number. A run that ends without converging still produces a result file and still counts as one of the ten -- it is part of the distribution being compared, and dropping the runs that went badly is how the comparison stops meaning anything.
pretrain() runs a final validation pass whenever args.do_valid is set, and the early stop only clears args.train_iters, so reaching the target loss was followed by one more evaluation. That evaluation draws its own VAE epsilon and flow-matching noise, so it can land above the target and contradict the evaluation that just ended the run, leaving a result file whose last eval_accuracy is worse than the one at run_stop. Clear do_valid alongside train_iters so the pass never runs, and refuse to emit any MLLOG record once run_stop is out, so an evaluation reached by some other path cannot append to a closed log either. The second guard keys on run_stopped rather than converged, which also covers aborted runs. Verified on 8x MI355X: a run converging at step 2 of a 4-step budget now ends at run_stop with a single eval_accuracy and nothing after it.
The per-microbatch evaluation RNG index was built from args.iteration and a microbatch counter that only reset when that value changed. Neither is what it was meant to be. args.iteration is the resume point: Megatron assigns it at setup and at checkpoint load and never inside the training loop, so an evaluation at step k drew one set of noise when reached continuously and a different set when reached after a resume. And because it never changes mid-run, the counter never reset, so two evaluations at the same step continued one stream rather than reproducing each other. Key on args.curr_iteration, the step the loop last completed, and reset the counter per evaluation rather than per step. The evaluator signals the boundary through a module-global counter in eval_session, which is import-free so the per-microbatch path does not pull in the Megatron evaluation stack. Measured on 8x MI355X with two evaluations forced at the same step: they reported 1.688877 and 1.690349 before, and 1.685579 twice after. A drift of that size is enough to carry a follow-up evaluation across a 0.586 target. Note this changes which noise every evaluation draws, so convergence evidence collected before it does not carry over.
| import pytest | ||
| import torch | ||
|
|
||
| import primus.backends.megatron.training.eval_session as eval_session |
## Summary
Every Flux run with `mlperf_mode: true` and `warmup_train_steps > 0`
dies during the first real training step, before a single iteration
completes. This makes the warmup wait on the gradient reduce-scatter it
dispatches.
The MLPerf warmup runs at the pre-data boundary, which is before
Megatron's `train()` executes. `train()` is where
`config.finalize_model_grads_func` gets assigned, and that callback is
the only caller of `finish_grad_sync()`. So with `overlap_grad_reduce:
true` a warmup backward dispatches the data-parallel reduce-scatter and
nothing ever waits on it. The handle is still outstanding when the first
real step dispatches its own, and Megatron asserts:
```
File ".../megatron/core/distributed/param_and_grad_buffer.py", line 530, in start_grad_sync
self.grad_reduce_handle is None
AssertionError: Should not have multiple communication calls outstanding at once
```
### The fix
Install `finalize_model_grads` for the duration of the warmup loop and
restore whatever was there before, so the boundary warmup does not
change what `train()` later sees. The `None` guard leaves the
in-`train_step` warmup path used by development recipes untouched, since
by then `train()` has already installed the callback — which is why only
MLPerf mode was affected.
### Why the escape hatch does not cover this
`start_grad_sync` no-ops when a handle is outstanding *and*
`is_first_batch` is still set. Warmup step 1 populates the ready counts
without dispatching; the `reset()` at the head of warmup step 2 records
the golden counts and clears `is_first_batch`; warmup step 2 then
dispatches for real. By the first training step the flag is long gone,
so the assertion fires.
## Test plan
- [x] Two unit tests in
`tests/unit_tests/backends/megatron/test_mlperf_patches.py`: the warmup
steps see a grad-finalize callback, and a callback the caller already
set is preserved rather than replaced. Both fail without the production
change.
- [x] Full `test_mlperf_patches.py` passes (46 tests) on an MI355X dev
container.
- [x] Bisected on 8x MI355X with the Flux 12B MXFP6 MLPerf recipe. The
FP8 MLPerf recipe fails identically, so this is not precision-specific;
`mlperf_mode: false` passes; `warmup_train_steps: 0` passes.
- [x] With this change the full MLPerf recipe at `warmup_train_steps: 2`
trains 20 iterations clean, and a full convergence run is in flight.
---------
Co-authored-by: guangphu <guangphu@amd.com>
resolve_validation_timesteps prefers the dataset's timestep column whenever the batch carries one, so a re-ingested split is evaluated correctly under either setting of eval_timestep_source. That leaves exactly one unsafe combination: shards ingested before the column was carried through, read under equidistant. There the positional fallback injects t = index % 8, which reproduces neither the timesteps nor the image-to-timestep pairing the published split defines, and it fails silently -- the loss it reports is indistinguishable in the logs from a correct one. The MLPerf recipes all set dataset and close that cell for themselves, but trainer_base.yaml defaults to equidistant for the diffusion recipes that have no annotated split, so the protection rested on every future submission recipe remembering to override it. A recipe copied from a non-MLPerf diffusion config would land back on injected timesteps. There are also two complete copies of the val split on shared storage, one annotated and one not, so the combination is reachable today by pointing a run at the wrong path. The check goes beside the existing eval_iters guard in the Energon provider rather than in the build_args patch that sizes the eval budget. A failure raised there is logged and swallowed by the patch runner, which is how eval_iters silently stayed at 0; eval_timestep_source is also a Primus-only key, merged onto args only after build_args has run. Placement within the provider matters for the same reason: validation dataloader construction is wrapped in a bare except that disables evaluation and carries on, so the guard has to precede it. Adds the first tests to drive the provider's guards through create_dataloaders, pinning that an MLPerf run on injected timesteps raises before any dataset is built and leaves eval_iters alone, and that a genuine construction failure is still swallowed as before. Also corrects the comment on evaluation_frequency, which described a getattr fallback the key no longer uses. No mlperf_logging release defines a constant for that name; it appears only in the checker's own rulesets, so the literal is the spelling rather than a fallback.
All four are fixed. Three of them landed in b7c5ba8, which went up a couple of hours after your review — same finding arrived at from the other end, by moving to Values from an 8x MI355X run's MLLOG on the current head: No On the comment specifically. You were right that it was the actual defect, so I have corrected it in fe20330 rather than just deleting the Net effect is no longer the same three keys. On the coverage finding. Agreed on all of it, including that §3.2 of the audit is falsified. The |
Applied in fe20330, in the place you named. Your reasoning about Placement within the provider matters for the same reason placement outside Two decisions worth flagging: It fails closed on an absent setting. The error names the way out. It gives the setting to change and points at
The histogram detail is genuinely useful, thanks — that the positional fallback re-derives timesteps from batch position and so looks uniform however much of the split went unread is a sharper statement of why these two defects hid each other than anything in the PR description. I will fold the per-shard skew check into the re-ingest verification. |
|
|
||
| monkeypatch.setattr(provider_module, "get_val_datasets", _get_val_datasets) | ||
|
|
||
| provider = EnergonDatasetProvider(task_encoder_factory=lambda: object()) |
|
|
||
| import pytest | ||
|
|
||
| import primus.backends.megatron.data.energon_dataset_provider as provider_module |
|
Heads-up on a second warmup failure on this branch, adjacent to the one #1069 fixed. Opened #1072 Symptom. With Cause. Megatron's gradient buckets calibrate on their first batch — #1069 is necessary but not sufficient. Installing
Two things worth flagging from chasing this: The two assertions look like separate bugs and are one. Before #1069 the accumulation-1 shape The failure is invisible to a step-count health check in one specific case. Our Also useful as a regression canary: with the fix in place the log line reports The drain count should stay at 0 while #1069's finalize install is on the path. Before #1069 it |
3 participants
Summary
The MLPerf Flux validation path reported a number that did not correspond to the
evaluation it ran. This branch makes the reported loss, the sample count, and the
timesteps all mean what they say, and clears two things that stopped the
evaluation-only path from running at all.
The reported loss was an average of per-rank ratios, not a ratio of globally
summed numerator and denominator, so ranks with differently sized batches were
weighted equally. It is now a single packed fp64 all-reduce producing a value
identical on every rank, which the target-loss early stop depends on: if ranks
disagree near the threshold, one can leave
train()alone while the others keeptraining and desync collectives into an NCCL hang.
The sample count was the count the configuration intended, added per
iteration regardless of how wide the batches really were, which is
indistinguishable from a correct run. It is now the count the reduction actually
observed, and a shortfall raises rather than logs.
eval_iterscould silently resolve to 0. The patch deriving it fromeval_samplesruns duringbuild_args, before Megatron initialises processgroups, so reading
args.data_parallel_sizeraisedAttributeError; the patchrunner logged it and carried on, and the job evaluated nothing and exited 0. The
width is now derived from
world_sizeand the parallel sizes, and the Energonprovider refuses to build validation dataloaders when
eval_samplesis set buteval_itersis 0.Ingest reported HuggingFace metadata as conversion failures. MLCommons
manifests list
dataset_info.jsonandstate.jsonbeside the Arrow data;parse_md5_manifestalready accepted asuffix_filterbutfetch_manifestnever passed one. They also consumed
max_filesslots, and one sorting ahead ofa data file would have shifted every shard index after it.
Diffusion recipes detoured into bookcorpus tokenisation.
prepare.pyread anabsent
train_data_pathas proof that a run wants a tokenised corpus. Everydiffusion recipe sets
dataloader_type: externaland brings its own Energonpipeline, so that built a dataset the run never opens and demanded
HF_TOKENfora tokenizer it never loads.
Also adds the val-only ingest config the MLPerf validation split is built from, and
routes evaluation reporting through one helper that drops to debug under MLPerf
mode, where the submission log reports the loss itself and Megatron's own reporting
is already suppressed.
Test plan
eval RNG isolation, timestep source, MLPerf patches, and ingest pipeline
3,712 per timestep across the 8 equidistant timesteps
29,696 samples exactly (58 iterations x 512) and bit-reproducible across repeats
mlperf_mode: true, confirming the mllogeval_accuracyeventmatches and the duplicate reporting is suppressed
pre-commit run --all-filesandtools/ci/check_version_consistency.pythe loss reflects randomly initialised weights. The runs validate the
machinery, not the number.
Note
feat/mxfp6-fused-mlpis stacked on this branch and carries its own fix for theFlux Q/K dtype mismatch, which this branch deliberately does not duplicate.