Repository navigation
test(autotune): stop pinning the rep-scaling test to an exact ceiling - #990
Merged
Merged
Conversation
`timed_repetitions_scale_inversely_with_launch_cost` asserted that a cheap fake launch lands on exactly 64 repetitions. It passed in isolation and failed under full-suite parallelism, where more than 1200 tests contend for the machine. The cause is that `FakeOp::run` simulates cost with `std::thread::sleep`, which can overshoot its request but never undershoot it. Contention inflates the measured per-iteration cost, the budget therefore buys fewer repetitions, and the realized count falls below the ceiling. Nothing about the autotuner is wrong; the assertion was measuring the host rather than the property. The property under test is that sampling effort scales inversely with launch cost, so the test now asserts that shape instead of one number: the cheap launch must sample well above the floor and stay within the ceiling, the costly launch must reach the floor exactly, and cheap must outsample costly. The costly direction stays an equality because overshooting a 20ms sleep only exhausts the budget sooner, so the floor is reached under any amount of load. Introduced in #906 and found while validating a later sub-issue of epic #909, where it was the only new failure in an otherwise clean regression run.
inureyes
added
type:test
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.
Summary
autotune::autotune_tests::timed_repetitions_scale_inversely_with_launch_costis flaky. It passes in isolation and fails under full-suite parallelism, where more than 1200 tests contend for the machine. This replaces the load-sensitive assertion with the property the test actually exists to check.Cause
FakeOp::runsimulates launch cost withstd::thread::sleep, which can overshoot its requested duration but never undershoot it. Under contention a declared 100us launch measures far slower, so the fixed 50ms sample budget buys fewer repetitions and the realized count lands below the ceiling. The test then failed itsassert_eq!(reps, 64). Nothing in the autotuner is wrong; the assertion was measuring host load rather than the behaviour under test.Fix
The property is that sampling effort scales inversely with launch cost, not that a cheap launch reaches one specific number. The test now asserts that shape: the cheap launch samples well above the floor and stays within the ceiling, the costly launch reaches the floor exactly, and cheap outsamples costly. The costly direction keeps its equality deliberately, because overshooting a 20ms sleep only exhausts the budget sooner, so the floor is reached under any amount of load and that assertion cannot go flaky in the same way.
Verification
Run on Apple M1 Ultra with
--features metal,accelerate, both in isolation and under the full-suite condition that originally exposed the failure.cargo test --release -p mlxcel-core --lib --features metal,accelerate autotune::gives 55 passed.cargo test -p mlxcel-core --release --features metal,accelerategives 1203 passed, 1 failed, where the single remaining failure is the pre-existingfused_moe_parity_tests::fused_moe_geglu_kernel_matches_references_gemma4_shapetracked in fix(tests): fused MoE GeGLU parity threshold is tighter than the gather_qmm reference's own jitter #964 and unrelated to this change.cargo clippy -p mlxcel-core --lib --tests --features metal,accelerate -- -D warningsandcargo fmt --all -- --checkare clean.Context
Introduced by #906 and found while validating a later sub-issue of epic #909, where it was the only new failure in an otherwise clean regression run. Worth fixing rather than tolerating, because this repository's CI does not run the test suite, so a local full-suite run is the project's real gate and an intermittent failure there costs someone a misattributed investigation.
Refs #906, #909