You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/12 03:23:09 UTC
[GitHub] [airflow] coopergillan opened a new pull request #10297: Add type annotations for Google mlengine_operator_utils.py
coopergillan opened a new pull request #10297:
URL: https://github.com/apache/airflow/pull/10297
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](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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
[GitHub] [airflow] coopergillan commented on a change in pull request #10297: Add type annotations for Google mlengine_operator_utils.py
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #10297:
URL: https://github.com/apache/airflow/pull/10297#discussion_r469283819
##########
File path: airflow/providers/google/cloud/utils/mlengine_operator_utils.py
##########
@@ -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']
Review comment:
@turbaszek - if I'm reading the code correctly, that means that `None` won't work. Hence calling `default_args['region']` directly.
----------------------------------------------------------------
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
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #10297: Add type annotations for Google mlengine_operator_utils.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #10297:
URL: https://github.com/apache/airflow/pull/10297#discussion_r469087617
##########
File path: airflow/providers/google/cloud/utils/mlengine_operator_utils.py
##########
@@ -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']
Review comment:
I would suggest `default_args.get("region", 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:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #10297: Add type annotations for Google mlengine_operator_utils.py
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #10297:
URL: https://github.com/apache/airflow/pull/10297
----------------------------------------------------------------
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
[GitHub] [airflow] coopergillan commented on a change in pull request #10297: Add type annotations for Google mlengine_operator_utils.py
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #10297:
URL: https://github.com/apache/airflow/pull/10297#discussion_r468983463
##########
File path: airflow/providers/google/cloud/utils/mlengine_operator_utils.py
##########
@@ -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']
Review 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](https://github.com/apache/airflow/blob/f618cdd19a5cdf09262436978817934d27668997/airflow/providers/google/cloud/utils/mlengine_operator_utils.py#L151).
----------------------------------------------------------------
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
[GitHub] [airflow] coopergillan commented on a change in pull request #10297: Add type annotations for Google mlengine_operator_utils.py
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #10297:
URL: https://github.com/apache/airflow/pull/10297#discussion_r468982332
##########
File path: airflow/providers/google/cloud/utils/mlengine_operator_utils.py
##########
@@ -24,31 +24,35 @@
import json
import os
import re
+from typing import Callable, Dict, Iterable, List, Optional, Tuple, TypeVar
from urllib.parse import urlsplit
import dill
+from airflow import DAG
from airflow.exceptions import AirflowException
from airflow.operators.python import PythonOperator
from airflow.providers.google.cloud.hooks.gcs import GCSHook
from airflow.providers.google.cloud.operators.dataflow import DataflowCreatePythonJobOperator
from airflow.providers.google.cloud.operators.mlengine import MLEngineStartBatchPredictionJobOperator
-
-def create_evaluate_ops(task_prefix, # pylint: disable=too-many-arguments
- data_format,
- input_paths,
- prediction_path,
- metric_fn_and_keys,
- validate_fn,
- batch_prediction_job_id=None,
- project_id=None,
- region=None,
- dataflow_options=None,
- model_uri=None,
- model_name=None,
- version_name=None,
- dag=None,
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
+
+
+def create_evaluate_ops(task_prefix: str, # pylint: disable=too-many-arguments
Review comment:
Not really sure why the diff showed up like this 🤷♂️
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on a change in pull request #10297: Add type annotations for Google mlengine_operator_utils.py
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10297:
URL: https://github.com/apache/airflow/pull/10297#discussion_r469151401
##########
File path: airflow/providers/google/cloud/utils/mlengine_operator_utils.py
##########
@@ -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']
Review comment:
Right, the `region` is necessary for `MLEngineStartBatchPredictionJobOperator`
----------------------------------------------------------------
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
[GitHub] [airflow] coopergillan commented on a change in pull request #10297: Add type annotations for Google mlengine_operator_utils.py
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #10297:
URL: https://github.com/apache/airflow/pull/10297#discussion_r469093883
##########
File path: airflow/providers/google/cloud/utils/mlengine_operator_utils.py
##########
@@ -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']
Review 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.
----------------------------------------------------------------
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