[AIRFLOW-6759] Added MLEngine operator/hook to cancel MLEngine jobs #7400

MarkYHZhang · 2020-02-11T16:08:52Z

Added MLEngineTrainingJobFailureOperator with cancel_job hook for MLEngine.

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

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.
Read the Pull Request Guidelines for more information.

boring-cyborg · 2020-02-11T16:08:56Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://apache-airflow-slack.herokuapp.com/

airflow/providers/google/cloud/hooks/mlengine.py

joeyfreund · 2020-02-11T16:19:17Z

airflow/providers/google/cloud/hooks/mlengine.py

+        Cancels a MLEngine job.
+
+        :param project_id: The Google Cloud project id within which MLEngine
+            job will be launched. If set to None or missing, the default project_id from the GCP


This comment (describing the behaviour when project_id is None) is inconsistent with the code below.

Added types for `job_id` Co-Authored-By: Tomek Urbaszek <turbaszek@gmail.com>

joeyfreund · 2020-02-11T16:21:44Z

airflow/providers/google/cloud/operators/mlengine.py

+        training job. (templated)
+    :type job_id: str
+    :param project_id: The Google Cloud project name within which MLEngine training job should run.
+        If set to None or missing, the default project_id from the GCP connection is used. (templated)


Same as the comment above, it seems like the code raises an error if project_id is none/missing.

All operators except BigQuery allow passing project_id as a parameter. However, if this parameter is omitted, the default value will be read from the credentials.
https://github.com/apache/airflow/blob/97a429f/airflow/providers/google/cloud/hooks/base.py#L334-L360
It is very difficult to authorize and not get project_id. This is not even possible with production deployment. You may not have project_id when using gcloud for authorization only.

airflow/providers/google/cloud/hooks/mlengine.py

turbaszek · 2020-02-11T16:24:03Z

airflow/providers/google/cloud/operators/mlengine.py

+    template_fields = [
+        '_project_id',
+        '_job_id',
+    ]


Let use tuple here :)

That's an option, but I thought it'd be better to maintain consistency between the operators (all other ones uses an array). Unless, there is some other non-style related reasons?

It varies. But in my opinion, this is immutable field :)

turbaszek · 2020-02-11T16:24:44Z

airflow/providers/google/cloud/operators/mlengine.py

+        if not self._project_id:
+            raise AirflowException('Google Cloud project id is required.')
+        if not self._job_id:
+            raise AirflowException(


Suggested change

raise AirflowException(

No need for that as job_id is required parameter.

Uhmm, I'm not super sure why it was raised either, but I see the same checking in MLEngineStartBatchPredictionJobOperator and MLEngineStartTrainingJobOperator. And I thought I better include it as well 😅

MLEngineOperators are not best ones :D

cleaner formating Co-Authored-By: Tomek Urbaszek <turbaszek@gmail.com>

codecov-io · 2020-02-11T17:02:02Z

Codecov Report

Merging #7400 into master will decrease coverage by 0.13%.
The diff coverage is 87.09%.

@@            Coverage Diff             @@
##           master    #7400      +/-   ##
==========================================
- Coverage    86.5%   86.37%   -0.14%     
==========================================
  Files         873      878       +5     
  Lines       40725    41189     +464     
==========================================
+ Hits        35231    35576     +345     
- Misses       5494     5613     +119

Impacted Files	Coverage Δ
airflow/providers/google/cloud/hooks/mlengine.py	`82.25% <82.35%> (ø)`	⬆️
...rflow/providers/google/cloud/operators/mlengine.py	`89.66% <92.85%> (+0.15%)`	⬆️
airflow/kubernetes/volume_mount.py	`44.44% <0%> (-55.56%)`	⬇️
airflow/kubernetes/volume.py	`52.94% <0%> (-47.06%)`	⬇️
airflow/kubernetes/pod_launcher.py	`47.18% <0%> (-45.08%)`	⬇️
...viders/cncf/kubernetes/operators/kubernetes_pod.py	`69.38% <0%> (-24.23%)`	⬇️
airflow/kubernetes/refresh_config.py	`50.98% <0%> (-23.53%)`	⬇️
airflow/utils/sqlalchemy.py	`84.93% <0%> (-11.74%)`	⬇️
airflow/config_templates/airflow_local_settings.py	`65.38% <0%> (-6.36%)`	⬇️
airflow/stats.py	`85.29% <0%> (-5.19%)`	⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0fa964...e815aac. Read the comment docs.

boring-cyborg · 2020-02-17T09:55:20Z

Awesome work, congrats on your first merged pull request!

…pache#7400) * [AIRFLOW-6759] Added MLEngine operator/hook to cancel MLEngine jobs * Update airflow/providers/google/cloud/hooks/mlengine.py Added types for `job_id` Co-Authored-By: Tomek Urbaszek <turbaszek@gmail.com> * Updates cancel_job doc * Update airflow/providers/google/cloud/hooks/mlengine.py cleaner formating Co-Authored-By: Tomek Urbaszek <turbaszek@gmail.com> * removed redundant error checking Co-authored-by: Tomek Urbaszek <turbaszek@gmail.com>

[AIRFLOW-6759] Added MLEngine operator/hook to cancel MLEngine jobs

f5c5f5b

boring-cyborg bot added the provider:google Google (including GCP) related issues label Feb 11, 2020

turbaszek requested review from turbaszek and mik-laj and removed request for turbaszek February 11, 2020 16:11

turbaszek reviewed Feb 11, 2020

View reviewed changes

airflow/providers/google/cloud/hooks/mlengine.py Outdated Show resolved Hide resolved

joeyfreund reviewed Feb 11, 2020

View reviewed changes

Update airflow/providers/google/cloud/hooks/mlengine.py

582e45e

Added types for `job_id` Co-Authored-By: Tomek Urbaszek <turbaszek@gmail.com>

joeyfreund reviewed Feb 11, 2020

View reviewed changes

Updates cancel_job doc

b3f1cf7

turbaszek reviewed Feb 11, 2020

View reviewed changes

airflow/providers/google/cloud/hooks/mlengine.py Outdated Show resolved Hide resolved

turbaszek reviewed Feb 11, 2020

View reviewed changes

Update airflow/providers/google/cloud/hooks/mlengine.py

7e13a62

cleaner formating Co-Authored-By: Tomek Urbaszek <turbaszek@gmail.com>

turbaszek approved these changes Feb 14, 2020

View reviewed changes

Mark Zhang added 2 commits February 14, 2020 14:42

removed redundant error checking

6de75c4

Merge branch 'master' of https://github.com/MarkYHZhang/airflow

e815aac

turbaszek merged commit 2c9345a into apache:master Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-6759] Added MLEngine operator/hook to cancel MLEngine jobs #7400

[AIRFLOW-6759] Added MLEngine operator/hook to cancel MLEngine jobs #7400

MarkYHZhang commented Feb 11, 2020 •

edited by boring-cyborg bot

boring-cyborg bot commented Feb 11, 2020

joeyfreund Feb 11, 2020

joeyfreund Feb 11, 2020

mik-laj Feb 11, 2020

turbaszek Feb 11, 2020

MarkYHZhang Feb 11, 2020

turbaszek Feb 12, 2020

turbaszek Feb 11, 2020

MarkYHZhang Feb 11, 2020 •

edited

turbaszek Feb 12, 2020

codecov-io commented Feb 11, 2020 •

edited

boring-cyborg bot commented Feb 17, 2020

[AIRFLOW-6759] Added MLEngine operator/hook to cancel MLEngine jobs #7400

[AIRFLOW-6759] Added MLEngine operator/hook to cancel MLEngine jobs #7400

Conversation

MarkYHZhang commented Feb 11, 2020 • edited by boring-cyborg bot

boring-cyborg bot commented Feb 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkYHZhang Feb 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 11, 2020 • edited

Codecov Report

boring-cyborg bot commented Feb 17, 2020

MarkYHZhang commented Feb 11, 2020 •

edited by boring-cyborg bot

MarkYHZhang Feb 11, 2020 •

edited

codecov-io commented Feb 11, 2020 •

edited