[BEAM-7746] Minor typing updates / fixes

[chadrik]

This PR corrects a few problems that have occurred over the past few weeks:

• bad annotations added in PRs
• mangling of type ignore comments by yapf (fixed up the issues and added an option to the config to prevent it in the future)

Plus it adds some more type annotations. Nothing too radical or dangerous here.

---

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

• Choose reviewer(s) and mention them in a comment (R: @username).
• Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
• Update CHANGES.md with noteworthy changes.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go		---	---		---	---
Java
Python		---			---	---
XLang	---	---	---		---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

[chadrik]

R: @udim

[chadrik]

R: @robertwb

[robertwb]

Just some minor comments, and it looks like there's some lint to fix, but other than that looks good to go.

[robertwb]

Why this change? (And below.)

[chadrik]

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.

[robertwb]

Same.

[robertwb]

I prefer the previous format, where there was single assignment rather than re-assignment. Is there any benefit to typing this?

[chadrik]

I completely agree. Unfortunately, this is a syntax error in mypy:

try:
  from apache_beam.runners.dataflow.internal import apiclient  # type: Optional[types.ModuleType]
except ImportError:
  apiclient = None

As is this:

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:

try:
  Optional[types.ModuleType]: import apache_beam.runners.dataflow.internal.apiclient as apiclient 
except ImportError:
  apiclient = None

In that context, this makes more sense:

apiclient: Optional[types.ModuleType] = None
try:
  from apache_beam.runners.dataflow.internal import apiclient
except ImportError:
  pass

[robertwb]

My question is why we need to type apiclient as types.ModuleType at all.

[chadrik]

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?

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?

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.

[chadrik]

I did some more research on this, and I found this mypy issue: python/mypy#1297

It suggests this idiom:

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:

if some_conditional():
  x = None
else:
  x = 1
reveal_type(x)  # Revealed type is 'Union[builtins.int, None]'

[chadrik]

Down to 79 errors!

Btw, mypy may have revealed some legitimate errors in a recent change:

apache_beam/runners/worker/operations.py:837: error: "float" not callable  [operator]
apache_beam/runners/worker/operations.py:838: error: "float" not callable  [operator]
apache_beam/runners/worker/operations.py:839: error: "float" not callable  [operator]
apache_beam/runners/worker/operations.py:841: error: "float" not callable  [operator]
apache_beam/runners/worker/operations.py:842: error: "float" not callable  [operator]
apache_beam/runners/worker/operations.py:846: error: Module has no attribute "LATEST_DOUBLES_URN"; maybe "LATEST_DOUBLES_TYPE"?  [attr-defined]
apache_beam/runners/worker/operations.py:852: error: Module has no attribute "LATEST_DOUBLES_URN"; maybe "LATEST_DOUBLES_TYPE"?  [attr-defined]

[chadrik]

Run Python2_PVR_Flink PreCommit

[chadrik]

Run Python PreCommit

[chadrik]

@robertwb I don't think the test failures are my fault because they were passing before I rebased onto master...

[udim]

LGTM, tests already failing here:
https://builds.apache.org/job/beam_PreCommit_Python_Cron/2443/

[chadrik]

Run PythonLint PreCommit

[chadrik]

Do we think this is safe to merge? I've been watching master for something that looks like it could solve the current test problems, and rebasing periodically.

[chadrik]

@udim All tests passing now

[udim]

Thanks, Chad! merging

[angoenka]

Thanks @chadrik @udim

Just a quick reminder that we should squash the fixup related commits to keep the history clean based on the committer guide https://beam.apache.org/contribute/committer-guide/#finishing-touches
Its ok to keep commits separate if they are unrelated changes.

[udim]

I want to leave some commits and squash others. There is no way to do that in the GH UI.