Skip to content

fix: make FuncFactory signature show NotSet defaults for required params - #88

Merged
thorwhalen merged 1 commit into
masterfrom
fix-48-funcfactory-notset-sentinel
Sep 22, 2026
Merged

thorwhalen merged 1 commit into
masterfrom
fix-48-funcfactory-notset-sentinel

Conversation

@thorwhalen

Copy link
Copy Markdown
Member

Summary

  • FuncFactory can be called with anywhere from none to all of the underlying function's arguments, but its signature previously showed the required-looking params (no default) as if they were mandatory to call the factory itself, which they aren't.
  • Activates the NotSet-sentinel approach that was already sketched out (commented-out) in i2/deco.py, and prototyped 3 years ago on the unmerged branch origin/48-using-a-notset-sentinel-to-solve-make-funcfactory-instances-signature-more-truthful-about-argument-requirements ("add Thor's patch" + follow-ups by @sylvainbonnot, Jan 2023).
  • Correction vs. that old branch: avoids re-indexing the signature through Sig.__getitem__ a second time (shown_factory_sig[self.include]), since that silently drops the return annotation -- a separate, pre-existing bug in Sig.__getitem__/from_params that is not addressed here (worth its own issue if relevant, related in spirit to Careful reordering when adding keyword-only params to a list that already has a variadic keyword. #17).
  • Updates the two affected FuncFactory doctests and the test_rm_params regression test (rm_params with allow_removal_of_non_defaulted_params doesn't exactly work #44) to reflect the new, more truthful signature display.

Closes #48

Dependents checked

Per fleet_dependents.json, on-box packages importing FuncFactory are front, ju, slang, streamlitfront. None introspect its Sig(...) string, so this is low-risk; full local test suite (761 passed, 2 xfailed) and black --check are green.

Test plan

  • pytest --doctest-modules i2/ -> 761 passed, 2 xfailed
  • black --check on changed files
  • Manual repro of the issue's FuncFactory example now shows (x=NotSet, y=NotSet, z=2)

🤖 Generated with Claude Code

https://claude.ai/code/session_011HSBVhDjRU4apSLcRkavv9

FuncFactory can be called with anywhere from none to all of the
underlying function's arguments, but its signature previously showed
the required-looking params (no default) as if they were mandatory to
call the factory itself -- which they aren't.

This activates the NotSet-sentinel approach that was already sketched
out (commented) in the code and prototyped on the unmerged branch
origin/48-using-a-notset-sentinel-to-solve-make-funcfactory-instances-signature-more-truthful-about-argument-requirements,
with one correction: avoid re-indexing the signature through
`Sig.__getitem__` a second time, since that silently drops the return
annotation (a separate, pre-existing bug in `Sig.__getitem__`/
`from_params`, not addressed here).

Updates the two affected FuncFactory doctests and the test_rm_params
regression test (#44) to reflect the new, more truthful
signature display.

Closes #48

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@thorwhalen
thorwhalen merged commit 70125c0 into master Sep 22, 2026
8 checks passed
@thorwhalen
thorwhalen deleted the fix-48-funcfactory-notset-sentinel branch September 22, 2026 13:20
@thorwhalen

Copy link
Copy Markdown
Member Author

Note (added after merge): this PR was self-reviewed only -- no independent sub-agent review, since it was merged before a crowsnest policy update requiring one for behaviour-changing merges. Flagging for transparency; the change itself (full local test suite: 761 passed, 2 xfailed; CI green) stands, but a second look is welcome.

thorwhalen added a commit that referenced this pull request Sep 22, 2026
…ired params (#88)" (#89)

This reverts commit 70125c0.

Post-merge adversarial review found that putting a NotSet sentinel default
on every FuncFactory param breaks signature-driven consumers in the fleet:

- front input elements: an int/float-annotated param crashes at element
  init (int(NotSet) -> TypeError); str params prefill the literal text
  "NotSet", which is then passed as a real value.
- py2http: the OpenAPI spec built for a FuncFactory is no longer
  JSON-serializable (Object of type Sentinel), and its params stop being
  listed as required.

Adds a regression test pinning the FuncFactory signature contract (the
wrapped function's defaults, no sentinel ones) until consumers are made
sentinel-aware. Re-landing plan is in #48.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
thorwhalen added a commit to i2mint/front that referenced this pull request Sep 22, 2026
* fix: treat i2's NotSet signature default as "required / no default"

Input elements, input specs, pydantic model specs and argument annotation
now read defaults through `front.util.param_default`, which maps i2's
`NotSet` sentinel to `Parameter.empty`. So a signature with NotSet
defaults (as a re-landed i2mint/i2#88 FuncFactory would show) no longer
crashes numeric inputs (`int(NotSet)`), prefills text inputs with
"NotSet", or becomes a pydantic default. No-op for current signatures.

Uses `i2.is_not_set` when available, else falls back on `i2.deco.NotSet`,
so the i2 floor is unchanged.

Refs i2mint/i2#48

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test: sharpen NotSet regression tests; import fallback sentinel eagerly (review follow-up)

The fixture no longer mutates _foo in place (Sig.__call__ sets __signature__
on its argument), so comparisons against the plain signature are meaningful;
6 of 8 tests now fail without the fix. The i2 fallback imports i2.deco.NotSet
at import time instead of on every call.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
thorwhalen added a commit to i2mint/streamlitfront that referenced this pull request Sep 22, 2026
…#13)

The legacy spec builders (`infer_type`, `get_func_args_specs`,
`page_funcs.*get_func_args_specs`, `get_func_elements_commands`) read
defaults through the new `util.signature_defaults(sig)`, which drops
params whose default is i2's `NotSet` sentinel. So a signature with NotSet
defaults (as a re-landed i2mint/i2#88 FuncFactory would show) gives the
same widgets as the plain one. No-op for current signatures. The
`mk_app` path goes through front, fixed separately (i2mint/front#34).

Uses `i2.is_not_set` when available, else falls back on `i2.deco.NotSet`.

Refs i2mint/i2#48

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
thorwhalen added a commit to i2mint/py2http that referenced this pull request Sep 22, 2026
* fix: treat i2's NotSet signature default as "required / no default"

`mk_input_schema_from_func` and `ParamsSpecifier.from_func` read defaults
through the new `schema_tools.param_default`, which maps i2's `NotSet`
sentinel to `Parameter.empty`. So a signature with NotSet defaults (as a
re-landed i2mint/i2#88 FuncFactory would show) keeps its params required
and yields a JSON-serializable OpenAPI spec, identical to the plain one.
No-op for current signatures.

Uses `i2.is_not_set` when available, else falls back on `i2.deco.NotSet`,
so the i2 requirement is unchanged.

Refs i2mint/i2#48

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test: pin the OpenAPI contract for a real FuncFactory showing NotSet (review follow-up)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix: make hosted CI green (stale http2py call, bottle 0.13 mount prefix)

- tests: http2py's mk_request_func_from_openapi_spec takes method as
  keyword-only since 2025-08; the positional call broke collection of
  simple_run_process_test.py (master CI red since then).
- mk_app: multi-API routes like 'foo_api' (as in its own doctest) are now
  mounted at '/foo_api'; bottle >= 0.13 and aiohttp reject prefixes
  without a leading slash, so the documented usage raised ValueError.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
thorwhalen added a commit to i2mint/dagapp that referenced this pull request Sep 22, 2026
`get_root_values` ignores NotSet defaults (falls back on the annotation's
default value or 0.0, as for a param with no default), and
`_compute_node_value` no longer skips a missing NotSet-defaulted param as
if it had a default. So a DAG built from signatures with NotSet defaults
(as a re-landed i2mint/i2#88 FuncFactory would show) never seeds a widget
with the sentinel. No-op for current signatures.

Uses `i2.is_not_set` when available, else falls back on `i2.deco.NotSet`.

Refs i2mint/i2#48

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
thorwhalen added a commit to i2mint/meshed that referenced this pull request Sep 22, 2026
`DAG.src_name_params` now maps a func node param's `NotSet` default (i2's
sentinel) to no default (`util.not_set_to_empty`) before merging and
sorting. Without this, once i2mint/i2#88 re-lands, a DAG holding a
FuncFactory node would silently reorder its positional params (NotSet
params sorted after required ones: `dag(1, 2)` binding to the wrong
roots), and merging with a same-named required param would raise.
No-op for current signatures.

Uses `i2.is_not_set` when available, else falls back on `i2.deco.NotSet`.

Refs i2mint/i2#48

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using a NotSet sentinel to solve make FuncFactory instances' signature more truthful about argument requirements.

1 participant