Skip to content

Normalize FloatParameter values to float to stabilize task_id (#3243) - #3448

Open
jaideeppyne wants to merge 3 commits into
spotify:masterfrom
jaideeppyne:fix/float-parameter-normalize
Open

Normalize FloatParameter values to float to stabilize task_id (#3243)#3448
jaideeppyne wants to merge 3 commits into
spotify:masterfrom
jaideeppyne:fix/float-parameter-normalize

Conversation

@jaideeppyne

Copy link
Copy Markdown

Description

Fixes #3243 ("int values to FloatParameter crashes worker").

FloatParameter defines only parse and inherits the base no-op normalize (which returns the value unchanged) and the base serialize (str(x)). Nothing coerces a stored value to float, so an int input survives as an int:

p = FloatParameter()
p.serialize(5)    # -> "5"
p.serialize(5.0)  # -> "5.0"

Two logically-equal values (5 == 5.0) therefore produce different task_ids. Across the scheduler↔worker boundary the assistant worker keys its scheduled tasks by the server's id but looks them up by the locally recomputed id, so the divergence surfaces as a KeyError that crashes the assistant (exactly the traceback in the issue).

Verified on master:

Foo(args=5).task_id    -> Foo_5_cecf3ca63d
Foo(args=5.0).task_id  -> Foo_5_0_a9203c326b     # diverge

(Within one process the instance cache masks it because 5 == 5.0; clearing Register.clear_instance_cache() between instantiations — as the reporter does — exposes it.)

Fix

Add FloatParameter.normalize to coerce the value to float (with a None guard so OptionalFloatParameter still works). normalize is applied to every parameter value, so T(5) and T(5.0) canonicalize to 5.0 → the same task_id. With the fix both yield Foo_5_0_a9203c326b. This mirrors the reporter's own serialize(float(x)) workaround, applied at the right layer.

Tests

test/parameter_test.py:

  • test_float_normalizes_int_to_floatFloatParameter().normalize(5) is 5.0 (a float), and equal int/float values serialize identically.
  • test_float_task_id_stable_for_int_and_float — a task with args=5 vs args=5.0 (instance cache cleared between) yields the same task_id.

Both fail on master and pass with the fix.

`FloatParameter` defined only `parse` and inherited the base no-op `normalize`,
so an `int` value (e.g. `5`) passed to a `FloatParameter` was stored as an `int`.
`serialize(5)` -> `"5"` while `serialize(5.0)` -> `"5.0"`, so two equal values
(`5 == 5.0`) produced different `task_id`s. Because the assistant worker keys
scheduled tasks by the server's id but looks them up by the locally recomputed
id, this divergence surfaces as a `KeyError` that crashes the worker (spotify#3243).

Add `FloatParameter.normalize` to coerce to `float` (guarding `None` for the
optional variant), so `T(5)` and `T(5.0)` canonicalize to the same value and
task_id. Verified against the package: without the fix the two task_ids diverge
(`Foo_5_...` vs `Foo_5_0_...`); with it both are `Foo_5_0_...`.

Adds unit tests: `FloatParameter().normalize(5)` is `5.0` (a `float`) and equal
int/float values serialize identically; and a task with `args=5` and `args=5.0`
(with the instance cache cleared between, as in the report) yields the same
task_id.

Fixes spotify#3243
@jaideeppyne
jaideeppyne requested a review from a team August 18, 2026 06:31
Normalizing an int to float coerced the stored value, which suppressed the
existing "value is not of type float" warning (breaking test_warning and
test_optional_float_parameter). Override serialize() instead: it coerces to
float only for the string/task_id representation, so equal int/float values
share a task_id while the stored value's type — and hence the warning — is
unchanged. This matches the reporter's own serialize(float(x)) workaround.
str(float(x)) raised ValueError on a non-numeric value (e.g. a wrong-type
'bad data'), breaking test_optional_float_parameter which expects the value to
reach the type-warning path rather than crash. Coerce only int (bool excluded)
to float; serialize anything else as-is.
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.

int values to FloatParameter crashes worker

1 participant