{{ message }}
Measure the streaming stripper against CPU time, not wall clock - #9035
Merged
Conversation
E AssertionError: early markup cost 1.451s against the reference's 1.316s E assert 1.4506334609999385 <= (1.3157524960000728 * 1.1) Introduced by #8538 (2026-08-12), which added the test with time.perf_counter. The source it guards is fine and is untouched here; the test has measured the wrong quantity since it was written. It compares how much WORK two code paths do, with a 10% margin, but timed them on the wall clock, which also measures whatever else the machine is running. A 10% margin does not survive that. On a 4-vCPU runner with other pytest workers in flight it failed while the code under test had not changed. process_time measures this process's CPU time, which is what the assertion is about and what neighbours cannot move. The margin and the min-of-3 sampling are untouched. I have NOT reproduced the original failure locally, so this is not verified as a fix for that specific run: three busy processes on the same core slow both arms equally and the ratio holds. What is demonstrated is the mechanism, on identical work: idle core: wall=0.0161s cpu=0.0160s core shared with 3 hogs: wall=0.0599s cpu=0.0150s Wall clock inflates 3.7x, CPU time does not move. Robustification: process_time is coarser than perf_counter, and 0.0 <= 0.0 * 1.10 is true, so a stopped clock or a lowered count would silently turn this into an assertion that cannot fail. Both arms now have to measure something. Checked by mutating the clock to return 0.0: reference arm measured 0.0000s; too small to compare against Whole file: 64 passed.
danielhanchen
force-pushed
the
fix-streaming-stripper-timing
branch
from
August 17, 2026 01:17
14a773e to
9b5fc2a
Compare
for more information, see https://pre-commit.ci
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Member
Author
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.

Cause
The test compares how much work two code paths do, with a 10% margin:
elapsedusedtime.perf_counter, which measures wall clock, which also measures whatever else the machine is doing. A 10% margin does not survive that. On a 4-vCPU runner with other pytest workers in flight it fails while the code under test has not changed.time.process_timemeasures this process's CPU time. That is the quantity the assertion is actually about, and neighbours do not move it. The 10% margin is untouched and bothmin-of-3 samples stay.What is and is not verified
I have not reproduced the original failure locally, so I cannot claim this is verified as a fix for that specific run. Three busy processes pinned to the same core slow both arms equally, so the ratio holds and both clocks pass (5 runs each, all green).
What is demonstrated is the mechanism, on identical work:
Wall clock inflates 3.7x under contention. CPU time does not move.
Whole file: 64 passed.
Which PR, and which side is wrong
Introduced by #8538 (2026-08-12), which added this test using
time.perf_counter. The source it guards is fine and is untouched here; the test has measured the wrong quantity since it was written.Robustification
process_timeis coarser thanperf_counter, and0.0 <= 0.0 * 1.10is true. A stopped clock, or acountsomeone lowers, would silently turn this into an assertion that cannot fail. Both arms now have to measure something, with a 0.05s floor that sits far below the ~1.3s each arm actually takes and far above the clock's resolution. Mutatingelapsedto return0.0:Found while measuring whether the backend suite can run under
pytest-xdist. This is the one failure in that set that is not an order dependency: it is a timing assertion that cannot share a machine, and I would rather fix the clock than widen the margin.