Skip to content

refactor(int32): per-model label dtype instead of sticky process-global#828

Merged
FabianHofmann merged 15 commits into
masterfrom
perf/int32-model-label-dtype
Jul 15, 2026
Merged

refactor(int32): per-model label dtype instead of sticky process-global#828
FabianHofmann merged 15 commits into
masterfrom
perf/int32-model-label-dtype

Conversation

@FBumann

@FBumann FBumann commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

This implements int dtype control per Model instead of through a global config option (global kept).
This resolves all potential issues with the global for little complexity.
I think this is the right approach.

Note

The following content was generated by AI.

Builds on the auto-widening behavior from #822 (now closed; this PR rebased onto master), but moves the label dtype from the sticky process-global option onto the Model itself.

What changed

  • The label dtype becomes a Model() argument: Model(dtypes={"labels": np.int64}), default np.int32. It is exposed read-only as Model.dtypes — a types.MappingProxyType mapping (a frozen dict) with a single "labels" field for now, extensible to more fields later (per review feedback). Unknown keys and non-int32/int64 dtypes are rejected. The linopy.options["label_dtype"] setting is removed entirely — embedders forward constructor kwargs instead (PyPSA: n.optimize(model_kwargs={"dtypes": {"labels": np.int64}})), so config.py returns to its pre-perf: default integer arrays to int32 for ~25% memory reduction #566 state.
  • A model widens only itself to int64 when its labels would exceed the int32 maximum; the UserWarning recommends passing dtypes={"labels": np.int64} upfront.
  • All cast sites read the dtype from their object's model reference; common.save_join takes it as an explicit fill_dtype parameter instead of reading the global.
  • read_netcdf restores the widened dtype per model (from the counters, as before). Pickle round-trips preserve it automatically since it is an instance attribute.
  • OptionSettings.widen_label_dtype() and its sticky-reset() semantics are removed, along with the test-suite fixture that guarded against global-state leakage.

Why per-model

#822 chose the process-global because "the four float→int cast sites … have no reliable model reference (expressions can be detached)". That premise doesn't hold in the current codebase: BaseExpression.__init__ raises unless model is a Model instance, and every label_dtype cast site is a method on an object with a validated model reference (BaseExpression, Variable, Variables, Constraints, CSRConstraint). The only model-less site, save_join, is called with integer_dtype=True from exactly three container properties that all have self.model in scope.

With a reliable model reference, a per-model monotonic widen gives the same no-narrowing guarantee at the same zero hot-path cost (an attribute read), and removes the global design's edge cases:

  • No revert footgun. Under the global design, following the documented revert (options["label_dtype"] = np.int32) while a widened model is alive lets the cast sites silently narrow >2^31 labels — int64 → int32 .astype() wraps to negative values without any error. Per-model, there is nothing to revert.
  • Widening travels with the model. A widened model shipped to another process via pickle/cloudpickle (dask, multiprocessing) keeps its dtype; the global flag does not cross process boundaries.
  • No cross-model contamination. perf(int32): auto-widen labels to int64 on overflow instead of raising #822 documents that one huge model permanently costs every later small model in the process its int32 memory/speed win. Per-model, small models are unaffected.
  • One knob in one place: the constructor argument. Nothing needs test isolation.
Cast-site → model-reference verification
Site Enclosing scope Model access
expressions.py vars cast in init BaseExpression.__init__ model param, enforced isinstance(model, Model) (check moved above the cast)
expressions.py sanitize BaseExpression self.model
expressions.py simplify (2×) LinearExpression self.model
variables.py ffill/bfill/sanitize Variable self.model (init enforces isinstance(model, Model))
variables.py flat remap Variables self.model
constraints.py _to_dataset (2×) CSRConstraint self._model
constraints.py flat remap Constraints self.model
common.py save_join free function dtype passed by callers (Variables.labels, Constraints.labels, Constraints.vars)

Tests

test/test_dtypes.py: the widen tests assert other models stay int32; the init-arg test drives the dtypes={"labels": ...} mapping (+ invalid-dtype and unknown-key rejection), plus test_dtypes_is_read_only_mapping (mutating m.dtypes raises TypeError), test_widen_applies_to_expressions, and test_auto_widen_survives_pickle; the netcdf test asserts the loaded model is widened. The restore_label_dtype fixture and the option tests are gone. Full suite passes.

🤖 Generated with Claude Code

FabianHofmann and others added 10 commits July 14, 2026 12:25
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).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ss-global

Store the label dtype on the Model (snapshot of options['label_dtype'] at
creation, exposed as Model.label_dtype) and widen only that model to int64
on overflow. Every cast site reads the dtype from its object's model
reference; save_join takes it as an explicit fill_dtype parameter.

This removes the sticky global state: the option and other models are
unaffected by one model's widening, reset() keeps its usual semantics,
and the widened dtype travels with the model through netCDF and pickle
round-trips instead of relying on process-global stickiness.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Model(label_dtype=np.int64) is more direct than mutating the global
option before construction; None falls back to options['label_dtype']
so embedders that construct the Model internally keep a process-wide
default.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extract Model._allocate_labels (shared by variable and constraint label
assignment) and config.validate_label_dtype (shared by the option setter
and Model.__init__).

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

The option predated the constructor argument; with per-model storage it
only duplicated the knob. Embedders forward constructor kwargs (PyPSA:
n.optimize(model_kwargs={'label_dtype': np.int64})), so no process-wide
setting is needed. config.py returns to its pre-int32 state.

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

@FabianHofmann FabianHofmann 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.

generalize label_dtype accessor to dtypes which is property pointing to a frozen dict with fields "labels" at first stage. can be expanded later.

Comment thread linopy/model.py Outdated

def _allocate_labels(self, start: int, end: int) -> np.ndarray:
"""Return the label range ``[start, end)``, widening the dtype on overflow."""
if end > np.iinfo(self._label_dtype).max:

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.

should only be true if dtype!=int64

@FBumann
FBumann changed the base branch from perf/int32-auto-widen to master July 15, 2026 12:42
Address review: generalize the per-model label-dtype accessor into a
`Model.dtypes` property returning a read-only mapping (`MappingProxyType`)
with a single `"labels"` field for now, extensible to more fields later.
The constructor takes `dtypes={"labels": ...}` in place of `label_dtype=`;
unknown keys and unsupported dtypes are rejected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codspeed-hq

codspeed-hq Bot commented Jul 15, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 173 untouched benchmarks
⏩ 173 skipped benchmarks1


Comparing perf/int32-model-label-dtype (e63d92b) with master (c02f5db)

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 marked this pull request as draft July 15, 2026 13:08
FBumann and others added 4 commits July 15, 2026 15:12
Drop the redundant _label_dtype field: the model now holds one
`_dtypes: dict[DtypeKey, type]` as the single source of truth, and
`Model.dtypes` returns a read-only view over it. Keys are typed with a
`DtypeKey = Literal["labels"]` alias; the constructor validates against it
via `get_args`. All internal cast sites read `model._dtypes["labels"]`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the `dtypes` argument validation out of `__init__` into a small
`Model._resolve_dtypes` staticmethod that merges the argument onto the
defaults in one loop, rejecting unknown keys and non-int32/64 dtypes.
`__init__` now just assigns its result.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The alias name is self-explanatory; a Literal alias cannot carry a real
docstring anyway.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@FBumann
FBumann marked this pull request as ready for review July 15, 2026 13:49
@FBumann
FBumann requested a review from FabianHofmann July 15, 2026 13:49

@FabianHofmann FabianHofmann 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.

great!

@FabianHofmann
FabianHofmann merged commit cb1cba2 into master Jul 15, 2026
24 checks passed
@FabianHofmann
FabianHofmann deleted the perf/int32-model-label-dtype branch July 15, 2026 18:03
@coroa

coroa commented Jul 16, 2026

Copy link
Copy Markdown
Member

This implemented auto-widening the dtypes now?

@FBumann

FBumann commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

This implemented auto-widening the dtypes now?

Yes!
And this led to storing it on the model to avoid issues

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.

3 participants