fix(app): reject checks with required params beyond the episode at registration - #91
Conversation
…gistration action_rate(episode, *, topics) registers silently and then fails once per episode at run time with TypeError, which App records as an infrastructure error and leaves the measurement column absent from the catalog. Inspect the function signature at registration: any required parameter beyond the single episode (required keyword-only included) now raises ValueError naming the parameter and showing the wrapper form. Closes Hebbian-Robotics#86
kstonekuan
left a comment
There was a problem hiding this comment.
Thank you @ayam04. Merging.
The message is the part I care most about, and it is right: it names the parameter, shows the wrapper form, and interpolates the real function name so the snippet is copy-pasteable rather than illustrative. That is the difference between an error that teaches and one that just refuses.
Handling required positional parameters beyond the episode as well as keyword-only was more than the issue asked for and it is the correct scope: both fail identically at run time, so both belong at registration.
What I validated on your branch against current main (f9ac687):
ruff check,ruff format --check,ty checkclean; 382 passed, 3 skipped. Nothing in the repo or the examples registered a check this now rejects.- Confirmed the message text by registering
action_ratebare, and confirmed optional keyword-only,*args/**kwargs, and episode-only checks all still register.
One hole left, and a follow-up if you want it: a zero-argument callable still registers and still fails per episode, with TypeError: takes 0 positional arguments but 1 was given. It is the mirror image of what you fixed and equally knowable from the signature. Out of scope for this PR, worth an issue.
Separately, and only as context rather than something to fix: functools.partial(action_rate, topics=[...]) is rejected too, but by the identity check, not yours, with cannot derive a stable implementation identity ... register it with version='...'. That is pre-existing and arguably correct, and it happens to be a second reason the wrapper form your message recommends is the right advice.
On the test_ffmpeg.py failures: environment, and they pass here. Third time you have checked against a clean main before reporting. Keep doing that.
Mirror of Hebbian-Robotics#91: a callable with no positional slot for the episode (def f(), def f(**kwargs), def f(*, episode)) registers cleanly and then fails once per episode with TypeError. _unsatisfiable_check_parameters now also reports whether the signature can receive the episode positionally (a plain parameter or *args); App.check() raises ValueError naming the check and stating the required signature. *args stays satisfiable since it absorbs the positional episode. Closes Hebbian-Robotics#93
#94) * fix(app): reject checks that cannot accept the episode at registration Mirror of #91: a callable with no positional slot for the episode (def f(), def f(**kwargs), def f(*, episode)) registers cleanly and then fails once per episode with TypeError. _unsatisfiable_check_parameters now also reports whether the signature can receive the episode positionally (a plain parameter or *args); App.check() raises ValueError naming the check and stating the required signature. *args stays satisfiable since it absorbs the positional episode. Closes #93 * keep a defaulted episode parameter satisfiable Maintainer fixup. The accepts-the-episode test skipped every defaulted positional parameter before claiming the episode's slot, so def check(episode=None) -> hflow.CheckResult was rejected at registration. It is called as check(canonical_episode), the default is never used, and it registered and ran fine on main, so this was a regression on a signature nothing had asked us to refuse. Claim the first positional slot for the episode whether or not it carries a default, and only then test later positional parameters for one. The rejected set is unchanged otherwise: def f(), def f(**kwargs), and def f(*, episode) still raise. Adds a test pinning registration and the runtime call convention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Kingston <kingston@hebbianrobotics.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#91 and #94 moved unsatisfiable signatures to registration time for app.check() and left the two structurally identical registration paths untouched. All three steps are invoked with exactly one positional Episode (app.py: checks and enrichments with the canonical episode, derived-signal functions with the source episode), so all three share the rule, and enrich()/derive() were still accepting a function the runtime could never call and failing once per episode instead. Extract the guard into _raise_if_step_cannot_take_only_an_episode and call it from all three, with the return annotation and wrapper name in the example varying per step kind. A derived channel is named by its topic, so the example's wrapper name is sanitized to an identifier. Also fix the wrapper example itself: it bound only the first missing parameter, so for a function needing two the snippet it told you to paste still raised TypeError on the second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #86
action_rate(episode, *, topics)registers viaapp.check()without complaint, then fails once per episode at run time withTypeError: missing 1 required keyword-only argument, which App records as an infrastructure error and leaves the measurement column absent from the catalog.Change: at registration,
App.check()now inspects the function signature and rejects any required parameter beyond the single canonical episode — including required keyword-only parameters (e.g.topics) and required positional parameters — with aValueErrornaming the unsatisfiable parameter(s) and showing the wrapper form:Optional parameters (with defaults),
*args/**kwargs, and episode-only checks are unaffected.Tests (both in tests/test_processing_regressions.py):
Validation:
test_ffmpeg.pyenv errors (no ffmpeg binaries in WSL; identical on clean main, confirmed previously with Kingston)