[GitHub] [beam] robertwb commented on a change in pull request #13004: [BEAM-7746] Get mypy passing on transforms

[GitBox <gi...@apache.org>]

robertwb commented on a change in pull request #13004:
URL: https://github.com/apache/beam/pull/13004#discussion_r499788649



##########
File path: sdks/python/apache_beam/transforms/core.py
##########
@@ -2465,22 +2470,28 @@ def expand(self, pcoll):
     return pcoll | Map(lambda x: (self._key_func()(x), x)) | GroupByKey()
 
 
-_dynamic_named_tuple_cache = {}
+_dynamic_named_tuple_cache = {
+}  # type: typing.Dict[typing.Tuple[str, typing.Tuple[str, ...]], typing.Type[tuple]]
 
 
 def _dynamic_named_tuple(type_name, field_names):
+  # type: (str, typing.Tuple[str, ...]) -> typing.Type[tuple]
   cache_key = (type_name, field_names)
   result = _dynamic_named_tuple_cache.get(cache_key)
   if result is None:
     import collections
     result = _dynamic_named_tuple_cache[cache_key] = collections.namedtuple(
         type_name, field_names)
-    result.__reduce__ = lambda self: (
-        _unpickle_dynamic_named_tuple, (type_name, field_names, tuple(self)))
+    if not typing.TYPE_CHECKING:

Review comment:
       Is it possible to suppress the warning rather than take this branch at runtime-only (which feels odd)? 

##########
File path: sdks/python/apache_beam/transforms/core.py
##########
@@ -2394,8 +2399,8 @@ class GroupBy(PTransform):
   """
   def __init__(
       self,
-      *fields,  # type: typing.Union[str, callable]
-      **kwargs  # type: typing.Union[str, callable]
+      *fields,  # type: typing.Union[str, typing.Callable]

Review comment:
       Is there any advantage to (untyped) `typing.Callable`?

##########
File path: sdks/python/apache_beam/transforms/userstate.py
##########
@@ -305,23 +323,37 @@ def validate_stateful_dofn(dofn):
                        (timer_spec, method_name, dofn))
 
 
-class RuntimeTimer(object):
+class BaseTimer(object):

Review comment:
       This is good. 

##########
File path: sdks/python/apache_beam/transforms/stats.py
##########
@@ -426,14 +432,15 @@ def __next__(self):
     return QuantileBufferIterator(self.elements, self.weighted, self.weight)
 
 
-class _QuantileState(object):
+class _QuantileState(Generic[T]):
   """
   Compact summarization of a collection on which quantiles can be estimated.
   """
-  min_val = None  # Holds smallest item in the list
-  max_val = None  # Holds largest item in the list
+  min_val = None  # type: Any  # Holds smallest item in the list

Review comment:
       T?

##########
File path: sdks/python/apache_beam/transforms/environments.py
##########
@@ -52,7 +55,7 @@
 from apache_beam.utils import proto_utils
 
 if TYPE_CHECKING:
-  from apache_beam.options.pipeline_options import PortablePipelineOptions
+  from apache_beam.options.pipeline_options import PortableOptions

Review comment:
       It'll be good to actually start enforcing these type checks so they don't go out of date...

##########
File path: sdks/python/apache_beam/transforms/window.py
##########
@@ -164,14 +168,19 @@ def merge(self, merge_context):
     raise NotImplementedError
 
   def is_merging(self):
+    # type: () -> bool
+
     """Returns whether this WindowFn merges windows."""
     return True
 
   @abc.abstractmethod
   def get_window_coder(self):
+    # type: () -> coders.Coder

Review comment:
       This should be a `Coder[BoundedWindow]`.  We could even make WindowFn parameterized by the window type, and restrict this further.

##########
File path: sdks/python/apache_beam/transforms/combiners.py
##########
@@ -968,13 +968,13 @@ def expand(self, pcoll):
         return (
             pcoll
             | core.ParDo(self.add_timestamp).with_output_types(
-                Tuple[T, TimestampType])  # type: ignore[misc]

Review comment:
       Nice? Does it cause any other issues? 

##########
File path: sdks/python/apache_beam/transforms/util.py
##########
@@ -709,10 +710,10 @@ def expand(self, pcoll):
     return (
         pcoll
         | 'AddRandomKeys' >> Map(lambda t: (random.getrandbits(32), t)).
-        with_input_types(T).with_output_types(KeyedT)  # type: ignore[misc]
+        with_input_types(T).with_output_types(KeyedT)

Review comment:
       PythonFormatter needs another pass here.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org