Normalize FloatParameter values to float to stabilize task_id (#3243) - #3448
Open
jaideeppyne wants to merge 3 commits into
Open
Normalize FloatParameter values to float to stabilize task_id (#3243)#3448jaideeppyne wants to merge 3 commits into
jaideeppyne wants to merge 3 commits into
Conversation
`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
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.
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.
Description
Fixes #3243 ("int values to FloatParameter crashes worker").
FloatParameterdefines onlyparseand inherits the base no-opnormalize(which returns the value unchanged) and the baseserialize(str(x)). Nothing coerces a stored value tofloat, so anintinput survives as anint:Two logically-equal values (
5 == 5.0) therefore produce differenttask_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 aKeyErrorthat crashes the assistant (exactly the traceback in the issue).Verified on
master:(Within one process the instance cache masks it because
5 == 5.0; clearingRegister.clear_instance_cache()between instantiations — as the reporter does — exposes it.)Fix
Add
FloatParameter.normalizeto coerce the value tofloat(with aNoneguard soOptionalFloatParameterstill works).normalizeis applied to every parameter value, soT(5)andT(5.0)canonicalize to5.0→ the sametask_id. With the fix both yieldFoo_5_0_a9203c326b. This mirrors the reporter's ownserialize(float(x))workaround, applied at the right layer.Tests
test/parameter_test.py:test_float_normalizes_int_to_float—FloatParameter().normalize(5)is5.0(afloat), and equal int/float values serialize identically.test_float_task_id_stable_for_int_and_float— a task withargs=5vsargs=5.0(instance cache cleared between) yields the sametask_id.Both fail on
masterand pass with the fix.