Add type annotations for Google mlengine_operator_utils.py #10297
Conversation
boring-cyborg
bot
added
the
provider:google
| T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name | ||
|
|
||
|
|
||
| def create_evaluate_ops(task_prefix: str, # pylint: disable=too-many-arguments |
There was a problem hiding this comment.
Not really sure why the diff showed up like this 🤷♂️
| @@ -203,7 +210,7 @@ def validate_err_and_count(summary): | |||
| if dag is not None and dag.default_args is not None: | |||
| default_args = dag.default_args | |||
| project_id = project_id or default_args.get('project_id') | |||
| region = region or default_args.get('region') | |||
| region = region or default_args['region'] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I would suggest default_args.get("region", None)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right, the region is necessary for MLEngineStartBatchPredictionJobOperator
There was a problem hiding this comment.
@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
force-pushed
the
add-more-typing-coverage
branch
from
f648232 to
c2628c6
Compare
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'sdefault_argsor else aKeyErrorwill 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.