feat(sdk): enable loading both JSON and YAML pipelines IR #1089

connor-mccarthy · 2022-03-17T16:36:29Z

Main changes
This PR switches load_json to load_yaml. This change is backward-compatible, as YAML is a superset of JSON.

Other changes

Added tests for load_yaml to demonstrate that is reads both JSON and YAML.
Added pyyaml as a library dependency.
Updated copyright where applicable.
Formatted tests/unit/aiplatform/test_utils.py with black.

Manually tested the four primary codepaths

Read JSON locally.
Read YAML locally.
Read JSON from Google Cloud Storage.
Read YAML from Google Cloud Storage.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

cc: @chensun @ji-yaqi

chensun

Consider adding some YAML snippets here along side the existing JSON one?

python-aiplatform/tests/unit/aiplatform/test_pipeline_jobs.py

Lines 67 to 104 in 52273c2

    
           _TEST_PIPELINE_SPEC_LEGACY = { 
        
               "pipelineInfo": {"name": "my-pipeline"}, 
        
               "root": { 
        
                   "dag": {"tasks": {}}, 
        
                   "inputDefinitions": {"parameters": {"string_param": {"type": "STRING"}}}, 
        
               }, 
        
               "schemaVersion": "2.0.0", 
        
               "components": {}, 
        
           } 
        
           _TEST_PIPELINE_SPEC = { 
        
               "pipelineInfo": {"name": "my-pipeline"}, 
        
               "root": { 
        
                   "dag": {"tasks": {}}, 
        
                   "inputDefinitions": { 
        
                       "parameters": { 
        
                           "string_param": {"parameterType": "STRING"}, 
        
                           "bool_param": {"parameterType": "BOOLEAN"}, 
        
                           "double_param": {"parameterType": "NUMBER_DOUBLE"}, 
        
                           "int_param": {"parameterType": "NUMBER_INTEGER"}, 
        
                           "list_int_param": {"parameterType": "LIST"}, 
        
                           "list_string_param": {"parameterType": "LIST"}, 
        
                           "struct_param": {"parameterType": "STRUCT"}, 
        
                       } 
        
                   }, 
        
               }, 
        
               "schemaVersion": "2.1.0", 
        
               "components": {}, 
        
           } 
        
           _TEST_TFX_PIPELINE_SPEC = { 
        
               "pipelineInfo": {"name": "my-pipeline"}, 
        
               "root": { 
        
                   "dag": {"tasks": {}}, 
        
                   "inputDefinitions": {"parameters": {"string_param": {"type": "STRING"}}}, 
        
               }, 
        
               "schemaVersion": "2.0.0", 
        
               "sdkVersion": "tfx-1.4.0", 
        
               "components": {}, 
        
           }

chensun · 2022-03-23T23:51:30Z

google/cloud/aiplatform/utils/yaml_utils.py

@@ -1,6 +1,6 @@
 # -*- coding: utf-8 -*-

-# Copyright 2020 Google LLC
+# Copyright 2022 Google LLC


nit: preserve the initial year, or leave it untouched.
https://opensource.google/documentation/reference/copyright#the_year

google/cloud/aiplatform/pipeline_jobs.py

sasha-gitg · 2022-03-24T20:58:47Z

google/cloud/aiplatform/utils/yaml_utils.py

 from typing import Any, Dict, Optional

+import yaml


Since this is an optional dependency is should be wrapped with an informative exception on import:

python-aiplatform/google/cloud/aiplatform/explain/lit.py

Lines 35 to 41 in 74ffa19

try:

import tensorflow as tf

except ImportError:

raise ImportError(

"Tensorflow is not installed and is required to load saved model. "

'Please install the SDK using "pip install google-cloud-aiplatform[lit]"'

)

Preference to import in the function that uses this package so it doesn't raise on module import.

sasha-gitg · 2022-03-24T21:53:51Z

tests/unit/aiplatform/test_utils.py

+class TestYamlUtils:
+    def test_load_yaml_from_local_file__with_json(self, yaml_file):
+        data = yaml_utils.load_yaml(yaml_file)
+        assert data["key"] == "val"


I think pytest supports dict equality with assert d1 == d2

sasha-gitg · 2022-03-29T18:07:31Z

tests/unit/aiplatform/test_pipeline_jobs.py

-        yield mock_load_json
+def mock_load_yaml_and_json(job_spec):
+    with patch.object(storage.Blob, "download_as_bytes") as mock_load_yaml_and_json:
+        job_spec_str = json.dumps(job_spec) if isinstance(job_spec, dict) else job_spec


It seems inconsistent to handle the json case as job_spec being deserialized and the yaml case as job_spec being serialized. Preference to add a comment describing this for future reference.

Definitely. That originates from the inconsistent starting point of the two representations in the tests: the JSON starts as a dict and the YAML starts as a string. In real usage, they would likely both be read in as strings from a file.

One option to make it more consistent is to put the json/dicts into strings as well, either literally with """ or programmatically (my preference) by wrapping in json.dumps at definition. If not one of those two options, I'll add a comment. WDYT?

json.dumps SGTM. Thanks!

connor-mccarthy · 2022-03-31T19:08:23Z

@sasha-gitg, can you please re-add the automerge label if all looks good?

gcf-merge-on-green · 2022-04-01T01:10:10Z

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

gcf-merge-on-green · 2022-04-01T22:58:10Z

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

gcf-merge-on-green · 2022-04-06T05:04:15Z

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

connor-mccarthy added 3 commits March 17, 2022 10:23

add pyyaml to dependencies

2af1550

read json and yaml using load_yaml

799ebd3

test yaml_utils

1a93032

connor-mccarthy requested a review from a team as a code owner March 17, 2022 16:36

connor-mccarthy marked this pull request as draft March 17, 2022 17:50

sasha-gitg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 18, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 18, 2022

connor-mccarthy marked this pull request as ready for review March 22, 2022 22:42

connor-mccarthy added 2 commits March 23, 2022 11:01

include pyyaml as extra_require

448aff9

Merge branch 'main' into enable-use-of-yaml-ir

9569623

chensun reviewed Mar 23, 2022

View reviewed changes

sasha-gitg reviewed Mar 24, 2022

View reviewed changes

google/cloud/aiplatform/pipeline_jobs.py Outdated Show resolved Hide resolved

connor-mccarthy added 2 commits March 24, 2022 09:17

update copyright

65a8fb6

add yaml test to PipelineJob test cases

78fb331

connor-mccarthy force-pushed the enable-use-of-yaml-ir branch from 11066e1 to 78fb331 Compare March 24, 2022 21:47

sasha-gitg requested changes Mar 24, 2022

View reviewed changes

connor-mccarthy added 5 commits March 24, 2022 15:48

wrap import in informative exception

0fddba2

update docstring

f9eecbd

use dict equality in tests

299edfb

Merge branch 'main' into enable-use-of-yaml-ir

3711b9e

Merge branch 'main' into enable-use-of-yaml-ir

42a1c09

connor-mccarthy requested review from chensun and sasha-gitg March 25, 2022 16:53

sasha-gitg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2022

Merge branch 'main' into enable-use-of-yaml-ir

bae839d

sasha-gitg approved these changes Mar 29, 2022

View reviewed changes

sasha-gitg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 29, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 29, 2022

use strings for all pipeline specs

7503eb8

Merge branch 'main' into enable-use-of-yaml-ir

31bab37

sasha-gitg added the automerge Merge the pull request once unit tests and other checks pass. label Mar 31, 2022

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 1, 2022

Merge branch 'main' into enable-use-of-yaml-ir

d106781

product-auto-label bot added the size: l Pull request size is large. label Apr 1, 2022

connor-mccarthy requested a review from sasha-gitg April 1, 2022 16:20

sasha-gitg added the automerge Merge the pull request once unit tests and other checks pass. label Apr 1, 2022

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 1, 2022

connor-mccarthy added 2 commits April 5, 2022 16:41

Merge branch 'main' into enable-use-of-yaml-ir

a34e3a4

clean up

3914dd0

connor-mccarthy force-pushed the enable-use-of-yaml-ir branch from ec57bd7 to 3914dd0 Compare April 5, 2022 16:43

connor-mccarthy requested a review from a team as a code owner April 5, 2022 16:43

product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Apr 5, 2022

Merge branch 'main' into enable-use-of-yaml-ir

1b90835

product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Apr 5, 2022

Merge branch 'main' into enable-use-of-yaml-ir

8124168

sasha-gitg added the automerge Merge the pull request once unit tests and other checks pass. label Apr 5, 2022

Merge branch 'main' into enable-use-of-yaml-ir

49f1c3a

sasha-gitg added automerge Merge the pull request once unit tests and other checks pass. and removed automerge Merge the pull request once unit tests and other checks pass. labels Apr 5, 2022

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 6, 2022

sasha-gitg merged commit f2e70b1 into googleapis:main Apr 6, 2022

release-please bot mentioned this pull request Apr 6, 2022

chore(main): release 1.12.0 #1059

Merged

release-please bot mentioned this pull request Jun 8, 2023

chore(main): release 1.24.1 #2196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk): enable loading both JSON and YAML pipelines IR #1089

feat(sdk): enable loading both JSON and YAML pipelines IR #1089

connor-mccarthy commented Mar 17, 2022 •

edited

Loading

chensun left a comment

chensun Mar 23, 2022

sasha-gitg Mar 24, 2022

sasha-gitg Mar 24, 2022

sasha-gitg Mar 29, 2022

connor-mccarthy Mar 29, 2022

sasha-gitg Mar 30, 2022

connor-mccarthy commented Mar 31, 2022

gcf-merge-on-green bot commented Apr 1, 2022

gcf-merge-on-green bot commented Apr 1, 2022

gcf-merge-on-green bot commented Apr 6, 2022

	_TEST_PIPELINE_SPEC_LEGACY = {
	"pipelineInfo": {"name": "my-pipeline"},
	"root": {
	"dag": {"tasks": {}},
	"inputDefinitions": {"parameters": {"string_param": {"type": "STRING"}}},
	},
	"schemaVersion": "2.0.0",
	"components": {},
	}
	_TEST_PIPELINE_SPEC = {
	"pipelineInfo": {"name": "my-pipeline"},
	"root": {
	"dag": {"tasks": {}},
	"inputDefinitions": {
	"parameters": {
	"string_param": {"parameterType": "STRING"},
	"bool_param": {"parameterType": "BOOLEAN"},
	"double_param": {"parameterType": "NUMBER_DOUBLE"},
	"int_param": {"parameterType": "NUMBER_INTEGER"},
	"list_int_param": {"parameterType": "LIST"},
	"list_string_param": {"parameterType": "LIST"},
	"struct_param": {"parameterType": "STRUCT"},
	}
	},
	},
	"schemaVersion": "2.1.0",
	"components": {},
	}
	_TEST_TFX_PIPELINE_SPEC = {
	"pipelineInfo": {"name": "my-pipeline"},
	"root": {
	"dag": {"tasks": {}},
	"inputDefinitions": {"parameters": {"string_param": {"type": "STRING"}}},
	},
	"schemaVersion": "2.0.0",
	"sdkVersion": "tfx-1.4.0",
	"components": {},
	}

	try:
	import tensorflow as tf
	except ImportError:
	raise ImportError(
	"Tensorflow is not installed and is required to load saved model. "
	'Please install the SDK using "pip install google-cloud-aiplatform[lit]"'
	)

feat(sdk): enable loading both JSON and YAML pipelines IR #1089

feat(sdk): enable loading both JSON and YAML pipelines IR #1089

Conversation

connor-mccarthy commented Mar 17, 2022 • edited Loading

chensun left a comment

Choose a reason for hiding this comment

chensun Mar 23, 2022

Choose a reason for hiding this comment

sasha-gitg Mar 24, 2022

Choose a reason for hiding this comment

sasha-gitg Mar 24, 2022

Choose a reason for hiding this comment

sasha-gitg Mar 29, 2022

Choose a reason for hiding this comment

connor-mccarthy Mar 29, 2022

Choose a reason for hiding this comment

sasha-gitg Mar 30, 2022

Choose a reason for hiding this comment

connor-mccarthy commented Mar 31, 2022

gcf-merge-on-green bot commented Apr 1, 2022

gcf-merge-on-green bot commented Apr 1, 2022

gcf-merge-on-green bot commented Apr 6, 2022

connor-mccarthy commented Mar 17, 2022 •

edited

Loading