feat(steps): derive stable identity for functools.partial steps - #104
Conversation
promiseeuler
left a comment
There was a problem hiding this comment.
The new functools.partial branch does not forward allow_opaque to the recursive _callable_implementation_identity(function.func) call. This breaks the existing explicit-version escape hatch when a partial wraps an opaque callable.
I reproduced this on commit 29055cc with Python 3.14:
compute_check_version("opaque", len, False, frozenset(), None, "manual-v1")
# succeeds
compute_check_version(
"opaque-partial", functools.partial(len), False, frozenset(), None, "manual-v1"
)
# ValueError: cannot derive a stable implementation identity ... register it with versionCould the recursive call pass allow_opaque=allow_opaque and cover this with a regression test? That preserves the declared-version contract for partials as well as ordinary opaque callables.
A partial is fully inspectable (func, args, keywords), so the opaque-callable refusal was wrong: registered partials silently shared versions when their bound arguments changed, because the manual version='...' workaround does not track binding changes. Identity now composes the wrapped callable's identity with stable values of args and keywords, and behavior configuration carries the same bound values. inspect.signature already resolves partials, so the registration guard from Hebbian-Robotics#86/Hebbian-Robotics#93 accepts them. Fixes Hebbian-Robotics#98
Maintainer fixup on the tests only; the implementation is unchanged. test_step_version_differs_when_partial_bindings_change named its two versions "a" and "b". The step name is part of the identity, so the assertion held whether or not the bound arguments were read at all. Hold the name fixed so only the binding can move the version, and assert stability across reconstruction so the identity cannot be address-derived. Add the case the issue called out as most likely to be got wrong: a partial bound to a value the identity machinery cannot describe must keep raising. Transparency of the wrapper does not make its contents transparent, and without this the version would silently stop tracking that value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
29055cc to
d2dd9a0
Compare
kstonekuan
left a comment
There was a problem hiding this comment.
Thank you @ayam04. Merging. The implementation is exactly right and I changed none of it.
Two things you got right that I want to name, because they are the parts that make this safe rather than just working.
You handled _callable_behavior_configuration as well as the identity. The issue only asked for the identity function. Adding the partial branch there too is why rebinding actually moves the version instead of merely being recorded somewhere. It would have been easy to fix the visible error and leave the version stale.
You did not touch the opaque path. A partial is identifiable only because its bound values are, and the whole change would have been a regression if binding an un-describable object had started producing a version. I verified it still refuses:
functools.partial(scored_by_client, client=OpaqueClient())
ValueError: cannot derive a stable version identity for captured value ... register the step with version='...'
What I verified beyond the tests, all by hand:
| property | result |
|---|---|
| binding changes the version (name held fixed) | 90a15f2ee7da vs 931c442493f1 |
| stable across reconstruction | identical |
| keyword order irrelevant | identical |
nested partial(partial(f, ...)) |
resolves |
| opaque bound value | still refused |
The fixup, tests only. test_step_version_differs_when_partial_bindings_change computed its two versions under the names "a" and "b". The step name is part of the identity, so that assertion passed whether or not the bound arguments were read at all: I confirmed v("a", x) != v("b", x) for the same partial. Held the name fixed so only the binding can move it, added the reconstruction-stability assertion so the identity cannot be address-derived, and added the opaque-refusal case the issue called out as the one most likely to be got wrong.
Worth generalizing from: when a test asserts "X changes Y", the thing to check is that nothing else in the fixture could also have changed Y. Two of your three tests were airtight; that one had a second free variable.
Also rebased you onto current main, which had moved twice (5d5cf71, 0b6b5de), and resolved a conflict where my commit and yours both appended to the same test file. 405 passed, 3 skipped after the fixup, with ruff, ruff format --check, and ty check clean.
One heads-up on that main move: the signature guard from #91/#94 now also covers app.enrich() and app.derive(), which were structurally identical registration paths that had been left accepting functions the runtime could never call. Your partial work composes with it cleanly, since inspect.signature resolves partials, which you had already checked.
ty caught one thing in my own added test, worth knowing about: binding an opaque object to action_rate's topics parameter fails typechecking because topics is Sequence[str]. I used a locally defined function taking client: object instead. Your choice to test with the real action_rate was right for every other case.
Fixes #98
A
functools.partialwas refused as an unidentifiable opaque callable, pushing users to hand-writeversion='...'— which silently stops tracking bound-argument changes (two differently-parameterized checks share a version). A partial is fully inspectable:func,args,keywords.Change (src/hflow/steps.py):
_callable_implementation_identity: partial branch composing the wrapped callable's identity with stable values ofargsandkeywords(dict path already sorts keys)_callable_behavior_configuration: partial branch carrying the same bound values, so binding changes alter the versioninspect.signatureresolves partials (verified:([], True)+ registers)Verified:
functools.partial(action_rate, topics=[...])) now registers with a versiontopicschanges; stable across repeated callsValidation:
test_ffmpeg.pyenv failures (identical on clean main)