[AIRFLOW-1272] Google Cloud ML Batch Prediction Operator by jiwang576 · Pull Request #2390 · apache/airflow

jiwang576 · 2017-06-22T21:13:48Z

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following AIRFLOW-1272 issues and references them in the PR title.

Description

Here are some details about my PR, including screenshots of any UI changes:
Added CloudMLBatchPredictionOperator, which is a wrapper around the Google Cloud ML Batch Prediction API.
Augmented the CloudMLHook in PR#2379, which handles submitting job request to Google Cloud ML Engine.

Tests

My PR adds the following unit tests:
tests.contrib.operators.test_cloudml_operator:CloudMLBatchPredictionOperatorTest
tests.contrib.hooks.test_gcp_cloudml_hook:TestCloudMLHook, which builds upon the same test class in PR#2379

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

There should be an Operator for the Google Cloud ML Batch Prediction Job, which supports executing Tensorflow neural network prediction as a service. Documentation is here: https://cloud.google.com/ml-engine/docs/how-tos/batch-predict.

mention-bot · 2017-06-22T21:13:52Z

@jiwang576, thanks for your PR! By analyzing the history of the files in this pull request, we identified @artwr, @jlowin and @criccomini to be potential reviewers.

codecov-io · 2017-06-22T21:43:34Z

Codecov Report

Merging #2390 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2390   +/-   ##
=======================================
  Coverage   69.27%   69.27%           
=======================================
  Files         146      146           
  Lines       11224    11224           
=======================================
  Hits         7775     7775           
  Misses       3449     3449

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 e414844...c65b8d9. Read the comment docs.

saguziel · 2017-06-26T20:49:58Z

airflow/contrib/hooks/gcp_cloudml_hook.py

+    """CloudML job operations helper class."""
+
+    def __init__(self, cloudml, project_name, job_id, job_spec=None):
+        assert project_name is not None and project_name is not ''


Should compared against '' with != since logically we are checking equality, not identity. I'm not even sure that '' is guaranteed to be interned.

Thank you! The fix was committed.

Changed precondition assertions inside the gcp_cloudml_hook.py.

criccomini · 2017-06-27T16:53:39Z

Please fix merge conflicts

criccomini · 2017-06-27T16:54:41Z

Please fix all comments that are:

"""like this

to be

"""
like this
"""

There should be an Operator for the Google Cloud ML Batch Prediction Job, which supports executing Tensorflow neural network prediction as a service. Documentation is here: https://cloud.google.com/ml-engine/docs/how-tos/batch-predict.

criccomini · 2017-06-27T16:57:11Z

airflow/utils/db.py

    merge_conn(
        models.Connection(
            conn_id='bigquery_default', conn_type='bigquery'))
+    merge_conn(


I think this was already added as part of:

https://github.com/apache/incubator-airflow/pull/2379/files#diff-1fd56b45d5b414bd50b37baaac692c40R133

Yes these are duplicate code when PR2379 was not merged into Airflow. I will get rid of those.
Thank you!

Still needs to get cleaned up. Same declaration is here:

https://github.com/apache/incubator-airflow/pull/2390/files#diff-1fd56b45d5b414bd50b37baaac692c40R134

criccomini · 2017-06-27T16:57:24Z

airflow/utils/db.py

            conn_id='presto_default', conn_type='presto',
            host='localhost',
            schema='hive', port=3400))
+    merge_conn(


Why two google_cloud_default entries?

criccomini · 2017-06-27T17:14:13Z

airflow/contrib/hooks/gcp_cloudml_hook.py

+
+    def create_job(self, project_name, job):
+        """
+        Creates a CloudML Job, and returns the Job object, which can be waited


"which can be waited upon"

Based on the return line below cloudml_job.wait_for_done(10), it looks to me like the waiting is done before create_job returns.

Thanks for the catch! We wrote a different docstring for that. Hopefully it reveals clearly what it does.

criccomini

This rebase looks kind of funky. It's showing a bunch of #2379. Is everything as intended?

criccomini · 2017-06-27T18:56:18Z

airflow/contrib/operators/cloudml_operator.py

+    """
+
+    template_fields = [
+        '_model',


Probably want project_name templated, too

Thanks Chris! the project_name is not intended for templating on purpose.

To answer the question of the funky rebase, I changed some of the code related to PR2379 for styles only. The main code changes are on the CloudMLBatchOperator (PR2390).

Got it, thanks for clarification.

criccomini

Overall, pretty good shape. Just a few nits/questions.

criccomini · 2017-06-27T19:49:41Z

airflow/contrib/operators/cloudml_operator.py

+                job_id)
+        except errors.HttpError as e:
+            if e.resp.status == 404:
+                logging.error(


Is this really an error? Seems like info is more appropriate

Done. Thanks for the suggestion!

criccomini · 2017-06-27T19:50:18Z

airflow/contrib/operators/cloudml_operator.py

+                logging.error(
+                    'Job with job_id {} does not exist. Will create it.'
+                    .format(job_id))
+                finished_prediction_job = hook.create_job(


Should wait_for_job_done be called here?

Sorry for the confusing interface. It's not needed as create_job is calling wait_for_job_done under the cover.
To make it clearer we refactored the hook interface and how the operator uses it. Now only create_job (which creates and wait until a job is finished) is "exposed" and used by the operator.

criccomini · 2017-06-27T19:51:22Z

airflow/utils/db.py

    merge_conn(
        models.Connection(
            conn_id='bigquery_default', conn_type='bigquery'))
+    merge_conn(


Still needs to get cleaned up. Same declaration is here:

https://github.com/apache/incubator-airflow/pull/2390/files#diff-1fd56b45d5b414bd50b37baaac692c40R134

This commit simplified the CloudMLHook class regarding job requests. Public interface of the hook now only exposes the create_job API, which contains the logic of creating the job, test for validation, and polling for the job until it's finished.

criccomini · 2017-06-28T22:24:44Z

cc @N3da I'm going to give you a day to look at this, since it's touching some of your stuff. I'll merge tomorrow if I don't hear from you.

N3da · 2017-06-28T22:39:57Z

@criccomini Thanks for the heads up. Ship it! :)

criccomini · 2017-06-28T22:58:52Z

@jiwang576 OK to merge this? Seems like you're still committing

jiwang576 · 2017-06-28T23:26:59Z

@criccomini Hi Chris, it's good to go. @N3da and I talked offline and I made some minor changes. Thanks for asking!

saguziel reviewed Jun 26, 2017

View reviewed changes

[AIRFLOW-1272] Fix precondition in cloudml hook

a9ed388

Changed precondition assertions inside the gcp_cloudml_hook.py.

Ji Wang added 5 commits June 27, 2017 17:15

[AIRFLOW-1272] Reorder helper functions

ac02f08

[AIRFLOW-1272] Merge conflicts with PR#2379

eb4d218

[AIRFLOW-1272] Merge remote-tracking branch

b9a26b2

[AIRFLOW-1272] Style-check merged files

3cbf050

criccomini reviewed Jun 27, 2017

View reviewed changes

Ji Wang added 5 commits June 27, 2017 21:23

[AIRFLOW-1272] Fix logging level and duplicated db setup

0f09ecc

[AIRFLOW-1272] Refactor hook code

7edd883

This commit simplified the CloudMLHook class regarding job requests. Public interface of the hook now only exposes the create_job API, which contains the logic of creating the job, test for validation, and polling for the job until it's finished.

[AIRFLOW-1272] Apply cosmetic restructuring and commenting to hook

d999ea6

[AIRFLOW-1272] Merge with upstream/master

9e01142

[AIRFLOW-1272] Remove trailing whitespace

01b76ed

[AIRFLOW-1272] Fix typo in create_version comments

c65b8d9

asfgit closed this in e92d6bf Jun 29, 2017

Conversation

jiwang576 commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Description

Tests

Commits

Uh oh!

mention-bot commented Jun 22, 2017

Uh oh!

codecov-io commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

criccomini commented Jun 27, 2017

Uh oh!

criccomini commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

criccomini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiwang576 Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

criccomini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

criccomini commented Jun 28, 2017

Uh oh!

N3da commented Jun 28, 2017

Uh oh!

criccomini commented Jun 28, 2017

Uh oh!

jiwang576 commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jiwang576 commented Jun 22, 2017 •

edited

Loading

codecov-io commented Jun 22, 2017 •

edited

Loading

criccomini commented Jun 27, 2017 •

edited

Loading

jiwang576 Jun 27, 2017 •

edited

Loading

jiwang576 commented Jun 28, 2017 •

edited

Loading