claudevdm commented on code in PR #41184:
URL: https://github.com/apache/beam/pull/41184#discussion_r4539487289
##########
sdks/python/apache_beam/typehints/decorators.py:
##########
@@ -175,6 +175,35 @@ def _annotation_namespaces(fn):
return getattr(target, '__globals__', {}), localns
+def _is_valid_type_hint(hint):
+ """Whether a resolved annotation only contains valid type hint components.
+
+ A string annotation can resolve to an object that is not a type, e.g. a
+ protobuf enum (an ``EnumTypeWrapper`` instance) used as
+ ``Mapping[str, 'schema_pb2.FeatureType']``. Beam's composite type hints
+ reject such parameters, so these annotations must not be resolved.
+ """
+ if hint is None or hint is Ellipsis:
+ return True
+ if isinstance(hint, (list, tuple)):
+ # Callable parameter lists, e.g. the [int] in Callable[[int], str].
+ return all(_is_valid_type_hint(h) for h in hint)
+ origin = get_origin(hint)
+ if origin is Literal:
+ # Literal arguments are values, not types.
+ return True
+ args = get_args(hint)
+ if origin is typing.Annotated:
+ args = args[:1] # The remaining arguments are metadata.
+ if args:
+ return all(_is_valid_type_hint(arg) for arg in args)
+ try:
Review Comment:
`validate_composite_type_param` rejects TypeVars, NewTypes and `type`
aliases, so any string annotation using one now falls back to `Any`:
```python
from __future__ import annotations
import typing
import apache_beam as beam
T = typing.TypeVar('T')
def first(xs: list[T]) -> T:
return xs[0]
p = beam.Pipeline()
print((p | beam.Create([[1]]) | beam.Map(first)).element_type)
# master: <class 'int'> this PR: Any
```
Could we validate the converted hint instead? The enum case still falls
back, since a non-type comes back from `convert_to_beam_type` unchanged. A test
like the repro above would be good to add too.
```suggestion
try:
validate_composite_type_param(
convert_to_beam_type(hint), 'Resolved type annotation')
except (TypeError, ValueError):
return False
```
##########
sdks/python/apache_beam/typehints/decorators.py:
##########
@@ -186,12 +215,17 @@ def one_hint():
one_hint.__annotations__ = {'value': annotation}
try:
- return typing.get_type_hints(
+ resolved = typing.get_type_hints(
one_hint, globalns=globalns, localns=localns,
include_extras=True)['value']
except (AttributeError, NameError, SyntaxError, TypeError):
# convert_to_beam_type maps unresolved strings and ForwardRefs to Any.
return annotation
+ if not _is_valid_type_hint(resolved):
+ # The annotation names something that is not a type. Keep it unresolved
+ # so that convert_to_beam_type maps it to Any, as for unresolved names.
+ return annotation
+ return resolved
Review Comment:
Not caused by this PR: a resolved annotation that Beam can't convert raises
instead of falling back to `Any`. This ran before #40625:
```python
from __future__ import annotations
from collections.abc import Generator
import apache_beam as beam
def gen(x: int) -> Generator[int]:
yield x
print([1] | beam.FlatMap(gen))
# ValueError: expecting type collections.abc.Generator[int] to have arity 3,
had arity 1 instead
```
Would it be worth guarding the conversion here as well?
```suggestion
try:
convert_to_beam_type(resolved)
except (TypeError, ValueError):
return annotation
return resolved
```
##########
sdks/python/apache_beam/typehints/decorators.py:
##########
@@ -175,6 +175,35 @@ def _annotation_namespaces(fn):
return getattr(target, '__globals__', {}), localns
+def _is_valid_type_hint(hint):
+ """Whether a resolved annotation only contains valid type hint components.
+
+ A string annotation can resolve to an object that is not a type, e.g. a
+ protobuf enum (an ``EnumTypeWrapper`` instance) used as
+ ``Mapping[str, 'schema_pb2.FeatureType']``. Beam's composite type hints
+ reject such parameters, so these annotations must not be resolved.
+ """
+ if hint is None or hint is Ellipsis:
+ return True
+ if isinstance(hint, (list, tuple)):
+ # Callable parameter lists, e.g. the [int] in Callable[[int], str].
+ return all(_is_valid_type_hint(h) for h in hint)
+ origin = get_origin(hint)
+ if origin is Literal:
+ # Literal arguments are values, not types.
+ return True
+ args = get_args(hint)
+ if origin is typing.Annotated:
+ args = args[:1] # The remaining arguments are metadata.
+ if args:
+ return all(_is_valid_type_hint(arg) for arg in args)
+ try:
Review Comment:
`validate_composite_type_param` rejects TypeVars, NewTypes and `type`
aliases, so any string annotation using one now falls back to `Any`:
```python
from __future__ import annotations
import typing
import apache_beam as beam
T = typing.TypeVar('T')
def first(xs: list[T]) -> T:
return xs[0]
p = beam.Pipeline()
print((p | beam.Create([[1]]) | beam.Map(first)).element_type)
# master: <class 'int'> this PR: Any
```
Could we validate the converted hint instead? The enum case still falls
back, since a non-type comes back from `convert_to_beam_type` unchanged. A test
like the repro above would be good to add too.
```suggestion
try:
validate_composite_type_param(
convert_to_beam_type(hint), 'Resolved type annotation')
except (TypeError, ValueError):
return False
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
