Specify task of training/tuning job in Airflow transform/deploy operator #590

yangaws · 2019-01-08T20:26:44Z

Issue #, if available:

Description of changes:

Specify task id and type of the training/tuning job in Airflow transform/endpoint operator. Then the API knows that which training job to transform/deploy if there are multiple jobs created in the DAG. To make this happen, additional changes are:

Make default name of jobs (training/tuning/transform/model/endpoint) random instead of global(all same). This also enables retry feature.
Remove the Airflow naming generation part in utils. Use the timestamp name postfix as before.

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

laurenyu · 2019-01-08T22:10:23Z

CHANGELOG.rst

@@ -5,6 +5,7 @@ CHANGELOG
 1.17.0.dev
 ==========

+* enhancement: Specify tasks from which training/tuning operator to transform/deploy in related operators


nit: and something to denote this is for Airflow/workflows

Updated! Also updated other entries of Airflow in CHANGELOG.

src/sagemaker/workflow/airflow.py

laurenyu · 2019-01-08T22:12:28Z

src/sagemaker/workflow/airflow.py

+    if estimator.uploaded_code is None:
+        return
+
+    postfix = os.path.join('/', 'source', 'sourcedir.tar.gz')


if we're actually concerned about using the correct separator for the OS, then you should use os.path.sep instead of '/'. however, if this is something where we actually just need slashes, then you can just use a format string here.

That line is removed since I used regex for the replace logic.

But it should be just '/' since it's in a s3 URI. Should be OS independent.

laurenyu · 2019-01-08T22:15:36Z

src/sagemaker/workflow/airflow.py

+    # s3://path/old_job/source/sourcedir.tar.gz will become s3://path/new_job/source/sourcedir.tar.gz
+    submit_uri = estimator.uploaded_code.s3_prefix
+    submit_uri = submit_uri[:len(submit_uri) - len(postfix)]
+    submit_uri = submit_uri[:submit_uri.rfind('/') + 1] + job_name + postfix


would this be potentially cleaner with a regex?

Good idea! Updated using regex

laurenyu · 2019-01-08T22:24:33Z

src/sagemaker/workflow/airflow.py

+        training_job = "{{ ti.xcom_pull(task_ids='%s')['Tuning']['BestTrainingJob']['TrainingJobName'] }}" % task_id
+        # need to strip the double quotes in json to get the string
+        job_name = "{{ ti.xcom_pull(task_ids='%s')['Tuning']['TrainingJobDefinition']['StaticHyperParameters']" \
+                   "['sagemaker_job_name'].replace('%s', '') }}" % (task_id, '"')


would it make more sense to use strip instead of replace?

Good idea! Updated

laurenyu · 2019-01-08T22:29:49Z

src/sagemaker/workflow/airflow.py

@@ -369,16 +421,22 @@ def model_config(instance_type, model, role=None, image=None):
    return config


-def model_config_from_estimator(instance_type, estimator, role=None, image=None, model_server_workers=None,
-                                vpc_config_override=vpc_utils.VPC_CONFIG_DEFAULT):
+def model_config_from_estimator(instance_type, estimator, task_id, task_type, role=None, image=None, name=None,


is this a breaking change?

yangaws

Address most of the comments. Need to rethink the attach one and will provide updates tomorrow.

yangaws · 2019-01-09T01:52:21Z

CHANGELOG.rst

@@ -5,6 +5,7 @@ CHANGELOG
 1.17.0.dev
 ==========

+* enhancement: Specify tasks from which training/tuning operator to transform/deploy in related operators


Updated! Also updated other entries of Airflow in CHANGELOG.

src/sagemaker/workflow/airflow.py

yangaws · 2019-01-09T01:55:23Z

src/sagemaker/workflow/airflow.py

+    if estimator.uploaded_code is None:
+        return
+
+    postfix = os.path.join('/', 'source', 'sourcedir.tar.gz')


That line is removed since I used regex for the replace logic.

But it should be just '/' since it's in a s3 URI. Should be OS independent.

yangaws · 2019-01-09T01:55:38Z

src/sagemaker/workflow/airflow.py

+    # s3://path/old_job/source/sourcedir.tar.gz will become s3://path/new_job/source/sourcedir.tar.gz
+    submit_uri = estimator.uploaded_code.s3_prefix
+    submit_uri = submit_uri[:len(submit_uri) - len(postfix)]
+    submit_uri = submit_uri[:submit_uri.rfind('/') + 1] + job_name + postfix


Good idea! Updated using regex

yangaws · 2019-01-09T01:55:46Z

src/sagemaker/workflow/airflow.py

+        training_job = "{{ ti.xcom_pull(task_ids='%s')['Tuning']['BestTrainingJob']['TrainingJobName'] }}" % task_id
+        # need to strip the double quotes in json to get the string
+        job_name = "{{ ti.xcom_pull(task_ids='%s')['Tuning']['TrainingJobDefinition']['StaticHyperParameters']" \
+                   "['sagemaker_job_name'].replace('%s', '') }}" % (task_id, '"')


Good idea! Updated

codecov-io · 2019-01-09T21:56:40Z

Codecov Report

Merging #590 into master will decrease coverage by 0.03%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
- Coverage   92.78%   92.75%   -0.04%     
==========================================
  Files          71       71              
  Lines        5367     5386      +19     
==========================================
+ Hits         4980     4996      +16     
- Misses        387      390       +3

Impacted Files	Coverage Δ
src/sagemaker/utils.py	`92.5% <ø> (-0.53%)`	⬇️
src/sagemaker/workflow/airflow.py	`91.82% <92.3%> (-0.4%)`	⬇️

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 9fd7e2b...862979d. Read the comment docs.

Specify task of training/tuning job in Airflow transform/deploy operator

bfa60b4

yangaws requested review from laurenyu and icywang86rui January 8, 2019 20:26

Merge branch 'master' into airflow_task

2d27a9f

laurenyu reviewed Jan 8, 2019

View reviewed changes

yangaws and others added 2 commits January 8, 2019 17:14

Merge branch 'master' into airflow_task

11fadaf

Address comments

7d3a09c

yangaws commented Jan 9, 2019

View reviewed changes

Merge branch 'master' into airflow_task

f06fc5f

Update changelog

862979d

laurenyu approved these changes Jan 9, 2019

View reviewed changes

yangaws merged commit 63b852b into aws:master Jan 10, 2019

Specify task of training/tuning job in Airflow transform/deploy operator #590

Specify task of training/tuning job in Airflow transform/deploy operator #590

Uh oh!

Conversation

yangaws commented Jan 8, 2019

Merge Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

yangaws left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

codecov-io commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

codecov-io commented Jan 9, 2019 •

edited

Loading