Skip to content

checks.py still hand-rolls five numeric guards that _field_guards now covers #498

Description

@kstonekuan

#487 landed the guard set in _field_guards (#494) and converted batching.py and the configuration half of build_ai_vlm_checks.py. checks.py is the largest file still hand-rolling it.

Measured on current main

38 hand-rolled isinstance(x, bool) sites remain across 16 files. Two groups are deliberately not targets:

  • catalog.py (6): coerces NumPy scalars, which _field_guards deliberately does not. See its module docstring. Leave it.
  • build_ai_vlm_checks.py (4): three parse external model responses and one is a genuine bool field. All four are correct as they are, and Centralize numeric range validation #494's review covers why.

That leaves checks.py as the biggest convertible one, with 5:

line parameter current shape
:991 shake_threshold_dps bool + np.isfinite + < 0
:998 unstable_min_duration_s bool + np.isfinite + < 0
:1005 horizontal_field_of_view_degrees bool + np.isfinite + not 0 < x <= 360
:1659 max_plausible_fps bool, then np.isfinite + <= 0
:1664 downsample_tolerance_fps bool, then np.isfinite + < 0

Four of the five map straight onto the new helpers:

require_non_negative_float(shake_threshold_dps, "shake_threshold_dps")
require_non_negative_float(unstable_min_duration_s, "unstable_min_duration_s")
require_positive_float(max_plausible_fps, "max_plausible_fps")
require_non_negative_float(downsample_tolerance_fps, "downsample_tolerance_fps")

horizontal_field_of_view_degrees is the odd one: it wants a half-open range, 0 < x <= 360. There is no helper for that. Add one only if a second caller wants the same shape; otherwise compose require_finite_float with the bound left in place, and say which you chose.

What this costs, which is the part to decide

Every message changes to _field_guards's spelling. "shake_threshold_dps must be finite and non-negative" becomes "shake_threshold_dps must be >= 0, got -1". The second names the offending value, which is better, but it is a user-visible string change across five parameters and it will move test assertions.

That was accepted for #487 and it is accepted here. Two things not to lose:

Definition of done

  1. rg -c "isinstance\([a-z_.]+, bool\)" src/hflow/checks.py reports at most 1 (the field-of-view case, if you leave it composed).
  2. Every refusal the file made before, it still makes: bool, non-finite, and out-of-range for all five parameters, each on a camera-less episode.
  3. Tests use match= with anchored patterns. On an episode with cameras a camera-processing failure also raises ValueError, so an unanchored pytest.raises(ValueError) passes whether or not the guard is there. fix: validate camera FPS thresholds #492 has the shape to copy.
  4. Mutation: neuter each guard in turn and confirm a test goes red.

Sequencing

Wait for #459 to merge. It is open against the guards in camera_frame_stats and camera_signal_quality in this same file and will add more of them. Converting underneath it would conflict.

Validation

uv sync --locked --all-extras
uv run ruff check
uv run ruff format --check
uv run ty check
uv run pytest -q

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requesthelp wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions