Core: sweep every transformers model_type across a process pool - #8995
Merged
Conversation
test_compile_every_transformers_model_type walks every model_type the
matrix's transformers ships and compiles each one. It was a serial loop, it
was the second most expensive step in Core at 415.7s, and it runs once per
matrix leg, so it cost about 21 minutes of runner time per push.
The work is independent per model and it is compile bound, not import bound.
Measured over the 359 model_types that have a modeling module:
importing 2.6s total 0.007s mean
compiling 81.8s total 0.228s mean
So there is nothing to shave inside the loop; the only lever is running it on
more than one core.
Uses spawn rather than fork, since the workers start after torch is already
loaded in the parent. Each worker re-imports the shim and gets its own
hermetic cache directory. Nothing in this test asserts on the cache path; the
per-model file assertions are in test_compile_real_modeling_module, which is
unchanged and still runs in process. Ordered imap keeps the known and
new-failure report in the same model order as before.
Verified identical, not just equally green: every one of the 383 model_types
gets the same verdict serially and pooled, 0 differences.
serial 85.0s ok=359
pooled 25.7s ok=359 4 workers
diffs 0
Whole step locally: 84.93s to 26.13s, 5 passed and 1 skipped either way.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
A worker that dies mid-task (segfault, OOM killer) never sets its result, and Pool.imap then blocks forever: python/cpython#66587, open since 2014. _maintain_pool replaces the worker but the in-flight job's cache entry is never set, so the parent waits on a condition nothing will signal. The serial loop could not lose a worker -- a segfault killed pytest outright and turned the leg red in seconds. Pooled, the same crash would sit until the job's 35 minute timeout, which costs far more than running the sweep pooled ever saves. Drive the iterator with next(timeout=600) and turn a stall into a named AssertionError. chunksize drops from 4 to 1, and that is required rather than incidental: Pool.imap only returns an IMapIterator, the thing that has a timeout, when chunksize == 1. For chunksize > 1 it returns a bare generator expression over the chunks (Lib/multiprocessing/pool.py:396-420), which has no next(timeout=...) at all. Per-task IPC is noise next to a ~1s compile: chunksize=4 26.13s chunksize=1 28.39s both ok=359 skipped=24 known-broken=0 new-failures=0, matching serial. Guard verified against a worker killed mid-task: fires after the budget with the results collected so far, instead of hanging.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
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
1 participant
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.
Compiler full-model-sweepis the second most expensive step in Core at 415.7s, and it runs once per matrix leg, so it costs about 21 minutes of runner time per push.Where the time goes
The step is 6 tests. One of them is essentially all of it:
That test walks every model_type the matrix's transformers ships and compiles each one, in a serial loop.
There is nothing to shave inside the loop. Splitting the cost over the 359 model_types that actually have a modeling module:
97% is compilation. The only lever is running it on more than one core.
For the record, the step's own comment estimated about 110s (383 models at ~0.3s). That estimate is accurate; it just describes a faster machine than the runner. At 415.7s the runner is doing ~1.16s per model. There is no mystery overhead here, which is worth stating since I first went looking for one.
The change
Run the sweep over a process pool. The models are independent: each compile reads one transformers modeling module and writes its own cache file.
test_compile_real_modeling_module, which is unchanged and still runs in process.SIGALRMbudget moves into the worker, in its own main thread, so it still bounds a single infinite-looping model_type. That guard exists because transformers >=5,<6 once hung for 30+ minutes.imap, so the known / new-failure report keeps the same model order it had as a serial loop.Classification, the report, and both assertions (
not new_failures,ok >= 200) are untouched.Verification
Equally green is not enough for a hard gate, so I checked that every model_type gets the same verdict, not just the same totals:
Whole step locally, before and after:
ok=359 skipped=24 known-broken=0 new-failures=0(359 + 24 = 383).3.25x locally. On the runner that maps to roughly 415.7s to ~130s per leg.
Caveat
This is validated on my machine, not on a GitHub runner, and it is a hard gate. Worth running staging CI on it before merging rather than taking the local numbers on faith.