[GitHub] [beam] chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes
chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes URL: https://github.com/apache/beam/pull/10822#discussion_r384836306 ## File path: sdks/python/apache_beam/transforms/external_java.py ## @@ -37,18 +39,19 @@ # Protect against environments where apitools library is not available. # pylint: disable=wrong-import-order, wrong-import-position +apiclient = None # type: Optional[types.ModuleType] Review comment: I did some more research on this, and I found this mypy issue: https://github.com/python/mypy/issues/1297 It suggests this idiom: ```python try: from apache_beam.runners.dataflow.internal import apiclient as _apiclient except ImportError: apiclient = None else: apiclient = _apiclient ``` The import is a bit longer and uglier, but it has 2 advantages: - no need to import `Optional` or `ModuleType` - the idiom I was using was actually making `apiclient` a generic ModuleType, dropping all knowledge of the members of `apache_beam.runners.dataflow.internal`. That's bad! The reason this works without explicit `Optional` annotation that mypy will automatically determine optionality in some cases, like this: ```python if some_conditional(): x = None else: x = 1 reveal_type(x) # Revealed type is 'Union[builtins.int, None]' ``` 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [beam] chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes
chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes URL: https://github.com/apache/beam/pull/10822#discussion_r384807421 ## File path: sdks/python/apache_beam/transforms/external_java.py ## @@ -37,18 +39,19 @@ # Protect against environments where apitools library is not available. # pylint: disable=wrong-import-order, wrong-import-position +apiclient = None # type: Optional[types.ModuleType] Review comment: > My question is why we need to type apiclient as types.ModuleType at all. Fair question. If we don't do this, we get the following error: ``` apache_beam/transforms/external_java.py:46: error: Incompatible types in assignment (expression has type "None", variable has type Module) [assignment] ``` To resolve this, we need to mark `apiclient` as `Optional` --- why can't we save ourselves some headache and leave out of the `types.ModuleType` part? ```python apiclient = None # type: Optional try: from apache_beam.runners.dataflow.internal import apiclient except ImportError: pass ``` If we do this, `apiclient` will become `Optional[Any]` --- Why can't we ignore the error? We can, but then mypy will mark the type as non-Optional, and that would remove the added protections that mypy provides against accidentally using the variable when it's `None`. --- Why can't we just add the type comment on the original `apiclient = None` line? ```python try: from apache_beam.runners.dataflow.internal import apiclient except ImportError: apiclient = None # type: Optional[types.ModuleType] ``` With this, we get the following error: ``` apache_beam/transforms/external_java.py:46: error: Name 'apiclient' already defined (by an import) [no-redef] ``` There is only one opportunity to override/influence the inferred type of a variable: on the first line where it is defined (think of type variable definitions like C/C++, but with python scoping rules). However, `apiclient` is defined via an import rather than an assignment, which forces us to preface the import with a variable definition. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [beam] chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes
chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes URL: https://github.com/apache/beam/pull/10822#discussion_r384807421 ## File path: sdks/python/apache_beam/transforms/external_java.py ## @@ -37,18 +39,19 @@ # Protect against environments where apitools library is not available. # pylint: disable=wrong-import-order, wrong-import-position +apiclient = None # type: Optional[types.ModuleType] Review comment: > My question is why we need to type apiclient as types.ModuleType at all. Fair question. If we don't do this (i.e. as with the original code), we get the following error: ``` apache_beam/transforms/external_java.py:46: error: Incompatible types in assignment (expression has type "None", variable has type Module) [assignment] ``` To resolve this, we need to mark `apiclient` as `Optional` --- why can't we save ourselves some headache and leave out of the `types.ModuleType` part? ```python apiclient = None # type: Optional try: from apache_beam.runners.dataflow.internal import apiclient except ImportError: pass ``` If we do this, `apiclient` will become `Optional[Any]` --- Why can't we ignore the error? We can, but then mypy will mark the type as non-Optional, and that would remove the added protections that mypy provides against accidentally using the variable when it's `None`. --- Why can't we just add the type comment on the original `apiclient = None` line? ```python try: from apache_beam.runners.dataflow.internal import apiclient except ImportError: apiclient = None # type: Optional[types.ModuleType] ``` With this, we get the following error: ``` apache_beam/transforms/external_java.py:46: error: Name 'apiclient' already defined (by an import) [no-redef] ``` There is only one opportunity to override/influence the inferred type of a variable: on the first line where it is defined (think of type variable definitions like C/C++, but with python scoping rules). However, `apiclient` is defined via an import rather than an assignment, which forces us to preface the import with a variable definition. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [beam] chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes
chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes URL: https://github.com/apache/beam/pull/10822#discussion_r384700835 ## File path: sdks/python/apache_beam/runners/common.py ## @@ -375,7 +379,7 @@ class DoFnInvoker(object): represented by a given DoFnSignature.""" def __init__(self, - output_processor, # type: OutputProcessor + output_processor, # type: _OutputProcessor Review comment: hmmm... well, at the time that I made this change I _think_ it resolved an error, but either I am mistaken or something changed in the module. I'm rolling this back. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [beam] chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes
chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes URL: https://github.com/apache/beam/pull/10822#discussion_r384689098 ## File path: sdks/python/apache_beam/transforms/external_java.py ## @@ -37,18 +39,19 @@ # Protect against environments where apitools library is not available. # pylint: disable=wrong-import-order, wrong-import-position +apiclient = None # type: Optional[types.ModuleType] Review comment: I completely agree. Unfortunately, this is a syntax error in mypy: ```python try: from apache_beam.runners.dataflow.internal import apiclient # type: Optional[types.ModuleType] except ImportError: apiclient = None ``` As is this: ```python try: import apache_beam.runners.dataflow.internal.apiclient as apiclient # type: Optional[types.ModuleType] except ImportError: apiclient = None ``` The reason is that type comments are (by design) not capable of doing anything that the new PEP 526 variable annotations are not. In other words, this is obviously wrong: ```python try: Optional[types.ModuleType]: import apache_beam.runners.dataflow.internal.apiclient as apiclient except ImportError: apiclient = None ``` In that context, this makes more sense: ```python apiclient: Optional[types.ModuleType] = None try: from apache_beam.runners.dataflow.internal import apiclient except ImportError: pass ``` 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [beam] chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing updates / fixes
chadrik commented on a change in pull request #10822: [BEAM-7746] Minor typing
updates / fixes
URL: https://github.com/apache/beam/pull/10822#discussion_r384678493
##
File path: sdks/python/apache_beam/io/iobase.py
##
@@ -1289,9 +1289,9 @@ class RestrictionProgress(object):
"""
def __init__(self, **kwargs):
# Only accept keyword arguments.
-self._fraction = kwargs.pop('fraction', None)
-self._completed = kwargs.pop('completed', None)
-self._remaining = kwargs.pop('remaining', None)
+self._fraction = kwargs.pop('fraction', None) # type: Optional[float]
+self._completed = kwargs.pop('completed', None) # type: Optional[int]
Review comment:
I was a pretty confused by this class, and I think there may actually be
some bugs in its handling of `None`-values, so I tried to do the bare minimum
of what I thought was right. No worries. I will revert this and leave this as
a final typing task to someone who understands it.
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:
us...@infra.apache.org
With regards,
Apache Git Services
