fix: apply the automatic BLAS thread cap without threadpoolctl, and on Sweep - #110
Merged
Conversation
…n Sweep Two defects in the automatic cap, fixed together because the second cannot land without the first. `_auto_cap_target` now declines when `threadpoolctl` cannot be imported, warning once per process and naming `OPENBLAS_NUM_THREADS`. Before, a plain `pip install cvx-quadprog` on Linux against OpenBLAS with more threads configured than physical cores raised `ImportError` from every `solve_qp` at or above `dynamic_n_thresh()` -- 256 on the fallback L2, 128 with `fast=True` -- on a call that never mentioned threading, with a message naming the `blas_threads` the caller had not passed. An opt-in extra was effectively mandatory on exactly the installation the stdlib-only probes exist to support. The gate is a defence nobody asked for, so a missing dependency now means no cap rather than no solve; an explicit `blas_threads=` still raises, because quietly not honouring a request is worse than saying so (#106). `Sweep` now consults the same gate, once per object since its `n` is fixed, and applies the answer to the `O(n^3)` factorisation in `__init__` and to every cache miss. It called `_solve_with_factors` directly, below the level `solve_qp` installs the context at, so the API most exposed to the 73x collapse -- large problems, solved repeatedly -- was the one path with no guard. Cache hits are deliberately left unwrapped: threadpoolctl costs ~100 us against an `O(nk)` recovery. `blas_threads=` is exposed on `Sweep.__init__` with the meaning it has on `solve_qp`, validated at construction because the factorisation is inside the cap (#107). Coverage stays at 100% of statements and branches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # O(n^3), so it is inside the cap. A bad `blas_threads` therefore raises | ||
| # here, at construction, rather than at the first solve. | ||
| with _threads.scoped_limit(self._blas_threads): | ||
| self._Rinv, _xu = _factorize(G, np.zeros(self.n), False) |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two threading-related defects in the BLAS thread auto-capping logic: it prevents the automatic cap from turning the optional threadpoolctl dependency into a hard runtime requirement, and it extends the same protection to the Sweep API (without wrapping its cache-hit hot loop).
Changes:
- Make the automatic cap degrade to “no cap + warn once” when
threadpoolctlis unavailable, while keeping explicitblas_threads=as a hard requirement. - Add
blas_threadssupport and automatic-cap application toSweep(factorisation + cache misses, not hits). - Update tests to account for optional availability of
threadpoolctl, and extend coverage for the new behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_threads.py | Adds skips when threadpoolctl is absent and adds regression/behavior tests for the auto-cap fallback and Sweep capping behavior. |
| src/cvx/quadprog/_threads.py | Adds threadpoolctl availability probing for the automatic cap, warns-and-declines when unavailable, and introduces scoped_limit(). |
| src/cvx/quadprog/_sweep.py | Adds blas_threads parameter and applies the cap to the factorisation and miss-path solves only. |
| README.md | Documents Sweep threading support and describes the automatic cap’s behavior and optional dependency behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+264
to
+266
| f"or set OPENBLAS_NUM_THREADS={cores} to cap the whole process instead.", | ||
| RuntimeWarning, | ||
| stacklevel=2, |
Comment on lines
+435
to
+436
| solve at `n = 10`. `blas_threads=` on `solve_qp` and on `Sweep` does the same | ||
| thing per call, and is used exactly as given. |
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.
Fixes #106. Fixes #107.
Two defects in the automatic BLAS thread cap. They land together because the
second cannot be fixed without the first: adding the cap to
Sweepassolve_qpinstalled it would have propagated #106's crash to a second API.#106 — an optional dependency reached from a path nobody opted into
solve_qpconsultsauto_cap_threadswheneverblas_threadsis omitted, thenenters
_threads.limit(...), which raises whenthreadpoolctl— an optionalextra — is absent. So on a plain
pip install cvx-quadprog, a process on Linuxwith NumPy against OpenBLAS and more threads configured than physical cores
raised
ImportErrorfrom every solve at or abovedynamic_n_thresh()(256on the 512 KB fallback, 128 with
fast=True), on a call that never mentionedthreading, with a message naming the
blas_threadsthe caller had not passed.The module docstring already stated the principle the code then broke — the
probes are written in stdlib and NumPy alone "so that they work on a plain
pip installof this package, which is exactly the installation the trap is setfor." The probes honoured that; the cap they fed turned the extra into a hard
requirement on precisely that installation, and turned a performance defence
into an availability failure.
What was missing is that the two callers of
limitwant opposite failure modes:blas_threads=Nis a request. A missing dependency must keep raising —silently not capping is not what the caller asked for. Unchanged.
makes
_auto_cap_targetdecline, which leaves the process exactly as it wasbefore the gate existed, with one warning per process pointing at
OPENBLAS_NUM_THREADS— which caps the whole process and needs nothinginstalled.
Availability is probed with the same
from threadpoolctl import threadpool_limitsthat
limitperforms, rather than afind_speccheck, so the two cannotdisagree: whatever makes
limitraise makes the gate decline. It is uncachedbecause its only caller is
_auto_cap_target, which is — so the import isattempted once per process either way, and a second cache would only be another
thing for a test to have to clear.
This was not reachable on the maintainers' machines (Accelerate exits at the
platform.system()test), which is why the suite passed at 100% branch coveragewith the defect in place.
#107 —
Sweephad no guard at allSweepcalls_solve_with_factorsdirectly, below the levelsolve_qpinstalls the context at, and
_sweep.pynever imported_threads. So the APImost exposed to the measured 73x collapse — large problems, solved repeatedly —
was the one path with no protection, and the cost was amplified rather than
equal: a
solve_qpcaller eats one bad solve, aSweepcaller eats it on__init__, on the firstsolve, and on every miss thereafter.Sweepnow consults the same gate once per object, sincenis fixed forits lifetime and so is the gate's answer, and applies it to the two blocks that
do
O(n^3)-or-worse BLAS work: the factorisation in__init__and the missbranch of
solve.Cache hits are deliberately left unwrapped. A hit is an
O(nk)recovery plusa KKT check, and
threadpoolctlcosts ~100 µs — wrapping it would tax exactlythe path the class exists to make cheap. Pushing the context down into
_solve_with_factorsinstead would have done precisely that, which is why it isat the call sites.
blas_threads=is exposed onSweep.__init__with the meaning it has onsolve_qp— an explicit count used as given, bypassing the gate. It isvalidated at construction rather than at the first solve, because the
factorisation is inside the cap.
Verification
The #106 regression, reproduced before and asserted after — a faked
oversubscribed machine with
threadpoolctlhidden as a plain install would haveit. Previously
ImportError; now the solve returns the right answer, with onewarning:
Where the cap lands for
Sweep, on the same faked machine:Every answer matches a cold
solve_qp— this is a performance knob on this pathtoo, and must be nothing else.
make allis green: 1143 tests, 100% of statements and branches on all tenmodules including the new code,
tyandmypy --strictclean,interrogateat100%,
make rhiza-testpassing.Two changes beyond the two fixes
pytest.importorskip("threadpoolctl")on two existing tests.test_auto_cap_threads_behaviorandtest_solve_qp_automatically_caps_large_problemsassert that a cap is applied, which now depends on the dependency being
importable. Without the skip they would fail confusingly on an install that
lacks it — a direct consequence of this change, so it is fixed here.
manual
threadpoolctlwrapping; it never mentioned the automatic cap at all,and the install note named only
solve_qp. Both brought up to date, includingwhat happens when
threadpoolctlis absent.Not done here
No performance numbers moved, so nothing in
README.mdordocs/paper/neededre-measuring under rule 3: the cap changes a thread count, not an algorithm, and
the hit path is untouched by design.
One commit rather than two, deliberately. Splitting it per issue would have left
scoped_limitunreferenced in the first commit and failed the 100% coveragegate, so a bisectable green history won over two changelog entries. Say if you
would rather have the two lines and I will restructure.
🤖 Generated with Claude Code