Set up script for Composer and support for getting gcsDagLocation from Composer environment #585

rajivpb · 2017-11-02T19:50:32Z

No description provided.

coveralls · 2017-11-02T20:20:03Z

Coverage remained the same at 77.771% when pulling b5eafa6 on rajivpb-composer-setup into 025b300 on master.

fenglu-db · 2017-11-02T20:20:55Z

google/datalab/contrib/pipeline/composer/setup.sh

+
+gcloud config set project $PROJECT
+gcloud auth login --activate $EMAIL
+sudo gcloud components repositories add https://storage.googleapis.com/composer-trusted-tester/components-2.json


First thing is to install composer specific gcloud and it may vary slightly depending on the environment (e.g., VM, shell, local) before any gcloud * operations.

fenglu-db · 2017-11-02T20:24:26Z

google/datalab/contrib/pipeline/composer/setup.sh

+sudo apt-get --only-upgrade install kubectl google-cloud-sdk google-cloud-sdk-datastore-emulator \
+google-cloud-sdk-pubsub-emulator google-cloud-sdk-app-engine-go google-cloud-sdk-app-engine-java \
+google-cloud-sdk-app-engine-python google-cloud-sdk-cbt google-cloud-sdk-bigtable-emulator \
+google-cloud-sdk-datalab


We can put apt-get first, and then composer gcloud installation, and finally composer related configs and environment creation.

…ameters

coveralls · 2017-11-03T21:02:53Z

Coverage remained the same at 77.825% when pulling 57111a7 on rajivpb-composer-setup into 8bd7431 on master.

coveralls · 2017-11-03T21:27:33Z

Coverage remained the same at 77.825% when pulling 57111a7 on rajivpb-composer-setup into 8bd7431 on master.

… script

coveralls · 2017-11-09T02:07:15Z

Coverage remained the same at 77.985% when pulling c422b10 on rajivpb-composer-setup into 2295d7b on master.

coveralls · 2017-11-09T02:22:26Z

Coverage remained the same at 77.985% when pulling c422b10 on rajivpb-composer-setup into 2295d7b on master.

qimingj · 2017-11-09T05:58:28Z

google/datalab/contrib/pipeline/airflow/setup.sh

+PROJECT=${1:-cloud-ml-dev}
+EMAIL=${2:[email protected]}
+ZONE=${3:-us-central1}
+ENVIRONMENT=${3:-rajivpb-airflow}


Should it be 4?

please fix.

Correct; fixed now.

I think you fixed the composer one but not airflow one yet?

Yeah, my bad - I kept thinking that all the comments referred to the composer's setup.sh and was a little confused on getting multiple comments for the same change. Will fix in a different PR mostly because I'm trying to get this in before demo time today.

…atalab into rajivpb-composer-setup

coveralls · 2017-11-10T04:27:45Z

Coverage increased (+0.06%) to 77.486% when pulling 959d4d5 on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T04:44:31Z

Coverage increased (+0.06%) to 77.486% when pulling dc02c7c on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T04:55:50Z

Coverage increased (+0.07%) to 77.499% when pulling dc02c7c on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T05:10:02Z

Coverage increased (+0.07%) to 77.499% when pulling dc02c7c on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T05:53:04Z

Coverage increased (+0.07%) to 77.499% when pulling e642b2f on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T09:21:14Z

Coverage increased (+0.06%) to 77.493% when pulling a44f131 on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T09:47:17Z

Coverage increased (+0.06%) to 77.489% when pulling 9774d10 on rajivpb-composer-setup into 7a472e9 on master.

chmeyers · 2017-11-10T17:37:07Z

google/datalab/contrib/pipeline/airflow/setup.sh

+PROJECT=${1:-cloud-ml-dev}
+EMAIL=${2:[email protected]}
+ZONE=${3:-us-central1}
+ENVIRONMENT=${3:-rajivpb-airflow}


please fix.

chmeyers · 2017-11-10T17:40:43Z

google/datalab/contrib/pipeline/composer/_composer.py

+  def gcs_dag_location(self):
+    if not self._gcs_dag_location:
+      environment_details = Api.environment_details_get(self._zone, self._environment)
+      if 'config' not in environment_details \


Per style guide, don't use \ for line continuation. Wrap the statement in parens instead.

chmeyers · 2017-11-10T17:44:08Z

google/datalab/contrib/pipeline/composer/_composer.py

+      # parts after the bucket. In those cases, the final file path needs to include those as well
+      additional_parts = ''
+      if len(gcs_dag_location_splits) > 4:
+        additional_parts = '/' + '/'.join(gcs_dag_location_splits[4:])


Instead of reconstructing this, just use the maxsplit argument to split so that the path section won't be split in the first place.

https://docs.python.org/2/library/stdtypes.html#str.split

Ah, cool. Done.

chmeyers · 2017-11-10T17:48:46Z

google/datalab/contrib/pipeline/composer/_composer.py

-    bucket = client.get_bucket(self.bucket_name)
-    filename = 'dags/{0}.py'.format(name)
+    try:
+      gcs_dag_location_splits = self.gcs_dag_location.split('/')


_ , _, bucket, filepath = self.gcs_dag_location.split('/', 3)

You should have gcs_dag_location() verify that what it returns is in the expected format with trailing slash, etc. And test whether this works with a top level directory in the bucket (i.e. gcs_dag_location = "gs://bucket/" )

That made my life easy, thanks!

chmeyers · 2017-11-10T17:49:15Z

google/datalab/contrib/pipeline/composer/setup.sh

+PROJECT=${1:-cloud-ml-dev}
+EMAIL=${2:[email protected]}
+ZONE=${3:-us-central1}
+ENVIRONMENT=${3:-datalab-testing-1}


4 instead of 3 here?

chmeyers · 2017-11-10T17:52:02Z

google/datalab/contrib/pipeline/composer/setup.sh

+
+gcloud config set project $PROJECT
+gcloud config set account $EMAIL
+gcloud auth login --activate $EMAIL


--activate is the default

Is this setup.sh only ever run manually? (Asking because users shouldn't gcloud auth login on a VM.)

Yes, setup.sh is only ever run manually on a local machine at the moment. It's not for the VM scenario.

(Removed --activate)

chmeyers · 2017-11-10T17:53:08Z

release.sh

@@ -16,6 +16,7 @@
 # Compiles the typescript sources to javascript and submits the files
 # to the pypi server specified as first parameter, defaults to testpypi
 # In order to run this script locally, make sure you have the following:
+# - A Python 3 environment (due to urllib issues)


Is there an issue open to fix this for Py2?

chmeyers · 2017-11-10T17:55:58Z

google/datalab/contrib/pipeline/composer/_composer.py

+              or 'gcsDagLocation' not in environment_details.get('config'):
+        raise ValueError('Dag location unavailable from Composer environment {0}'.format(
+          self._environment))
+      self._gcs_dag_location = environment_details['config']['gcsDagLocation']


Add a slash to the end of the location if it doesn't end with a slash?

coveralls · 2017-11-10T18:30:11Z

Coverage increased (+0.06%) to 77.489% when pulling bb69916 on rajivpb-composer-setup into 7a472e9 on master.

rajivpb

Thanks Chris and Bradley! Feedback addressed; PTAL.

rajivpb · 2017-11-10T18:05:39Z

release.sh

@@ -16,6 +16,7 @@
 # Compiles the typescript sources to javascript and submits the files
 # to the pypi server specified as first parameter, defaults to testpypi
 # In order to run this script locally, make sure you have the following:
+# - A Python 3 environment (due to urllib issues)


rajivpb · 2017-11-10T18:08:08Z

google/datalab/contrib/pipeline/composer/_composer.py

+  def gcs_dag_location(self):
+    if not self._gcs_dag_location:
+      environment_details = Api.environment_details_get(self._zone, self._environment)
+      if 'config' not in environment_details \


rajivpb · 2017-11-10T18:12:14Z

google/datalab/contrib/pipeline/composer/_composer.py

+      # parts after the bucket. In those cases, the final file path needs to include those as well
+      additional_parts = ''
+      if len(gcs_dag_location_splits) > 4:
+        additional_parts = '/' + '/'.join(gcs_dag_location_splits[4:])


Ah, cool. Done.

rajivpb · 2017-11-10T18:19:01Z

google/datalab/contrib/pipeline/composer/_composer.py

+              or 'gcsDagLocation' not in environment_details.get('config'):
+        raise ValueError('Dag location unavailable from Composer environment {0}'.format(
+          self._environment))
+      self._gcs_dag_location = environment_details['config']['gcsDagLocation']


rajivpb · 2017-11-10T18:19:23Z

google/datalab/contrib/pipeline/composer/_composer.py

-    bucket = client.get_bucket(self.bucket_name)
-    filename = 'dags/{0}.py'.format(name)
+    try:
+      gcs_dag_location_splits = self.gcs_dag_location.split('/')


That made my life easy, thanks!

chmeyers · 2017-11-10T18:56:17Z

google/datalab/contrib/pipeline/composer/_composer.py

+          'Dag location {0} from Composer environment {1} is in incorrect format'.format(
+            gcs_dag_location, self._environment))
+
+      self._gcs_dag_location = gcs_dag_location + '/'


What if it already ends with a slash?

Initially disallowed it via regex, but later decided to be more tolerant. Inserted check now.

coveralls · 2017-11-10T19:03:07Z

Coverage increased (+0.05%) to 77.483% when pulling 2934bd5 on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T19:15:55Z

Coverage increased (+0.05%) to 77.483% when pulling 2934bd5 on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T19:40:00Z

Coverage increased (+0.05%) to 77.483% when pulling 009a5e5 on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T19:53:13Z

Coverage increased (+0.06%) to 77.489% when pulling 009a5e5 on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T20:06:57Z

Coverage increased (+0.06%) to 77.489% when pulling 009a5e5 on rajivpb-composer-setup into 7a472e9 on master.

coveralls · 2017-11-10T20:21:39Z

Coverage increased (+0.06%) to 77.489% when pulling 009a5e5 on rajivpb-composer-setup into 7a472e9 on master.

qimingj · 2017-11-10T20:33:09Z

google/datalab/contrib/pipeline/airflow/setup.sh

+PROJECT=${1:-cloud-ml-dev}
+EMAIL=${2:[email protected]}
+ZONE=${3:-us-central1}
+ENVIRONMENT=${3:-rajivpb-airflow}


I think you fixed the composer one but not airflow one yet?

qimingj · 2017-11-10T20:37:14Z

google/datalab/contrib/bigquery/commands/_bigquery.py

@@ -21,6 +20,10 @@ def _create_pipeline_subparser(parser):
                                                  'transform data using BigQuery.')
  pipeline_parser.add_argument('-n', '--name', type=str, help='BigQuery pipeline name',
                               required=True)
+  pipeline_parser.add_argument('-e', '--environment', type=str,
+                               help='The name of the Composer or Airflow environment.')


It's only the name of the Composer right? Not used in Airflow?

You're right. At the of putting the comment, I made it slightly future looking since we plan to work on an Airflow environment. Will change in a different PR mostly because I'm trying to get this in before demo time today.

qimingj · 2017-11-10T20:39:51Z

google/datalab/contrib/bigquery/commands/_bigquery.py

@@ -44,11 +47,17 @@ def _pipeline_cell(args, cell_body):
    bq_pipeline_config = utils.commands.parse_config(
        cell_body, utils.commands.notebook_environment())
    pipeline_spec = _get_pipeline_spec_from_config(bq_pipeline_config)
+    # TODO(rajivpb): This import is a stop-gap for
+    # https://github.com/googledatalab/pydatalab/issues/593
+    import google.datalab.contrib.pipeline._pipeline
    pipeline = google.datalab.contrib.pipeline._pipeline.Pipeline(name, pipeline_spec)
    utils.commands.notebook_environment()[name] = pipeline

    # If a composer environment and zone are specified, we deploy to composer
    if 'environment' in args and 'zone' in args:


I think "'environment' in args and 'zone' in args" is always True. If user does not provide values for them, they will be None but still 'environment' and 'zone' exist in args.

Perhaps you want "if args['environment'] and args[ 'zone']:".

Oh, I didn't know that. Will verify and fix in a different PR mostly because I'm trying to get this in before demo time today.

Rajiv Bharadwaja added 2 commits November 2, 2017 12:07

Bash script for setting up Composer

ad956cc

Parameterizing more stuff

b5eafa6

fenglu-db reviewed Nov 2, 2017

View reviewed changes

Rajiv Bharadwaja added 2 commits November 3, 2017 13:39

Merge branch 'master' into rajivpb-composer-setup

0172ad8

Removing commented out commands and adding default values for the par…

57111a7

…ameters

Rajiv Bharadwaja and others added 2 commits November 8, 2017 17:43

Finishing up the Composer setup script and starting the airflow setup…

ac9104c

… script

Merge branch 'master' into rajivpb-composer-setup

c422b10

rajivpb requested a review from qimingj November 9, 2017 01:46

Re-organizing some of the comments

a14ae52

qimingj reviewed Nov 9, 2017

View reviewed changes

Support for fetching gcs dag location from Composer environment

94a3a75

rajivpb force-pushed the rajivpb-composer-setup branch from aadc6c6 to 94a3a75 Compare November 10, 2017 04:08

Merge branch 'master' into rajivpb-composer-setup

d5ad2f9

rajivpb changed the title ~~Bash script for setting up Composer~~ Set up Composer and get gcsDagLocation Nov 10, 2017

Rajiv Bharadwaja added 4 commits November 9, 2017 20:13

Add verification of bucket name

2a7f891

Merge branch 'rajivpb-composer-setup' of github.com:googledatalab/pyd…

4bf2a61

…atalab into rajivpb-composer-setup

Minor refactoring, and added comments

3646d44

Unit-tests for Composer.gcs_dag_location

959d4d5

Add stop-gap contrib imports to enable e2e

dc02c7c

Add comments to release script

e642b2f

rajivpb requested a review from chmeyers November 10, 2017 05:21

Remove timeoutMs from call to Composer API; this arg is invalid

a44f131

Fix flake8 errors

9774d10

chmeyers reviewed Nov 10, 2017

View reviewed changes

Correct wrong index for shell script arguments

bb69916

rajivpb force-pushed the rajivpb-composer-setup branch from 4a7c7ee to 58b9167 Compare November 10, 2017 18:47

rajivpb commented Nov 10, 2017

View reviewed changes

Refactor all gcs_dag_location checks into the property.

2934bd5

rajivpb force-pushed the rajivpb-composer-setup branch from 58b9167 to 2934bd5 Compare November 10, 2017 18:54

chmeyers reviewed Nov 10, 2017

View reviewed changes

Rajiv Bharadwaja added 2 commits November 10, 2017 11:32

Disallow trailing slash in gcs_dag_location from Composer

4d6162d

Allow trailing slash in gcs dag location

009a5e5

rajivpb force-pushed the rajivpb-composer-setup branch from 0917b1f to 009a5e5 Compare November 10, 2017 19:38

chmeyers approved these changes Nov 10, 2017

View reviewed changes

qimingj approved these changes Nov 10, 2017

View reviewed changes

rajivpb merged commit 31a9a64 into master Nov 10, 2017

rajivpb deleted the rajivpb-composer-setup branch November 10, 2017 21:04

rajivpb pushed a commit that referenced this pull request Nov 10, 2017

Address @qimingj's code-review feedback from #585

e902508

rajivpb mentioned this pull request Nov 10, 2017

Datalab should create a new Composer environment, and retrieve the dag-bucket name #565

Closed

rajivpb added a commit that referenced this pull request Nov 10, 2017

Address @qimingj's code-review feedback from #585 (#607)

018e3a2

chmeyers mentioned this pull request Jan 29, 2018

release.sh needs to work in python 2 environments as well #605

Open

Set up script for Composer and support for getting gcsDagLocation from Composer environment #585

Set up script for Composer and support for getting gcsDagLocation from Composer environment #585

Conversation

rajivpb commented Nov 2, 2017

coveralls commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 3, 2017

coveralls commented Nov 3, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

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

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 10, 2017

rajivpb left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

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