Repository navigation
Conversation
Use typing.get_type_hints for annotated functions and class constructors so forward references are converted to their intended Beam type hints. Preserve the existing Any fallback when annotations cannot be resolved. Add regression coverage for string and future annotations, methods, constructors, and unresolved names.
There was a problem hiding this comment.
📋 Review Summary
This PR updates IOTypeHints.from_callable to resolve string annotations via typing.get_type_hints before converting them to Beam types, addressing issue #19954. However, relying on a single typing.get_type_hints call and gating strictly on top-level isinstance(..., str) introduces issues with partial resolution failures, nested forward references (e.g., typing.List['int']), and callable objects/functools.partial.
🔍 General Feedback
- When any single annotation fails to resolve in
typing.get_type_hints, the exception aborts resolution for the entire callable, causing all other valid string annotations on the function (including all annotations whenfrom __future__ import annotationsis active) to degrade toAny. - Forward references nested inside generic types (such as
typing.List['int']) are notstrinstances at the top level and are currently bypassed. - Callables supported by
inspect.signaturesuch asfunctools.partialand callable class instances (__call__) are not unwrapped before callingtyping.get_type_hints.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Match the formatter's expected layout for the return annotation and postponed-annotation regression test.
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40625 +/- ##
=============================================
+ Coverage 56.16% 79.93% +23.76%
=============================================
Files 1124 488 -636
Lines 178336 82612 -95724
Branches 1489 0 -1489
=============================================
- Hits 100170 66036 -34134
+ Misses 75645 16576 -59069
+ Partials 2521 0 -2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Assigning reviewers: R: @tvalentyn for label python. This pull request likely touches a core component ("core" label). Please review with scrutiny. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
R: @jrmccluskey who is an expert on this code -- thanks! |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
|
looks like some test failures may be relevant: @Gaurav598 can you take a look please? |
|
I compared The direct traceback is I pushed |
Fixes #19954.
What changed
IOTypeHints.from_callablepreviously passed string annotations toconvert_to_beam_type, which maps them toAny. Resolve string annotations withtyping.get_type_hintsbefore conversion, including annotations on class constructors. Keep the existingAnyfallback when a name cannot be resolved, and leave already evaluated annotations unchanged.Validation
intbecameAny.typehints.List[str]example, postponed annotations, methods, constructors, and unresolved names.IOTypeHintsTestmethods using the extracted source with dependency stubs on Python 3.12 and 3.14: all passed.py_compileand trailing whitespace checks passed.The normal Beam test import could not run in this environment: the source archive lacks generated protobuf modules, and
google.protobufis not installed. The dependency-light harness excludestest_origin_annotated, which requires the full Beam package.