fix(backends): resolve postponed annotations in ollama tool schema - #1509
fix(backends): resolve postponed annotations in ollama tool schema#1509planetf1 wants to merge 3 commits into
Conversation
Match the module-top-level import style used in PR generative-computing#1509's test/backends/test_tools_pep563.py for the same fixture pattern. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…re (generative-computing#1504) * fix(stdlib): resolve postponed annotations in generative stub signature inspect.signature(func) returns literal annotation strings when the caller's module has `from __future__ import annotations` (PEP 563), corrupting both the rendered function signature and the argument value-quoting check in describe_function()/get_argument(). Pass eval_str=True so annotations resolve the same way typing.get_type_hints() already resolves them elsewhere in the codebase. Fixes generative-computing#1503 Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> * fix(stdlib): drop stale issue ref and guard PEP 563 test preconditions Regression test comments cited generative-computing#1476 (the original, unrelated report) instead of generative-computing#1503 (this fix); per project convention, drop the numeric ref rather than just correct it. Also pin each test's precondition via __annotations__ so it can't pass vacuously if the fixture module ever drops `from __future__ import annotations`. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> * test(stdlib): move PEP 563 fixture imports to module level Match the module-top-level import style used in PR generative-computing#1509's test/backends/test_tools_pep563.py for the same fixture pattern. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com> --------- Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
Fix is correct and I reproduced the failure and the fix locally. One structural change on how the test is split across files — details inline.
6da9180 to
2447632
Compare
convert_function_to_ollama_tool read raw inspect.signature() annotations, which are unresolved strings under `from __future__ import annotations` (PEP 563). Pydantic could not build a schema for any non-builtin parameter type, raising PydanticUserError. Resolve with eval_str=True, matching the fix already applied to genstub.py for the same root cause. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
2447632 to
172f0c1
Compare
…eturn annotation regression Assisted-by: opencode Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
|
Addressed: the test lives in |
…st module Addresses review feedback on generative-computing#1509. The PEP 563 regression tests were moved into test_discriminated_union_tools.py, already the only module exercising convert_function_to_ollama_tool, rather than living in a second module for the same function. The previous change only renamed the standalone file instead of moving the tests, which did not address the review. Also renames the samples module to _postponed_annotation_samples.py, dropping the PEP number from the filename as requested. The module-level 'from __future__ import annotations' still requires the sample code to sit in its own module; only the tests move. Verified the moved tests are not vacuous: against the pre-fix baseline 2 of 4 fail, and against parameter-blind eval_str=True resolution 3 of 4 fail. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
|
rewriting/fixing up after some local workspace confusion |
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
Both earlier threads resolved cleanly — the tests are now in test_discriminated_union_tools.py as TestPostponedAnnotations (fits the file's existing class style, no need to switch to module-level functions) and the sample module name drops the PEP number. Verified locally: all 27 tests in the file pass, ruff/mypy clean, and the fix matches #1507 — the per-parameter approach also correctly avoids the regression that plain eval_str=True would introduce on TYPE_CHECKING-only return annotations. One small non-blocking nit inline.
| assert props["to"]["title"] == Address.__name__ | ||
| assert props["to"]["properties"]["city"]["type"] == "string" | ||
|
|
||
| def test_type_checking_only_return_with_builtin_params(self): |
There was a problem hiding this comment.
Nit: test_resolves_postponed_parameter_annotation above guards its precondition (assert send_letter.__annotations__["to"] == "Address") so it can't silently stop exercising postponed annotations if _postponed_annotation_samples.py ever drops from __future__ import annotations. These three TC-only tests have no equivalent guard — they'd keep passing without testing the resolution path. Consider a matching assert (e.g. assert isinstance(tc_return_custom_param.__annotations__["period"], str)) or a one-liner on why it's unnecessary. Non-blocking.
2 participants
Pull Request
Issue
Fixes #1507
Description
convert_function_to_ollama_tool()built its dynamic Pydantic schema model from rawinspect.signature(func)parameter annotations. Underfrom __future__ import annotations(PEP 563), those annotations are strings rather than real type objects, so Pydantic could not resolve any non-builtin parameter type — raisingPydanticUserErrorat tool-conversion time. Builtin types (str,int, ...) happened to work by luck via Pydantic's builtin-name lookup.Fix: try
eval_str=Truefirst; on any exception (e.g. TYPE_CHECKING-only return type), fall back to per-parameter resolution viaeval(p.annotation, func.__globals__)for each string parameter annotation, leaving genuinely-unresolvable ones as strings for Pydantic to report. This is a strict behavioural superset ofmain— the fallback starts frominspect.signature(func)(identical tomain) and only ever replaces string annotations with successfully-resolved objects, so no regression is structurally possible on that path.Why not
eval_str=Truealone? The full-signature approach was blocked by four independent reviewers: it evaluates the return annotation too (which this schema never consumes), turning TYPE_CHECKING-only or forward-referenced return types into hardNameError/AttributeErrorwheremainsucceeded. 84 in-repo callables have this shape, includingpython_toolinmellea/stdlib/tools/interpreter.py. A naive try/except fallback (Fix A) is insufficient — it reintroducesPydanticUserErrorwhen TYPE_CHECKING return + custom parameter coexist.Cross-PR note:
genstub.py(unmerged branchissue-1476) applieseval_str=Truefor the identical root cause (#1503) and shares the same return-annotation regression. Coordinating the two PRs is a follow-up.Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing.