[JakeStevens]

Summary:

Add optional --dump_output to the native classification runner for independent full-output validation. Write the original FP32 logits before reference comparison, including mismatches and non-finite outputs, without altering their bits or numerical acceptance.

The runner loads --categories and --expected before creating the dump, so an input error exits 2 without leaving a file, and a missing reference cannot be satisfied by the runner's own dump. As a result, a bad --expected path now fails before the top-5 and argmax are printed, even without --dump_output.

write_floats removes an incomplete file when the write or close fails. The LLM runner's --dump_first_logits now uses it instead of its own copy of the same logic.

This change was written with help from Claude Code.

Differential Revision: D129375473

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/24505

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Planned maintenance affecting GB300 CI jobs

❌ 2 Cancelled Jobs, 1 Pending, 3 Unrelated Failures

As of commit df7f7b1 with merge base ee4b85b ():

CANCELLED JOBS - The following jobs were cancelled. Please retry:

• pull / nxp-mcuxpresso-test / linux-job (gh)
  Could not load credentials from any providers
• pull / unittest-editable / windows / windows-job (gh)

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• trunk / test-torchao-huggingface-checkpoints (lfm2_5_1_2b, mt-l-arm64g4-16-62, executorch-ubuntu-22.04-gc... / linux-job (gh) (trunk failure)
  RuntimeError: Command bash /__w/_temp/exec_script failed with exit code 1
• trunk / test-torchao-huggingface-checkpoints (lfm2_5_1_2b, mt-l-x86iavx512-8-64, executorch-ubuntu-22.04-... / linux-job (gh) (trunk failure)
  RuntimeError: Command bash /__w/_temp/exec_script failed with exit code 1
• trunk / unittest-release / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@JakeStevens has exported this pull request. If you are a Meta employee, you can view the originating Diff in D129375473.

[github-actions]

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

[shoumikhin]

@claude review effort=max

[claude]

Claude finished @shoumikhin's task in 22m 33s —— View job

---

PR Review: #24505

Nothing blocks this PR. The main gaps: tolerance errors are still caught after the dump is written (the description says otherwise), nothing tests the new runner behavior, and neither the error messages nor the docs say the dump path must not already exist.

Detailed review

Summary

The PR adds --dump_output to the native classification runner, backed by a new exclusive-create write_floats helper that --dump_first_logits now also uses. I found no correctness bugs in the changed code. The problems are:

• one gap in the description's "an input error exits 2 without leaving a file" guarantee;
• no tests at the runner level;
• less informative error messages;
• the new flag's behavior isn't documented.

All five files were reviewed. I couldn't build or run anything because this job doesn't allow compiler invocations, so everything below comes from reading the code.

Code Quality

• ClassificationRunner.cpp:156-165: input checks still happen after the dump is written and after the model runs.
  • --rtol/--atol and the reference length are only checked inside compare_floats (runner_utils.cpp:95 and :104), which runs after write_floats. So --expected ref.bin --dump_output out.bin --rtol=-1 creates out.bin, prints error: tolerances must be finite…, and exits 3. The guarantee only holds for errors that throw.
  • A typo in --expected or --categories, or a leftover --dump_output file, is only reported after Package::load, compile and execute (lines 70-150).
  • Fix: read both files and check the tolerances right after the required-flags check at line 64. Return 1 there, which is what the README documents for invalid arguments. Then check reference.size() against numel_of(out_sizes) once it is known at line 127. The guarantee then holds for every input error, and bad input fails before the expensive work. Fix this →
• runner_utils.cpp:41 and :48: the new error messages don't give the cause.
  • Because the file is created exclusively, the usual failure is a file left over from a previous run. That now prints error: cannot create out.bin, which looks the same as a missing directory or a permission problem, and exits 2 just like a load failure.
  • For --dump_first_logits this is worse than before. The old cannot create new logits dump: <path> at least hinted that the file must be new. The description's "same logic" doesn't mention that the messages changed.
  • Fix: save errno right after the call that fails (before std::remove, which can overwrite it) and append std::strerror. That gives cannot create out.bin: File exists. Fix this →
• test_runner_utils.cpp:99-109: process-wide state is restored by hand.
  • If ASSERT_EQ(setrlimit…) at line 100 fails, the test returns with SIGXFSZ still ignored.
  • Any exception other than std::runtime_error escapes line 105 with the 4-byte RLIMIT_FSIZE still set. Every later file write in the process then fails with EFBIG (for example --gtest_output=xml, or tests reordered by --gtest_shuffle).
  • A small RAII guard that restores both avoids this. Fix this →

Testing

• No test runs the runner-level behavior this PR is about.

  • .ci/scripts/test-native.sh builds run_classification_cpu, run_classification_ggml_cpu and run_classification (lines 29, 39 and 49) but never runs them.

  • .ci/scripts/test_native_llm_runner.py:78-90 runs run_hf_llm_ggml without --dump_first_logits, so the refactored call at LlmRunner.cpp:205-207 never runs in CI either.

  • The description's guarantees depend only on the order of statements at ClassificationRunner.cpp:160-165: the reference is read before the dump, no file is left on input errors, and the dump is kept on FAIL. Nothing would catch a later reordering.

  • Cheapest coverage:

    • In test_native_llm_runner.py, pass --dump_first_logits and check that the file holds one vocabulary row whose argmax is the first generated token. Prefill is a single call at position 0 (LlmRunner.cpp:235).
    • Run one classification runner with --dump_output and check three cases: the dump is bit-exact, a missing --expected exits 2 with no file, and an existing dump path exits 2 with the file untouched. The make_fixture.py package would work as the model; backends/native/python/README.md:202-203 says the vision example accepts it.

    Fix this →

• test_runner_utils.cpp:94: the branch where fclose fails is never tested.
  • On glibc with a 4 KiB st_blksize (ext4, tmpfs), the 4096-byte fwrite bypasses the stdio buffer and returns short, so only written != values.size() fires.
  • A 1000-class dump is 4000 bytes and fits entirely in the buffer, so a real ENOSPC shows up at fclose. That is the "or close fails" path the description mentions.
  • Fix: add a second case that writes less than the buffer, for example two floats under the same 4-byte limit. Also check that the message says cannot write, so a failed fopen can't make the test pass. Fix this →

API Design

• ClassificationRunner.cpp:31 and backends/native/README.md:303-311: the new flag's behavior isn't documented.

  • The help text ("Write raw float32 logits to a new file.") doesn't say that an existing path is refused, which ends the run with exit 2 after the model has executed. --dump_first_logits does say "refuse to overwrite" (LlmRunner.cpp:56).
  • The README section that documents these runners' flags, output and exit codes doesn't mention the flag at all.
  • If you change the help text, consider renaming the flag to --dump_logits to match this file's "logits" wording and the LLM flag.
  • run_classification.py takes the same flags but doesn't get this one. Is that intended?

  Fix this →

Specific Comments

• LlmRunner.cpp:207 and :210: logits.data() + logits_numel - vocabulary_size is now written out here and again as row three lines below. Move row above both ifs and pass {row, vocabulary_size}.
• runner_utils.h:17: write_floats reads like the inverse of read_floats, but it refuses to overwrite and deletes the file on failure. A one-line comment, or a name like write_new_floats, would make that clear to the next caller.
• test_runner_utils.cpp:28: RunnerOutputTest only tests write_floats. Naming the fixture WriteFloatsTest would let each test drop its WriteFloats prefix.
• test_runner_utils.cpp:51-52: "PreservesAllBits" covers ±0, two normal values, +inf and one quiet-NaN payload. To match the name, add −inf (0xff800000), a subnormal (0x00000001) and a negative NaN (0xffc00001). Leave out signalling NaNs, because returning a float through x87 can quiet them.
• runner_utils.cpp:44 (pedantic): for the empty span in WriteFloatsCreatesEmptyOutput, values.data() is nullptr. This works because glibc only marks the FILE* argument nonnull, but C 7.1.4 makes a null pointer argument undefined even with a zero count. Skipping fwrite for an empty span removes that dependence.

Recommendation

Approve. Nothing blocks. The two cheapest changes worth making before landing:

• Move the tolerance and reference-length checks before the dump, or narrow the description's claim.
• Add --dump_first_logits to the existing LLM CI script.

Review steps

• Loaded the /pr-review skill
• Gathered context: single commit 4aa83a0, 5 files, +125/−16. The checkout is shallow, so I diffed against HEAD~1.
• Ran a panel of 3 independent reviewers, each starting from a different file
• Consolidated and deduplicated their findings: about 20 candidates became 10. I dropped the pre-existing or not-actionable ones: "wbx" portability, MSVC C4996, the CI job that runs the test, and truncation if the process is killed.
• Fact-checked each finding against the code and glibc's stdio.h
• Posted the review