refactor: pipeline trigger code + cloud function #30

ghost · 2023-08-07T17:07:08Z

Description

Separated the pipeline trigger code (for make run) from the Cloud Function

Cloud Function now located in terraform/modules/cloudfunction/src
Pipeline trigger code now located in pipelines/src/pipelines/utils/trigger_pipeline.py
Changed PipelineJob display_name option to no longer use the template_path - template_path might be too long in the case of Artifact Registry URIs
Added unit test for upload_pipeline
pr-checks.yaml CI pipeline now runs unit tests for the util scripts
trigger-tests.yaml CI pipeline now runs unit tests only for the Cloud Function
Terraform code updated
Docs updated
Makefile updated
E2E tests updated to use new trigger_pipeline function
Added a workaround to Specifying artifact regististry images with tags fails googleapis/python-aiplatform#2181 in the Cloud Function - now resolves the AR tag before passing to PipelineJob

How has this been tested?

New and existing unit tests
Modified make commands have been tested locally
E2E tests as part of CI
Manually test cloud function deployment and operation
test release.yaml CI/CD

Checklist

I have commented my code, particularly in hard-to-understand areas
I have successfully run the E2E tests
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
I have updated any relevant documentation to reflect my changes

ghost · 2023-08-07T17:49:52Z

/gcbrun

ghost · 2023-08-07T18:09:22Z

/gcbrun

ghost · 2023-08-07T18:10:59Z

/gcbrun

ghost · 2023-08-08T16:26:30Z

/gcbrun

ghost · 2023-08-08T16:30:43Z

/gcbrun

ghost · 2023-08-09T08:43:57Z

/gcbrun

ghost · 2023-08-09T08:50:08Z

/gcbrun

ghost · 2023-08-09T09:07:33Z

/gcbrun

felix-datatonic · 2023-08-09T14:38:42Z

pipelines/src/pipelines/utils/trigger_pipeline.py

+    enable_caching = os.environ.get("enable_pipeline_caching", None)
+    if enable_caching:
+        enable_caching = bool(strtobool(enable_caching))


Suggested change

enable_caching = os.environ.get("enable_pipeline_caching", None)

if enable_caching:

enable_caching = bool(strtobool(enable_caching))

import os.environ as env

enable_caching = lower(env.get("enable_pipeline_caching")) in ["1", "true"]

also, I'd move this to ArgumentParser()

This logic is not correct. If the env variable is not set, the result should be None, not False

(equally lower(None) will cause an error)

how about something like:

enable_caching = os.environ.get("enable_pipeline_caching") if enable_caching is not None: enable_caching = lower(enable_caching) in ["1", "true"]

this will work and default to False if enable_pipeline_caching isn't in the env:

Suggested change

enable_caching = os.environ.get("enable_pipeline_caching", None)

if enable_caching:

enable_caching = bool(strtobool(enable_caching))

enable_caching = lower(env.get("enable_pipeline_caching", "")) in ["1", "true"]

It needs to default to None if not provided

pipelines/src/pipelines/utils/trigger_pipeline.py

felix-datatonic · 2023-08-09T14:42:42Z

pipelines/src/pipelines/utils/trigger_pipeline.py

+    pipeline_root = os.environ["VERTEX_PIPELINE_ROOT"]
+    service_account = os.environ["VERTEX_SA_EMAIL"]
+
+    enable_caching = os.environ.get("enable_pipeline_caching", None)


use upper case as we do for other env vars?

I don't mind - this is an env variable that we set when running the command (make run enable_pipeline_caching=true) rather than one picked up from env.sh

let's use upper case to standardise. also, you can change the code to:

Suggested change

enable_caching = os.environ.get("enable_pipeline_caching", None)

enable_caching = os.environ.get("enable_pipeline_caching")

pipelines/src/pipelines/utils/upload_pipeline.py

felix-datatonic · 2023-08-09T14:50:38Z

pipelines/tests/utils/test_trigger_pipeline.py

+        ("true", True),
+        ("False", False),
+        ("false", False),
+        (None, None),  # enable_pipeline_caching env var not set


add "0" and "1"

terraform/modules/scheduled_pipelines/scheduled_jobs.auto.tfvars.example

terraform/modules/cloudfunction/src/requirements-dev.txt

ghost · 2023-08-14T11:04:29Z

/gcbrun

ghost · 2023-08-14T11:04:44Z

Looks like we might be blocked by this issue

ghost · 2023-08-14T16:25:44Z

/gcbrun

felix-datatonic · 2023-08-15T08:18:08Z

pipelines/src/pipelines/utils/trigger_pipeline.py

+    pipeline_root = os.environ["VERTEX_PIPELINE_ROOT"]
+    service_account = os.environ["VERTEX_SA_EMAIL"]
+
+    enable_caching = os.environ.get("enable_pipeline_caching", None)


let's use upper case to standardise. also, you can change the code to:

Suggested change

enable_caching = os.environ.get("enable_pipeline_caching", None)

enable_caching = os.environ.get("enable_pipeline_caching")

felix-datatonic · 2023-08-15T08:19:30Z

pipelines/src/pipelines/utils/trigger_pipeline.py

+    enable_caching = os.environ.get("enable_pipeline_caching", None)
+    if enable_caching:
+        enable_caching = bool(strtobool(enable_caching))


this will work and default to False if enable_pipeline_caching isn't in the env:

Suggested change

enable_caching = os.environ.get("enable_pipeline_caching", None)

if enable_caching:

enable_caching = bool(strtobool(enable_caching))

enable_caching = lower(env.get("enable_pipeline_caching", "")) in ["1", "true"]

pipelines/tests/utils/test_upload_pipeline.py

felix-datatonic · 2023-08-15T08:22:11Z

terraform/modules/cloudfunction/src/main.py

+    enable_caching = payload.get("enable_pipeline_caching")
+    if enable_caching:
+        enable_caching = bool(strtobool(enable_caching))


Suggested change

enable_caching = payload.get("enable_pipeline_caching")

if enable_caching:

enable_caching = bool(strtobool(enable_caching))

enable_caching = lower(payload.get("enable_pipeline_caching", "")) in ["1", "true"]

felix-datatonic · 2023-08-15T08:22:31Z

terraform/modules/cloudfunction/src/main.py

+    encryption_spec_key_name = os.environ.get("VERTEX_CMEK_IDENTIFIER") or None
+    network = os.environ.get("VERTEX_NETWORK") or None


Suggested change

encryption_spec_key_name = os.environ.get("VERTEX_CMEK_IDENTIFIER") or None

network = os.environ.get("VERTEX_NETWORK") or None

encryption_spec_key_name = os.environ.get("VERTEX_CMEK_IDENTIFIER")

network = os.environ.get("VERTEX_NETWORK")

or None is needed in case the env variable is set to an empty string -> becomes None instead

terraform/modules/cloudfunction/src/main.py

terraform/modules/cloudfunction/src/requirements.txt

terraform/modules/cloudfunction/test/test_main.py

Co-authored-by: Felix Schaumann <[email protected]>

ghost · 2023-08-15T08:36:44Z

this will work and default to False if enable_pipeline_caching isn't in the env:

It needs to default to None if not provided

ghost · 2023-08-17T08:41:01Z

/gcbrun

ghost · 2023-08-17T09:02:59Z

/gcbrun

felix-datatonic

LGTM!

ghost force-pushed the decouple_trigger_code branch 3 times, most recently from 854b2ec to 7351898 Compare August 7, 2023 17:42

ghost force-pushed the decouple_trigger_code branch 2 times, most recently from b25aae5 to d84c7e7 Compare August 7, 2023 18:09

ghost force-pushed the decouple_trigger_code branch from d84c7e7 to eab027f Compare August 7, 2023 18:10

ghost force-pushed the decouple_trigger_code branch 7 times, most recently from bf1118e to 70dad7a Compare August 8, 2023 16:26

ghost force-pushed the decouple_trigger_code branch from 70dad7a to dcaca24 Compare August 8, 2023 16:30

ghost force-pushed the decouple_trigger_code branch from dcaca24 to e58afb1 Compare August 9, 2023 08:41

ghost force-pushed the decouple_trigger_code branch from e58afb1 to f11a92d Compare August 9, 2023 08:49

ghost force-pushed the decouple_trigger_code branch 2 times, most recently from e5d2766 to b973d85 Compare August 9, 2023 09:02

ghost marked this pull request as ready for review August 9, 2023 09:07

ghost force-pushed the decouple_trigger_code branch from b973d85 to 2fd9d61 Compare August 9, 2023 09:34

felix-datatonic reviewed Aug 9, 2023

View reviewed changes

refactor: pipeline trigger code + cloud function

0b226cd

Jonny Browning added 4 commits August 14, 2023 10:54

fix: terraform variables declaration

cce5a15

fix: fix cloud function YAML and bump google-cloud-aiplatform version

5a8a167

fix: terraform variables file

438c01e

fix: add more permissions to CF service account

2c8ce1e

Jonny Browning added 2 commits August 14, 2023 12:14

docs: fix example terraform config

72c7860

fix: add workaround for Vertex Pipelines bug

e61c539

felix-datatonic reviewed Aug 15, 2023

View reviewed changes

refactor: tidy regex group

0e37aba

Co-authored-by: Felix Schaumann <[email protected]>

Jonny Browning added 4 commits August 15, 2023 09:43

fix: bump google-cloud-aiplatform version in cloud function

59bd7e6

fix: update ENABLE_PIPELINE_CACHING env var

8b66dc0

refactor: remove redundant default value

4db1c2c

test: tidy test imports

ce82e95

Jonny Browning added 2 commits August 17, 2023 09:49

style: pre-commit fixes

638bc90

refactor: env var handling code

868b91c

felix-datatonic approved these changes Aug 18, 2023

View reviewed changes

ghost merged commit d16d566 into develop Aug 18, 2023

felix-datatonic deleted the decouple_trigger_code branch November 13, 2023 17:10

felix-datatonic mentioned this pull request Nov 15, 2023

Release 2.0 GoogleCloudPlatform/vertex-pipelines-end-to-end-samples#69

Open

6 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: pipeline trigger code + cloud function #30

refactor: pipeline trigger code + cloud function #30

ghost commented Aug 7, 2023 •

edited by ghost

Loading

ghost commented Aug 7, 2023

ghost commented Aug 7, 2023

ghost commented Aug 7, 2023

ghost commented Aug 8, 2023

ghost commented Aug 8, 2023

ghost commented Aug 9, 2023

ghost commented Aug 9, 2023

ghost commented Aug 9, 2023

felix-datatonic Aug 9, 2023

ghost Aug 9, 2023

ghost Aug 9, 2023

ghost Aug 9, 2023

felix-datatonic Aug 15, 2023

ghost Aug 15, 2023

felix-datatonic Aug 9, 2023

ghost Aug 9, 2023

felix-datatonic Aug 15, 2023

felix-datatonic Aug 9, 2023

ghost commented Aug 14, 2023

ghost commented Aug 14, 2023

ghost commented Aug 14, 2023

felix-datatonic Aug 15, 2023

felix-datatonic Aug 15, 2023

felix-datatonic Aug 15, 2023

felix-datatonic Aug 15, 2023

ghost Aug 15, 2023

ghost commented Aug 15, 2023

ghost commented Aug 17, 2023

ghost commented Aug 17, 2023

felix-datatonic left a comment

	enable_caching = os.environ.get("enable_pipeline_caching", None)
	enable_caching = os.environ.get("enable_pipeline_caching")

		encryption_spec_key_name = os.environ.get("VERTEX_CMEK_IDENTIFIER") or None
		network = os.environ.get("VERTEX_NETWORK") or None

refactor: pipeline trigger code + cloud function #30

refactor: pipeline trigger code + cloud function #30

Conversation

ghost commented Aug 7, 2023 • edited by ghost Loading

Description

How has this been tested?

Checklist

ghost commented Aug 7, 2023

ghost commented Aug 7, 2023

ghost commented Aug 7, 2023

ghost commented Aug 8, 2023

ghost commented Aug 8, 2023

ghost commented Aug 9, 2023

ghost commented Aug 9, 2023

ghost commented Aug 9, 2023

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

ghost commented Aug 14, 2023

ghost commented Aug 14, 2023

ghost commented Aug 14, 2023

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

ghost commented Aug 15, 2023

ghost commented Aug 17, 2023

ghost commented Aug 17, 2023

felix-datatonic left a comment

Choose a reason for hiding this comment

ghost commented Aug 7, 2023 •

edited by ghost

Loading