Skip to content

perf(int32): auto-widen labels to int64 on overflow instead of raising#822

Open
FabianHofmann wants to merge 5 commits into
masterfrom
perf/int32-auto-widen
Open

perf(int32): auto-widen labels to int64 on overflow instead of raising#822
FabianHofmann wants to merge 5 commits into
masterfrom
perf/int32-auto-widen

Conversation

@FabianHofmann

@FabianHofmann FabianHofmann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

I argue that auto-adjustment of dtypes is the way to go and not over-complex in the implementation. this allows user to go wild with their problem sizes.

Note

The following content was generated by AI.

Stacked on #566 (perf/int32). Answers the "auto-adjust the dtype once the boundary is hit" question from #566's review, replacing the hard ValueError overflow guard with automatic widening.

What changed

When add_variables / add_constraints would push the label counter past the int32 maximum (~2.1 billion), instead of raising, options["label_dtype"] is widened to int64:

  • Monotonic — only ever int32 → int64, never back.
  • Sticky — survives options.reset() (a new OptionSettings.widen_label_dtype() moves the default too), so once any model in the process mints a >2^31 label, int64 becomes the floor everywhere for the rest of the process. Revert explicitly with options["label_dtype"] = np.int32.

Why sticky-global rather than per-model

The four float→int cast sites in expressions.py / common.save_join run during expression building and have no reliable model reference (expressions can be detached). A per-model dtype would leave those sites able to silently narrow an already-int64 label back to int32; guarding them with a per-array .max() would tax the exact build path #566 speeds up. Reading the process-global keeps them correct at zero hot-path cost, and the monotonic+sticky invariant guarantees no array is ever asked to narrow int64 values.

Previously-allocated int32 batches stay valid (they only ever held small values) and upcast to int64 on concat — verified that a widened label 2147483648 survives a variables.flat / constraints.flat round-trip without wrapping.

Trade-off

A process that builds one >2.1B-label model then many small ones keeps int64 labels throughout (losing the int32 memory/speed win for the small ones). Documented in the release note and user-overridable.

Tests

test/test_dtypes.py: the two test_overflow_guard_* tests (which asserted ValueError) are replaced with test_auto_widen_variables / test_auto_widen_constraints (assert labels become int64 and the option flips) and test_auto_widen_is_sticky (widen survives reset). A restore_label_dtype fixture resets the sticky global so the widen tests don't leak into the rest of the suite.

@FabianHofmann FabianHofmann requested a review from FBumann July 14, 2026 09:51
@FBumann

FBumann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@FabianHofmann I would say auto-widening should stay a convenience. Users should get a Warning.
Its recommended to set the dtype explicitly if large models are expected.
This will probably be faster, need less memory and cant lead to upcasting issues.

@FabianHofmann

Copy link
Copy Markdown
Collaborator Author

@FBumann good point, doing this now

Replace the int32 overflow ValueError in add_variables/add_constraints
with a monotonic, sticky widen of options["label_dtype"] to int64. Once
a model crosses ~2.1 billion labels the process uses int64 labels
everywhere (surviving options.reset()), so no cast site can silently
narrow an int64 label back to int32. Mixed int32/int64 arrays upcast on
concat, so previously-allocated int32 batches stay valid.
read_netcdf reconstructs label arrays directly and restores the counters
without going through the add_variables/add_constraints widen path, so a
model that had auto-widened to int64 would load with int64 label arrays
while the process option stayed int32 -- the silent-downcast corner,
reachable via read_netcdf and solve(remote=...). Widen the option on load
whenever a restored counter exceeds the int32 maximum.
…odels

Address review: keep auto-widening a convenience but emit a UserWarning
at the int32->int64 transition, advising users to set label_dtype=int64
upfront (avoids the mid-build upcast, faster and lighter).
@FabianHofmann FabianHofmann force-pushed the perf/int32-auto-widen branch from 244e37c to d338858 Compare July 14, 2026 10:26
@FabianHofmann FabianHofmann changed the base branch from perf/int32 to master July 14, 2026 10:26
@codspeed-hq

codspeed-hq Bot commented Jul 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 173 untouched benchmarks
⏩ 173 skipped benchmarks1


Comparing perf/int32-auto-widen (e215297) with master (4ca8d5a)

Open in CodSpeed

Footnotes

  1. 173 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@FBumann

FBumann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

We need to rerun the codpseed after master finished with the original in32 PR

@FabianHofmann

Copy link
Copy Markdown
Collaborator Author

We need to rerun the codpseed after master finished with the original in32 PR

do you know how to retrigger it?

@FBumann

FBumann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@FabianHofmann pushing an empty commit

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@FBumann

FBumann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

@FabianHofmann I think if we want to have an auto-widening that doesnt risk silent corruption. Therefore we need to store the labels dtype on the model, read from global options on creation, and optionally overwritable.

Without it we will have issues with:

  • uncatchable errors when a user reverts the global option while an int64 Model lives
  • pickling and reloading a Model with int64

Also, Expressions actually always have a related Model (cant be detachted!)

@FBumann FBumann left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go for a per Model dtype instead. See #828

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants