From 1f761ce43be0e4043effdc0df4c54404b22d05ad Mon Sep 17 00:00:00 2001 From: Jammy2211 Date: Mon, 27 Jul 2026 14:58:23 +0100 Subject: [PATCH 1/3] fix: cast the MultiStart gradient chunk size to int MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The step loop does `for _ in range(iterations)`, but `AbstractSearch.__init__` stores `iterations_per_full_update` as a float (abstract_search.py:219) so the inf-like 1e99 config default is representable. The crash was latent because `min(1e99, steps_remaining)` returns the int operand — only a user-supplied cadence *below* the remaining budget reaches `range` and raises `TypeError: 'float' object cannot be interpreted as an integer`. Six RAL chain jobs died on it. Cast at the consumer via a new `_steps_in_chunk` helper rather than changing the shared float coercion, which every other search relies on. The helper also makes the defect testable without JAX: `_fit` needs jax, optax and a JAX-traceable Analysis, and the library suite is NumPy-only. Co-Authored-By: Claude Opus 5 --- .../search/mle/multi_start_gradient/search.py | 29 +++++++++- .../search/mle/test_multi_start_gradient.py | 58 +++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/autofit/non_linear/search/mle/multi_start_gradient/search.py b/autofit/non_linear/search/mle/multi_start_gradient/search.py index 6c8ea48b4..f3fcf1581 100644 --- a/autofit/non_linear/search/mle/multi_start_gradient/search.py +++ b/autofit/non_linear/search/mle/multi_start_gradient/search.py @@ -156,6 +156,31 @@ def __init__( self.logger.debug(f"Creating {self.optax_method} MultiStartGradient Search") + def _steps_in_chunk(self, steps_remaining: int) -> int: + """ + The number of gradient steps to run before the next ``perform_update`` + checkpoint boundary, given how much of the ``n_steps`` budget is left. + + ``AbstractSearch.__init__`` stores ``iterations_per_full_update`` as a + **float**, because the packaged default is the inf-like ``1e99`` (a + single chunk, i.e. checkpoint only at the end). ``range`` requires an + ``int``, so the chunk size is cast here at the consumer rather than in + the shared float coercion, which every other search relies on. + + Without the cast the loop only survives on the default: ``min(1e99, + steps_remaining)`` returns the ``int`` operand, so the float never + reaches ``range``. A user-supplied cadence *below* the remaining budget + (e.g. ``iterations_per_full_update=50`` with ``n_steps=3000``) does + reach it, and raises ``TypeError: 'float' object cannot be interpreted + as an integer``. + + Parameters + ---------- + steps_remaining + The number of steps left in the ``n_steps`` budget. + """ + return int(min(self.iterations_per_full_update or self.n_steps, steps_remaining)) + def _fit( self, model: AbstractPriorModel, @@ -281,9 +306,7 @@ def batched_value_and_grad(params): while total_steps < self.n_steps and stop_reason != "converged": steps_remaining = self.n_steps - total_steps - iterations = min( - self.iterations_per_full_update or self.n_steps, steps_remaining - ) + iterations = self._steps_in_chunk(steps_remaining) # Convergence is assessed every step (``fom_history`` updates every # step) rather than only at the ``iterations_per_full_update`` boundary, diff --git a/test_autofit/non_linear/search/mle/test_multi_start_gradient.py b/test_autofit/non_linear/search/mle/test_multi_start_gradient.py index 59ea78630..4412c35dd 100644 --- a/test_autofit/non_linear/search/mle/test_multi_start_gradient.py +++ b/test_autofit/non_linear/search/mle/test_multi_start_gradient.py @@ -136,6 +136,64 @@ def test__convergence_default_is_on_and_carried(): assert cls(convergence=custom).convergence is custom +def test__steps_in_chunk__real_cadence_is_an_int_range_can_consume(): + """The step loop does ``for _ in range(...)`` over the chunk size, but + ``iterations_per_full_update`` is stored as a float by ``AbstractSearch``. + + A cadence *below* the remaining budget is the case that reaches ``range`` + (with the ``1e99`` default, ``min`` returns the int ``steps_remaining`` + instead), and it used to raise ``TypeError: 'float' object cannot be + interpreted as an integer`` on step-loop entry. + """ + search = af.MultiStartAdam(n_steps=3000, iterations_per_full_update=50) + + # The knob really is stored as a float — this is what made the crash latent. + assert isinstance(search.iterations_per_full_update, float) + + iterations = search._steps_in_chunk(steps_remaining=3000) + + assert iterations == 50 + assert isinstance(iterations, int) + # The actual failing operation in ``_fit``. + assert len(list(range(iterations))) == 50 + + +def test__steps_in_chunk__clamps_to_the_remaining_budget(): + # Near the end of the budget the chunk shrinks to what is left, so the loop + # never overshoots ``n_steps``. + search = af.MultiStartAdam(n_steps=3000, iterations_per_full_update=50) + + assert search._steps_in_chunk(steps_remaining=20) == 20 + assert isinstance(search._steps_in_chunk(steps_remaining=20), int) + + +def test__steps_in_chunk__default_cadence_is_a_single_chunk(): + # The packaged default is the inf-like 1e99: one chunk covering the whole + # budget (checkpoint only at the end), which is why the crash never fired + # on a default run. + search = af.MultiStartAdam(n_steps=300) + + assert search.iterations_per_full_update == pytest.approx(1e99) + + iterations = search._steps_in_chunk(steps_remaining=300) + + assert iterations == 300 + assert isinstance(iterations, int) + + +def test__steps_in_chunk__falsy_cadence_falls_back_to_n_steps(): + # A falsy cadence means "no checkpoint boundary": fall back to the full + # ``n_steps`` budget rather than a zero-length chunk that would spin the + # while-loop forever. ``__init__`` cannot produce this (a falsy argument + # resolves from config to 1e99), so it is set directly — which is exactly + # what a caller overwriting the attribute post-construction does. + search = af.MultiStartAdam(n_steps=120) + search.iterations_per_full_update = 0.0 + + assert search._steps_in_chunk(steps_remaining=120) == 120 + assert isinstance(search._steps_in_chunk(steps_remaining=120), int) + + def test__check_if_converged__plateau_stops_climbing_does_not(): # rtol/atol both zero: converges only on an exactly-flat window. convergence = af.MultiStartGradientConvergence( From df6b77844fb306d8a98449025224948a2497eb40 Mon Sep 17 00:00:00 2001 From: Jammy2211 Date: Mon, 27 Jul 2026 15:31:47 +0100 Subject: [PATCH 2/3] fix: floor the MultiStart chunk size at 1 step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the previous commit: `int` truncates towards zero, so a fractional cadence below 1 (e.g. iterations_per_full_update=0.5) yields `range(0)` — no steps run, `total_steps` never advances, and the enclosing while-loop spins forever re-running `perform_update`. That traded a loud TypeError for a silent hang, which on HPC burns the whole allocation. Floor at 1: the slowest cadence that still progresses, and it can never overshoot `steps_remaining` (>= 1 whenever the loop runs). Co-Authored-By: Claude Opus 5 --- .../search/mle/multi_start_gradient/search.py | 12 +++++++++++- .../search/mle/test_multi_start_gradient.py | 12 ++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/autofit/non_linear/search/mle/multi_start_gradient/search.py b/autofit/non_linear/search/mle/multi_start_gradient/search.py index f3fcf1581..41b9663c2 100644 --- a/autofit/non_linear/search/mle/multi_start_gradient/search.py +++ b/autofit/non_linear/search/mle/multi_start_gradient/search.py @@ -174,12 +174,22 @@ def _steps_in_chunk(self, steps_remaining: int) -> int: reach it, and raises ``TypeError: 'float' object cannot be interpreted as an integer``. + The chunk is floored at 1 step because ``int`` truncates towards zero: a + fractional cadence below 1 would otherwise give ``range(0)``, so + ``total_steps`` would never advance and the enclosing ``while`` loop + would spin forever re-running ``perform_update``. One step per chunk is + the slowest *progressing* cadence, and it can never overshoot + ``steps_remaining``, which is at least 1 whenever the loop is entered. + Parameters ---------- steps_remaining The number of steps left in the ``n_steps`` budget. """ - return int(min(self.iterations_per_full_update or self.n_steps, steps_remaining)) + return max( + 1, + int(min(self.iterations_per_full_update or self.n_steps, steps_remaining)), + ) def _fit( self, diff --git a/test_autofit/non_linear/search/mle/test_multi_start_gradient.py b/test_autofit/non_linear/search/mle/test_multi_start_gradient.py index 4412c35dd..30abc814c 100644 --- a/test_autofit/non_linear/search/mle/test_multi_start_gradient.py +++ b/test_autofit/non_linear/search/mle/test_multi_start_gradient.py @@ -181,6 +181,18 @@ def test__steps_in_chunk__default_cadence_is_a_single_chunk(): assert isinstance(iterations, int) +def test__steps_in_chunk__fractional_cadence_never_yields_a_zero_length_chunk(): + """``int`` truncates towards zero, so a cadence below 1 would give + ``range(0)``: no steps run, ``total_steps`` never advances, and the step + loop's enclosing ``while`` spins forever. The chunk is floored at 1.""" + search = af.MultiStartAdam(n_steps=300, iterations_per_full_update=0.5) + + iterations = search._steps_in_chunk(steps_remaining=300) + + assert iterations == 1 + assert isinstance(iterations, int) + + def test__steps_in_chunk__falsy_cadence_falls_back_to_n_steps(): # A falsy cadence means "no checkpoint boundary": fall back to the full # ``n_steps`` budget rather than a zero-length chunk that would spin the From eac4b08671f1dbf5128f42785e1fed160cf9a302 Mon Sep 17 00:00:00 2001 From: Jammy2211 Date: Mon, 27 Jul 2026 15:54:29 +0100 Subject: [PATCH 3/3] fix: reject an unusable step cadence instead of clamping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows an adversarial review of the previous commit. `max(1, int(...))` silently laundered invalid input: -5, 0.5 and 50.9 all became a plausible-looking cadence, so a typo would quietly run a schedule the user never asked for. Validate instead, and name the bad value. The same review showed the "chunk can never overshoot steps_remaining" claim held only because `n_steps` is an integer — the loop guard proves steps_remaining > 0, not >= 1, so a float n_steps=2.5 leaves a 0.5 remainder that truncates to a zero-length chunk and hangs. n_steps is annotated `int` but never validated, so it is checked too; both checks together restore the invariant the floor was papering over. Also adds a wiring guard: every other test drives _steps_in_chunk directly, so all of them would still pass if _fit went back to computing the chunk inline — the exact regression this fix is about. Co-Authored-By: Claude Opus 5 --- .../search/mle/multi_start_gradient/search.py | 58 +++++++++++++++---- .../search/mle/test_multi_start_gradient.py | 50 +++++++++++++--- 2 files changed, 90 insertions(+), 18 deletions(-) diff --git a/autofit/non_linear/search/mle/multi_start_gradient/search.py b/autofit/non_linear/search/mle/multi_start_gradient/search.py index 41b9663c2..1df2b6d57 100644 --- a/autofit/non_linear/search/mle/multi_start_gradient/search.py +++ b/autofit/non_linear/search/mle/multi_start_gradient/search.py @@ -174,22 +174,60 @@ def _steps_in_chunk(self, steps_remaining: int) -> int: reach it, and raises ``TypeError: 'float' object cannot be interpreted as an integer``. - The chunk is floored at 1 step because ``int`` truncates towards zero: a - fractional cadence below 1 would otherwise give ``range(0)``, so - ``total_steps`` would never advance and the enclosing ``while`` loop - would spin forever re-running ``perform_update``. One step per chunk is - the slowest *progressing* cadence, and it can never overshoot - ``steps_remaining``, which is at least 1 whenever the loop is entered. + Both the cadence and ``n_steps`` are validated as whole numbers of at + least one step, rather than being coerced into something plausible. A + value below 1 (or a fractional one) truncates to ``range(0)``: no step + runs, ``total_steps`` never advances, and the enclosing ``while`` loop + spins forever re-running ``perform_update`` — a silent hang that on a + cluster burns the whole allocation. Clamping such a value to 1 would + hide the mistake instead of reporting it, so it raises here. Once both + are validated, ``steps_remaining`` is an ``int`` of at least 1 whenever + the loop is entered, so the returned chunk is at least 1 and never + overshoots the remaining budget. Parameters ---------- steps_remaining The number of steps left in the ``n_steps`` budget. + + Raises + ------ + ValueError + If ``n_steps``, or an explicitly supplied + ``iterations_per_full_update``, is not a whole number of at least 1. """ - return max( - 1, - int(min(self.iterations_per_full_update or self.n_steps, steps_remaining)), - ) + self._check_step_count(self.n_steps, "n_steps") + + # A falsy cadence means "no intermediate checkpoint": one chunk covering + # the whole budget. Only a value the user actually supplied is validated. + if self.iterations_per_full_update: + self._check_step_count( + self.iterations_per_full_update, "iterations_per_full_update" + ) + cadence = self.iterations_per_full_update + else: + cadence = self.n_steps + + return int(min(cadence, steps_remaining)) + + def _check_step_count(self, value, name: str): + """ + Reject a step count that cannot describe a whole number of gradient + steps. ``float`` values are allowed — the packaged config default + ``1e99`` is one — provided they are integral. + """ + try: + is_whole = value == int(value) + except (TypeError, ValueError, OverflowError): + is_whole = False + + if not is_whole or value < 1: + raise ValueError( + f"{type(self).__name__}: `{name}` must be a whole number of " + f"gradient steps and at least 1, but was {value!r}. A fractional " + "or sub-1 value gives a zero-length step chunk, which never " + "advances the search and would hang it rather than fail." + ) def _fit( self, diff --git a/test_autofit/non_linear/search/mle/test_multi_start_gradient.py b/test_autofit/non_linear/search/mle/test_multi_start_gradient.py index 30abc814c..818396550 100644 --- a/test_autofit/non_linear/search/mle/test_multi_start_gradient.py +++ b/test_autofit/non_linear/search/mle/test_multi_start_gradient.py @@ -1,3 +1,5 @@ +import inspect + import numpy as np import pytest @@ -181,16 +183,48 @@ def test__steps_in_chunk__default_cadence_is_a_single_chunk(): assert isinstance(iterations, int) -def test__steps_in_chunk__fractional_cadence_never_yields_a_zero_length_chunk(): - """``int`` truncates towards zero, so a cadence below 1 would give - ``range(0)``: no steps run, ``total_steps`` never advances, and the step - loop's enclosing ``while`` spins forever. The chunk is floored at 1.""" - search = af.MultiStartAdam(n_steps=300, iterations_per_full_update=0.5) +def test__fit_step_loop_takes_its_chunk_size_from_the_helper(): + """Wiring guard: every other test here exercises ``_steps_in_chunk`` + directly, so all of them would still pass if ``_fit`` went back to computing + the chunk inline — which is the exact regression this fix is about. - iterations = search._steps_in_chunk(steps_remaining=300) + ``_fit`` cannot be executed from this suite (it needs jax, optax and a + JAX-traceable ``Analysis``, and the library suite is NumPy-only), so the + call site is asserted at the source level instead. + """ + source = inspect.getsource(af.MultiStartAdam._fit) - assert iterations == 1 - assert isinstance(iterations, int) + assert "self._steps_in_chunk(" in source + # the un-cast inline form the crash came from must not come back + assert "self.iterations_per_full_update or self.n_steps" not in source + + +@pytest.mark.parametrize("cadence", [0.5, 50.9, -5]) +def test__steps_in_chunk__unusable_cadence_raises_rather_than_being_clamped(cadence): + """A cadence below 1 (or a fractional one) truncates to ``range(0)``: no + step runs, ``total_steps`` never advances, and the step loop's enclosing + ``while`` spins forever re-running ``perform_update``. + + Clamping such a value to 1 would hide the mistake and silently run a + cadence the user did not ask for, so it is rejected instead — a hang on a + cluster is far more expensive than an error at the first chunk boundary. + """ + search = af.MultiStartAdam(n_steps=300, iterations_per_full_update=cadence) + + with pytest.raises(ValueError, match="iterations_per_full_update"): + search._steps_in_chunk(steps_remaining=300) + + +@pytest.mark.parametrize("n_steps", [2.5, 0, -10]) +def test__steps_in_chunk__unusable_n_steps_raises(n_steps): + """``steps_remaining`` is only guaranteed to be an ``int`` of at least 1 + because ``n_steps`` is one. A fractional ``n_steps`` leaves a fractional + remainder (e.g. 0.5) that truncates to a zero-length chunk, so the budget + is validated too rather than assumed from its type annotation.""" + search = af.MultiStartAdam(n_steps=n_steps) + + with pytest.raises(ValueError, match="n_steps"): + search._steps_in_chunk(steps_remaining=1) def test__steps_in_chunk__falsy_cadence_falls_back_to_n_steps():