Skip to content

Preserve lowest-instance diagnostics in parallel eval#1749

Closed
latifkasuli wants to merge 3 commits into
binius-zk:mainfrom
latifkasuli:fix-valuetable2-parallel-diagnostics-order
Closed

Preserve lowest-instance diagnostics in parallel eval#1749
latifkasuli wants to merge 3 commits into
binius-zk:mainfrom
latifkasuli:fix-valuetable2-parallel-diagnostics-order

Conversation

@latifkasuli

Copy link
Copy Markdown
Contributor

Summary

Restores deterministic error selection in EvalForm::evaluate_batched_parallel.

The parallel batch evaluator documents that when several instances fail, diagnostics report the lowest-index failing instance, matching serial batch evaluation. The current collect::<Result<Vec<_>, _>>() form short-circuits on a Rayon scheduling-dependent stripe error, so the reported instance can vary when failures occur in different stripes.

This collects all stripe results and explicitly returns the error with the minimum instance, preserving the documented serial-compatible behavior.

Repro

On clean origin/main, this existing test is flaky under Rayon:

for i in $(seq 1 50); do
  RAYON_NUM_THREADS=8 cargo test -p binius-m4-prover --features rayon \
    value_table2::tests::parallel_failure_diagnostics_match_serial_for_failures_in_different_stripes \
    --lib -- --exact || exit $?
done

I reproduced a failure on run 2: serial reported instance 5, while parallel reported instance 7.

Test plan

  • RAYON_NUM_THREADS=8 repeated flaky-test repro: 50/50 passes after the fix
  • cargo test -p binius-m4-prover --features rayon --lib
  • cargo test -p binius-m4-prover --lib
  • cargo test -p binius-frontend --lib
  • cargo +nightly-2026-01-01 fmt --check
  • cargo clippy -p binius-frontend -p binius-m4-prover --all-features --tests --benches -- -D warnings

@latifkasuli latifkasuli marked this pull request as ready for review July 9, 2026 15:35
@latifkasuli latifkasuli requested a review from jimpo as a code owner July 9, 2026 15:35
@jimpo

jimpo commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

I don't think this is important. Why do you think it is?

@jimpo jimpo closed this Jul 9, 2026
@jimpo jimpo reopened this Jul 9, 2026
@latifkasuli

Copy link
Copy Markdown
Contributor Author

Agreed that this is not important for proof correctness: the witness is invalid either way, and verifier/prover behavior is not affected.

The reason I thought it was worth fixing is narrower: this preserves the existing diagnostic contract and removes an actual flaky test on main. BatchPopulateError documents that batched evaluation reports the lowest failing instance, matching serial evaluation. With collect::<Result<Vec<_>, _>>(), Rayon can return whichever failing stripe gets observed first, so the existing cross-stripe test intermittently reports instance 7 instead of serial's 5. I reproduced that on clean main (failed on run 2 with RAYON_NUM_THREADS=8), while this PR made the same reproducer pass 50/50 locally.

So this is mainly a deterministic diagnostics / CI flake fix, not a security or proof correctness fix. If you prefer the parallel path to report any failing instance instead, I can close this and we should update the doc comment plus the test to stop requiring serial-compatible lowest-instance diagnostics.

@latifkasuli

Copy link
Copy Markdown
Contributor Author

Closing this in favor of the lighter fix: keep the parallel short-circuit behavior and update the diagnostic contract/test so the parallel path is only required to report a failing instance, not the lowest failing instance. That matches the feedback that deterministic lowest-instance diagnostics are not important for correctness.

@latifkasuli

Copy link
Copy Markdown
Contributor Author

Clarification: by "parallel short-circuit behavior" I mean the current collect::<Result<Vec<_>, _>>() implementation.

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.

2 participants