Skip to content

Add type annotations for Google mlengine_operator_utils.py#10297

Merged
potiuk merged 1 commit intoapache:masterfrom
coopergillan:add-more-typing-coverage
Aug 16, 2020
Merged

Add type annotations for Google mlengine_operator_utils.py#10297
potiuk merged 1 commit intoapache:masterfrom
coopergillan:add-more-typing-coverage

Conversation

@coopergillan
Copy link
Contributor

Add type annotations to mlengine_operator_utils.py, including a few changes to ensure the right types are passed through. Specifically, if region is not given, it must be provided in the DAG's default_args or else a KeyError will now be raised.

I am trying to slowly chip away at the modules with little or no coverage per the command given in #9708. This one was at the top of the list =)

related: #9708


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added the provider:google Google (including GCP) related issues label Aug 12, 2020
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure why the diff showed up like this 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When default_args.get('region') was being used, mypy said that the type was a Union[str, Any, None].

This change also happens to have the fortunate side effect of now matching what was already in the docstring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest default_args.get("region", None)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think region is actually required in the underlying operator and is expecting a string. Passing None as the second argument here is actually already the default behavior, too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the region is necessary for MLEngineStartBatchPredictionJobOperator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turbaszek - if I'm reading the code correctly, that means that None won't work. Hence calling default_args['region'] directly.

Add type annotations, including a few changes to ensure the right types
are passed through. Specifically, if region is not given, it must be
provided in the DAG's default_args.
@coopergillan coopergillan force-pushed the add-more-typing-coverage branch from f648232 to c2628c6 Compare August 12, 2020 03:28
@turbaszek turbaszek requested a review from mik-laj August 16, 2020 05:31
@potiuk potiuk merged commit e195a98 into apache:master Aug 16, 2020
@coopergillan coopergillan deleted the add-more-typing-coverage branch August 16, 2020 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants