damccorm commented on code in PR #40757:
URL: https://github.com/apache/beam/pull/40757#discussion_r4381280614
##########
sdks/python/apache_beam/typehints/typehints.py:
##########
@@ -368,6 +368,16 @@ def is_typing_generic(type_param):
return isinstance(type_param, typing._GenericAlias)
+def _is_unchecked_protocol(type_param):
+ """Determines if an object is a typing.Protocol class that is not
+ runtime_checkable, which isinstance() and issubclass() reject.
+ """
+ return (
+ isinstance(type_param, type) and
+ getattr(type_param, '_is_protocol', False) and
+ not getattr(type_param, '_is_runtime_protocol', False))
Review Comment:
Worth considering while we're here: even with `@typing.runtime_checkable`,
Python raises `TypeError: Protocols with non-method members don't support
issubclass()` when a `Protocol` declares attributes or `@property` members
(e.g. `x: int`), which still crashes both `is_subhint(sub, base)` and
`issubclass(sub, base)` in `is_consistent_with`, even though `isinstance()` in
`check_constraint` works fine at runtime.
Keeping `_is_unchecked_protocol` for `check_constraint`, and using a helper
in `is_consistent_with` that checks whether `issubclass(object, base)` raises
`TypeError` when `getattr(base, '_is_protocol', False)` is true, would handle
both unchecked protocols and `@runtime_checkable` attribute/property protocols
cleanly while keeping runtime `isinstance` checks active for
`@runtime_checkable` protocols.
##########
sdks/python/apache_beam/typehints/typehints.py:
##########
@@ -1536,6 +1548,9 @@ def is_consistent_with(
# Cannot check unsupported parameterized generic which will cause
issubclass
# to fail with an exception.
return False
+ elif _is_unchecked_protocol(base):
+ # Protocols are matched structurally, which issubclass cannot check.
+ return True
Review Comment:
Because `elif _is_unchecked_protocol(base):` comes after `elif
isinstance(sub, RowTypeConstraint):` and `elif isinstance(sub,
TypeConstraint):`, container and `Row` element types (e.g. `p |
beam.Create([[1, 2]]) | beam.Map(fn)` where `fn` takes a `HasLen` protocol)
still return `False` and fail pipeline type checking. Could we move this check
right after `elif isinstance(base, TypeConstraint):` and add a test case with a
container input (e.g. `list[int]`)?
##########
sdks/python/apache_beam/typehints/typecheck_test.py:
##########
@@ -134,6 +135,19 @@ def test_wrapper_pipeline_type_check(self):
with self.assertRaisesRegex(ValueError, r'int.*is not iterable'):
_ = self.p | beam.Create([1, 2, 3]) | beam.ParDo(dofn)
+ def test_protocol_type_hint(self):
+ # https://github.com/apache/beam/issues/20986
+ class HasUpper(typing.Protocol):
+ def upper(self) -> str:
+ ...
+
+ def fn(e: HasUpper) -> str:
+ return e.upper()
+
+ result = self.p | beam.Create(['a', 'b']) | beam.Map(fn)
+ assert_that(result, equal_to(['A', 'B']))
+ self.p.run()
Review Comment:
Nit: Could we also add a corresponding test in
`PerformanceRuntimeTypeCheckTest` right below to make sure the
`--performance_runtime_type_check` path stays covered too?
--
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]
