claudevdm commented on code in PR #41184:
URL: https://github.com/apache/beam/pull/41184#discussion_r4543159975
##########
sdks/python/apache_beam/typehints/decorators.py:
##########
@@ -175,6 +175,25 @@ def _annotation_namespaces(fn):
return getattr(target, '__globals__', {}), localns
+def _is_convertible_type_hint(hint):
+ """Whether a resolved annotation converts to a valid Beam type hint.
+
+ A string annotation can resolve to something Beam cannot represent, e.g. a
+ protobuf enum (an ``EnumTypeWrapper`` instance, not a type) used as
+ ``Mapping[str, 'schema_pb2.FeatureType']``, or a typing construct that
+ convert_to_beam_type does not support, such as ``Generator[int]``. The
+ converted hint is validated (rather than the resolved annotation) so that
+ TypeVars, NewTypes and ``type`` aliases, which convert to valid hints, are
+ still resolved.
+ """
+ try:
Review Comment:
Should this case fall back to `Any` too? It still raises the original enum
error:
```python
import typing
import apache_beam as beam
from apache_beam.pvalue import PCollection
from google.protobuf import descriptor_pb2
class Types(beam.PTransform):
def expand(self, pcoll: 'PCollection[int]') -> (
'PCollection[typing.Mapping[str,
descriptor_pb2.FieldDescriptorProto.Type]]'):
return pcoll | beam.Map(lambda x: {})
print([1] | Types())
# TypeError: Value-type parameter to a Mapping hint must be a non-sequence,
a type, or a TypeConstraint
```
I think it's because `convert_to_beam_type` returns `PCollection[...]` and
`TaggedOutput[...]` unchanged without looking inside, so the check passes, and
Beam converts the inner type later. The previous commit caught it by checking
each type argument. Would it work to check the arguments again before
converting?
```suggestion
if hint is None or hint is Ellipsis:
return True
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.
for arg in args:
# A list is a Callable parameter list, e.g. the [int] in
# Callable[[int], str].
for inner in (arg if isinstance(arg, list) else [arg]):
if not _is_convertible_type_hint(inner):
return False
try:
```
##########
sdks/python/apache_beam/typehints/decorators.py:
##########
@@ -175,6 +175,25 @@ def _annotation_namespaces(fn):
return getattr(target, '__globals__', {}), localns
+def _is_convertible_type_hint(hint):
+ """Whether a resolved annotation converts to a valid Beam type hint.
+
+ A string annotation can resolve to something Beam cannot represent, e.g. a
+ protobuf enum (an ``EnumTypeWrapper`` instance, not a type) used as
+ ``Mapping[str, 'schema_pb2.FeatureType']``, or a typing construct that
+ convert_to_beam_type does not support, such as ``Generator[int]``. The
+ converted hint is validated (rather than the resolved annotation) so that
+ TypeVars, NewTypes and ``type`` aliases, which convert to valid hints, are
+ still resolved.
+ """
+ try:
+ validate_composite_type_param(
+ convert_to_beam_type(hint), 'Resolved type annotation')
+ except (TypeError, ValueError):
Review Comment:
Should a recursive `type` alias fall back to `Any` here as well? It gets
past this `except`:
```python
from __future__ import annotations
import apache_beam as beam
type JSON = dict[str, JSON] | list[JSON] | str | int | None
def parse(s: str) -> JSON:
return {}
print([''] | beam.Map(parse))
# RecursionError: maximum recursion depth exceeded
```
`convert_to_beam_type` unwraps the alias, finds the alias again inside, and
never stops, and `RecursionError` isn't a `TypeError` or `ValueError`. This ran
before #40625 and already fails on master. Is that something to handle in this
PR or separately?
--
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]
