Share the strength GP fit across tests: 399s -> 135s - #39
Merged
Conversation
The Python suite took ~6.5 minutes, slow enough to discourage running it
before pushing. Profiling showed the time was not spread across the 270
tests. In a profiling run totalling 476 s, the five slowest entries
accounted for 456 s of it, and four of those were fits of the SAME
production strength GP (durations from that run, which was on a busier
machine than the before/after figures further down):
108.6s setup test_models.py::TestPredictiveQualityRegression
108.3s setup test_models.py::TestGetModelListWithCost
106.9s call test_lengthscale_identifiability
102.9s call test_strength_curve_monotonicity
They really are one fit. SustainableConcreteModel.fit_strength_model
calls fit_strength_gp(X, Y, Yvar, X_bounds) on data.strength_data, which
is exactly what the other two call directly; fit_strength_gp takes
seed=0 by default and applies it via torch.manual_seed, and every caller
uses that default; DATA_PATH is load_concrete_strength's default path.
Same inputs, same seed, same function.
test/shared_fits.py fits once per process and hands out copies. Isolation
is structural rather than by convention: the fitted objects live in a
closure with no module-level name bound to them, so the only way to reach
one is through an accessor, and every accessor deepcopies. Each property
that makes this safe was measured rather than assumed --
mutating a copy's parameters leaves the original untouched
a deepcopy costs ~0.00s against a ~100s fit
zero shared tensor identities and zero shared storage, copy vs original
Note fit_strength_model returns the model already in EVAL mode, so mode
changes are moot in practice; deepcopy isolates them regardless. deepcopy
also drops GPyTorch's prediction_strategy by design, so each copy rebuilds
its posterior caches rather than inheriting the original's -- posteriors
were checked bit-identical.
The provider drives the public fit_strength_model rather than reaching
for fit_strength_gp, so the production path under test is unchanged and
the two test_models classes still assert against a model built the way
they built it before.
Timings, same `make test-py`, same machine, coverage on (the 476 s
profiling run above was on a busier machine and is not the baseline):
before 405s, 393s
after 141s, 130s
Coverage stays at 100%, and 270 tests pass. Test ordering was varied
across runs while developing this (different modules first, and briefly
under xdist with several worker counts and distribution modes), which is
the evidence against order dependence creeping in with a shared fixture.
Parallelism was tried and deliberately removed. pytest-xdist did help
before the fits were shared (435s -> 200s), but afterwards it stopped
paying for itself -- 135s serial against ~184s at -n 4, since workers are
separate processes that each re-import torch and, without grouping, each
refit. It also came with a sharp edge: with the default --dist load the
shared fit scatters across workers and the suite measured 265s, slower
than not parallelising at all. Simpler to leave it out; the note above
test-py in the Makefile records the numbers so this is not rediscovered.
SebastianAment
force-pushed
the
test-suite-parallelism
branch
from
August 14, 2026 10:05
8d947cc to
8419493
Compare
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.
Speeds up the Python test suite from ~399 s to ~135 s (2.9×) with no change to what is tested.
The problem
The runtime wasn't spread across the 270 tests. In a profiling run totalling 476 s, the five slowest entries accounted for 456 s, and four of those were fits of the same production strength GP:
test_models.py::TestPredictiveQualityRegression(setup)test_models.py::TestGetModelListWithCost(setup)test_lengthscale_identifiabilitytest_strength_curve_monotonicity(Those durations come from that profiling run, which was on a busier machine than the before/after figures below — they are not directly comparable.)
They really are one fit:
fit_strength_modelcallsfit_strength_gp(X, Y, Yvar, X_bounds)ondata.strength_data— exactly what the other two call directly —fit_strength_gpdefaults toseed=0and applies it viatorch.manual_seed, andDATA_PATHisload_concrete_strength's default. Same inputs, same seed, same function. Verified bit-exactly: old and new paths producestate_dictdiffs of0.0.The fix
test/shared_fits.pyfits once per process and hands outdeepcopys. The fitted objects live in a closure with no module-level name bound to them, so no caller reaches one by accident.Measured rather than assumed:
deepcopycosts ~0.00 s against a ~100 s fitThe provider drives the public
fit_strength_modelrather than reaching forfit_strength_gp, so the production path under test is unchanged.Results
270 tests pass, coverage 100%, CI green on 3.11 and 3.12. Verified from a clean worktree of the commit itself, not just my working tree.
Why there is no parallelism here
pytest-xdistwas prototyped and dropped. It helped before the fits were shared (435 → 200 s, on a busier machine), but afterwards stopped paying for itself: 135 s serial against ~184 s at-n 4, since workers are separate processes that each re-import torch and, without grouping, each refit. With the default--dist loadthe shared fit scatters and the suite measured 265 s — slower than not parallelising at all. The numbers are in the Makefile so this isn't rediscovered.Risk
Test-only; no
boxcrete/changes. The thing worth a reviewer's eye is test isolation — a shared mutable fixture can create order-dependent failures. Ordering was varied deliberately (reverse module order, and under xdist with several worker counts and distribution modes) and all 270 passed in every arrangement.