Fix test_ch_funcs fossil of the double_up_as_factory keyword bug - #76
Open
thorwhalen wants to merge 1 commit into
Open
Fix test_ch_funcs fossil of the double_up_as_factory keyword bug#76thorwhalen wants to merge 1 commit into
thorwhalen wants to merge 1 commit into
Conversation
`ch_funcs` is built with `i2.double_up_as_factory`, whose first parameter is `func_nodes`. Before i2mint/i2#82, passing the wrapped object by keyword landed it in `**kwargs`, so `ch_funcs(func_nodes=..., func_mapping=...)` silently returned a `functools.partial` factory instead of a `DAG`. `test_ch_funcs_no_change` was written against that broken behaviour: it added a trailing `()` to turn the unexpected factory into the DAG it wanted. Once i2 is fixed, `ch_funcs(func_nodes=...)` returns the DAG directly and that trailing `()` calls the DAG instead, raising `TypeError: missing a required argument: 'a'`. Changes: - `test_ch_funcs_no_change` now passes the func nodes positionally and reads `new_dag.func_nodes` directly. This is what the test always meant to assert, and it behaves identically on both the old and the new i2. - New `test_ch_funcs_takes_func_nodes_by_keyword` pins that `ch_funcs(func_nodes=...)` returns a `DAG` and not a `partial`, so the fossil cannot silently come back. It is feature-gated on the installed i2 rather than on a version number, so this commit is safe to land before i2#82 ships and starts enforcing itself the moment it does. - Added the module docstring. Claude-Session: https://claude.ai/code/session_01Kug7UUbVeCQgruvNXUq63c
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.
Land this BEFORE (or together with) i2mint/i2#82
i2mint/i2#82 fixes
double_up_as_factoryso that a decorator given its wrapped objectby keyword decorates instead of silently returning a
functools.partialfactory.meshed.dag.ch_funcsis built withdouble_up_as_factory, and one meshed test waswritten against the broken behaviour. That test goes pass -> fail the moment i2#82
lands. This PR removes the fossil, so it should land first (or at the same time).
It is written to be safe to merge now, before i2#82 ships — see "Ordering" below.
The fossil
ch_funcs's first parameter isfunc_nodes. Before i2#82:test_ch_funcs_no_changeused the keyword form and then added a trailing()to turnthe unexpected factory into the DAG it actually wanted:
After i2#82,
ch_funcs(func_nodes=...)returns the DAG directly, so that trailing()calls the DAG, which raises:
Verified A/B against a local i2 checkout:
meshed/tests/test_ch_funcs.pygives3 passedwith i2 master and1 failed, 2 passedwith the i2#82 branch, deterministically.The fix
test_ch_funcs_no_changenow passes the func nodes positionally and readsnew_dag.func_nodesdirectly. That is what the test always meant to assert("an identity mapping changes nothing"), and it behaves identically on old and new i2,
so this test keeps covering its intent regardless of which i2 is installed.
test_ch_funcs_takes_func_nodes_by_keywordpins thatch_funcs(func_nodes=...)returns a
DAGand not apartial, and that it agrees with the positional form —so the fossil cannot silently come back.
Ordering / why this is safe to merge before i2#82
The new pin asserts behaviour that only the fixed i2 has. Rather than pin an i2 version
that does not exist yet, it feature-detects (
_wrapped_by_keyword_is_supported()builds a throwaway
double_up_as_factorydecorator and checks it). So:The pin is not vacuous: with the guard temporarily removed and pre-#82 i2 installed it
fails with
AssertionError: ch_funcs wrongly returned a factory.Test results
meshed is already red on master — 3 pre-existing doctest failures unrelated to this
change (
meshed/dag.py,meshed/makers.py,meshed/scrap/cached_dag.py). Those areuntouched here. Full suite (
pytest --doctest-modules):Same 3 pre-existing failures in every case: the baseline is not worsened, and the
regression is gone.
Ecosystem survey
I searched the whole local package ecosystem for other call sites of this shape — passing
the wrapped object by keyword to a
double_up_as_factory-built decorator and then callingthe result — both statically (every such decorator in i2, meshed, front, larder, py2http,
plunk, wip_qh) and dynamically (a temporary probe in
double_up_as_factorylogging everycall that takes the new code path, run across all 46 i2 dependents' suites).
This test was the only one in executed code. A re-review of the survey turned up a second occurrence of the same fossil in
meshed/scrap/notebook.ipynb— a 2023 scratch notebook whose saved output shows the buggyfunctools.partialdirectly. That one is dead code (scrap/is in this repo's CIpaths-to-ignore, and no plugin collects notebooks), so it changes no test result; it is left untouched rather than edited, because rewriting its source without re-running it would desynchronise the cells from their stored outputs. Details are in the i2#82 thread.https://claude.ai/code/session_01Kug7UUbVeCQgruvNXUq63c