Skip to content

fix(flux): start each MLPerf run with an empty result file - #1071

Closed
gphuang wants to merge 2 commits into
fix/flux-eval-correctnessfrom
fix/mlperf-mllog-truncate
Closed

fix(flux): start each MLPerf run with an empty result file#1071
gphuang wants to merge 2 commits into
fix/flux-eval-correctnessfrom
fix/mlperf-mllog-truncate

Conversation

@gphuang

@gphuang gphuang commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

The MLLOG logger opens MLLOG_OUTPUT_FILE append-only, and run_and_time.sh derives that path from RUN_INDEX alone. Relaunching a run under the same index therefore writes both attempts into one result_N.txt, and the compliance checker reports every once-per-run key twice.

This turned up on a real MXFP6 Flux convergence run. A first attempt died early, the fixed relaunch converged, and the checker then reported ~20 failures — run_start, init_stop, opt_name, train_samples, eval_samples and all the submission metadata, each "required EXACTLY_ONE occurrence but found 2". None of them were real. Splitting the file at the second init_start and re-running the checker left exactly one finding, the known mxfp6 vocabulary gap this script already documents.

The fix truncates the file where the run is set up, next to the mkdir that creates its directory. One file is one run.

Test plan

  • bash -n examples/mlperf/flux1/megatron/run_and_time.sh
  • Re-ran mlperf_logging.compliance_checker --ruleset 6.0.0 on the manually split log from the affected run: all duplicate-key findings cleared, only the mxfp6 precision-vocabulary failure remains
  • Next MLPerf run relaunched under the same RUN_INDEX produces a single-run result_N.txt (covered by the issue 306 ten-run campaign)

guangphu added 2 commits September 1, 2026 11:56
The MLLOG logger opens MLLOG_OUTPUT_FILE append-only and the path is
derived from RUN_INDEX, so relaunching a run under the same index
concatenates two runs into one result_N.txt. The compliance checker then
reports every once-per-run key twice -- run_start, init_stop, opt_name and
all the submission metadata -- and the failures look like a broken
submission rather than a relaunch.

Truncate the file where the run is set up, next to the mkdir that creates
its directory. One file is one run.
MLLOG_OUTPUT_FILE is overridable and need not sit under RESULTS_DIR, and
the script runs with set -e, so truncating a path in a directory that does
not exist would abort the run on a bare redirect error. Give the file the
same mkdir courtesy RESULTS_DIR already gets.
@gphuang

gphuang commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Note: author review of the full diff

Reviewed fix/flux-eval-correctness...HEAD in full. One blocking finding, now fixed in f0f240fa.

Blocking (fixed). MLLOG_OUTPUT_FILE is overridable and need not sit under RESULTS_DIR, and the script runs under set -euo pipefail. Truncating a path whose parent directory does not exist would have aborted the run on a bare redirect error, turning a convenience override into a launch failure. The file now gets the same mkdir -p courtesy RESULTS_DIR already has.

Considered and not blocking. Truncating a caller-supplied path is destructive, but the caller named it as this run's output file, so one-run-one-file is the expected semantics rather than a surprise. The truncation sits before the cache-drop and before any GPU work, so a permission problem fails the run in the first second instead of four hours in.

Checks run. bash -n clean. Truncation and nested-path creation verified against a scratch harness that writes an attempt, relaunches under the same index, and confirms only the second attempt survives. The compliance checker was re-run at ruleset 6.0.0 on the manually split log from the affected MXFP6 run, which cleared every duplicate-key finding and left only the known mxfp6 precision-vocabulary failure this script already documents in a comment.

Ready to merge.

@gphuang
gphuang marked this pull request as ready for review September 1, 2026 11:59
@gphuang

gphuang commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Note: handing this over — it fixes a file this chain introduced

This targets fix/flux-eval-correctness, which is part of the PR 1055 chain and under active development, so I am not self-merging it. Please take it whenever it suits the chain — or tell me to, and I will.

The change is two lines in examples/mlperf/flux1/megatron/run_and_time.sh, which 11d47e7b introduced on this branch. MLLOG_OUTPUT_FILE is derived from RUN_INDEX alone and the logger opens it append-only, so relaunching under the same index writes both attempts into one result_N.txt. The compliance checker then reports every once-per-run key twice — run_start, init_stop and the rest — which reads as a broken submission when the run itself was fine. Issue 306 hit exactly this and had to split the file by hand at the second init_start before the checker would give a usable verdict.

Fix is to truncate at setup so one file always holds one run:

mkdir -p "$(dirname "${MLLOG_OUTPUT_FILE}")"
: > "${MLLOG_OUTPUT_FILE}"

The mkdir -p keeps it working when MLLOG_OUTPUT_FILE is overridden to a nested path, and set -euo pipefail means a failure to truncate aborts the run visibly rather than silently appending again.

This matters most for a multi-seed campaign, where relaunches are routine and each run needs its own parseable log.

@gphuang
gphuang marked this pull request as draft September 1, 2026 14:02
@gphuang

gphuang commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Closing to keep the trail clean — this was avoidable without a code change.

The trap is real: MLLOG_OUTPUT_FILE is derived from RUN_INDEX and the logger opens it append-only, so relaunching under the same index writes two runs into one result_N.txt and the compliance checker then reports every once-per-run key twice. But I only hit it because I relaunched under index 0. Using a fresh RUN_INDEX per run avoids it entirely, which makes this a convenience rather than a fix, and it targets a branch under active development.

Leaving the mechanism recorded here so the next person recognises the symptom: twenty duplicate-key compliance failures on a run that was actually fine, cleared by splitting the log at the second init_start. If a multi-seed campaign later wants the guard rail, this is a two-line change worth reopening on a settled branch.

@gphuang gphuang closed this Sep 1, 2026
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