fix: account for len(thresholds) when dispatching the vectorized PR-curve update (closes #3299) - #3452
Open
Kayvan-Zahiri wants to merge 2 commits into
Open
Conversation
Kayvan-Zahiri
requested review from
SkafteNicki and
justusschock
as code owners
August 11, 2026 17:01
…urve update The vectorized update paths materialise a preds.numel() * len(thresholds) intermediate, but the dispatch only looked at the input size. Passing a large thresholds argument therefore silently broke the constant memory behaviour that argument is documented to provide, allocating 288 MB for a 60000 x 3 input with 200 thresholds. Bound the intermediate as well as the input. Both update paths already return identical state, so falling back to the loop only trades speed for memory. Closes Lightning-AI#3299
for more information, see https://pre-commit.ci
Kayvan-Zahiri
force-pushed
the
fix/prc-threshold-aware-dispatch
branch
from
August 24, 2026 18:35
564560f to
be4a47a
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.
Closes #3299.
The bug
thresholdsis documented to bound memory, but the dispatch that chooses betweenthe vectorized and loop update paths never looks at how many thresholds there are.
The vectorized path then allocates
preds.numel() * len(thresholds)elements atand again for
unique_mapping. For the reported case,predsof shape(60000, 3)withthresholds=200, the gate computes540,000 <= 1,000,000andpicks the vectorized path, which then asks for 288 MB of int64, twice over:
MPS surfaces this as an OOM because its ceiling is lower, but the memory model is
wrong on every device. CPU just absorbs it.
The fix
Bound the intermediate as well as the input. The binary dispatch has the same
defect (
preds.numel() <= 50_000, also blind tolen_t), so both are fixed.This is a pure narrowing: a case that previously went vectorized either still does,
or falls back to the loop. Nothing that used the loop before is now vectorized, so
no workload gets slower unless it was also allocating hundreds of megabytes.
Why this is safe
The two paths return identical state. I verified across thresholds of 3, 7, 20 and
200 for multiclass and 3, 20, 200 for binary, on both the vectorized and loop
implementations, with
torch.equalin every case. Falling back only trades speedfor memory, never correctness.
Tests
test_multiclass_update_memory_does_not_scale_with_thresholds, parametrized over5 and 200 thresholds, asserts which path the dispatch actually picks. The 200 case
fails on
mainand passes here; the 5 case passes both ways, which pins that smallthreshold counts keep their current fast path.
test_multiclass_update_paths_agreeandtest_binary_update_paths_agreepin theinvariant the fix depends on.
Full file: 214 passed, 24 skipped, 4 xfailed, no regressions.
One question for a reviewer
_MAX_VECTORIZED_ELEMENTS = 1_000_000(8 MB of int64, doubled byunique_mapping)is my choice, not something derived from an existing constant. It keeps the reported
case out of the vectorized path with room to spare, but the right budget is really a
maintainer call and I would happily change it. If you would rather express it as a
byte budget, or scale it from the existing
50_000/1_000_000gates, say whichand I will update.
Not covered here
_multilabel_precision_recall_curve_updatehas the samepreds.numel() * len_tallocation and no loop fallback to dispatch to, so it is unconditionally exposed.
Fixing it means writing a multilabel loop implementation, which felt like a separate
change from the reported bug. Happy to add it to this PR or open a follow-up,
whichever you prefer.