Use None, not inf, for "no limit" resample settings - #219
Merged
Conversation
float("inf") is not valid JSON, so any tool that serializes a Settings
object -- e.g. ezmsg-dashboard, which hit this in ezmsg-org/ezmsg-dashboard#4
-- fails on the defaults rather than on anything the user did.
Switch the two affected fields to an Optional sentinel, which serializes
cleanly and matches the convention already used by resample_rate,
target_rate, and friends:
ResampleSettings.max_chunk_delay inf -> None
ResampleSettings.reference_reset_after_chunks float -> int | None
reference_reset_after_chunks keeps its finite default of 3; only its
"disabled" spelling changes, since its docstring used to tell users to
pass float("inf") and walk straight into the bug.
Non-finite input is normalized to None rather than rejected, so existing
pipelines that pass inf keep working. The publisher's wait loop drops its
np.isfinite check for a plain is-not-None branch.
ResampleState.last_write_time keeps -inf: it is state, never serialized
into a settings payload.
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.
Why
float("inf")is not valid JSON. Any tool that serializes aSettingsobject trips over it — ezmsg-dashboard#4 hits this on our defaults, so it fails before the user has touched anything.A sweep of the repo turned up exactly one Settings field defaulting to infinity, declared twice:
resample.pyResampleSettings.max_chunk_delaynp.infresampleconcat.pyResampleConcatSettings.max_chunk_delaynp.inf(forwarded)Plus one near-miss:
reference_reset_after_chunksdefaults to a finite3, but its docstring told users to passfloat("inf")to disable reset recovery — straight into the same bug. Two tests did exactly that.What
max_chunk_delay:float = np.inf→float | None = None(no limit — never extrapolate on wall-clock alone).reference_reset_after_chunks:float = 3→int | None = 3. Default behaviour unchanged; only the "disabled" spelling moves frominftoNone.ResampleConcatSettings; its now-unusednumpyimport is dropped._as_limit()helper folds non-finite values ontoNone, so pipelines already passinginfkeep working rather than breaking on upgrade.np.isfinite(timeout)for a plainis not Nonebranch.Nonealso matches the convention already used across this repo for optional numerics (resample_rate,downsample.target_rate,align.gain,kaiser.ripple), and gives a dashboard an unambiguous "unset" to render.Considered and rejected: a large finite sentinel
float32max (~3.4e38) was the other candidate. These are float64 scalars in wall-clock-seconds arithmetic, so it's an arbitrary magic number that still renders as garbage in a UI field — and it would silently flip a code path, sending the publisher intoasyncio.wait_forwith an absurd deadline instead of the plain event wait.Not changed
ResampleState.last_write_timekeeps-np.inf. It's state, never serialized into a settings payload. The remaininginfhits in the repo are anp.nextafterdirection argument, a benchmark min-tracker, and two comments — all correct as-is.Testing
test_resample_reset_disabled_can_stallis parametrized over[None, float("inf")]so the back-compat path stays covered. The integration test and its docstring move toNone.ruff checkcleanjson.dumps(..., allow_nan=False), which previously raisedValueError: Out of range float values are not JSON compliantNote for the dashboard side
This fixes the defaults. A user can still type
infinto a float field at runtime, and this repo will accept it (normalizing internally) — so if ezmsg-dashboard serializes settings before handing them to the processor, it still needs its own guard.