From 4e73ba0c6eb25ce5d415a18f57703b04e8ccd2e7 Mon Sep 17 00:00:00 2001 From: Chen Sun Date: Thu, 23 Feb 2023 19:48:30 +0000 Subject: [PATCH 1/3] Fix env set in component yaml not captured --- sdk/python/kfp/components/structures.py | 2 +- sdk/python/kfp/components/structures_test.py | 32 +++++++------------ .../test_data/pipelines/pipeline_with_env.py | 2 ++ .../pipelines/pipeline_with_env.yaml | 8 +++-- 4 files changed, 21 insertions(+), 23 deletions(-) diff --git a/sdk/python/kfp/components/structures.py b/sdk/python/kfp/components/structures.py index a969b62fad4..e44cdf9d121 100644 --- a/sdk/python/kfp/components/structures.py +++ b/sdk/python/kfp/components/structures.py @@ -343,7 +343,7 @@ def from_container_dict( image=container_dict['image'], command=container_dict.get('command'), args=container_dict.get('args'), - env=None, # can only be set on tasks + env=container_dict.get('env'), resources=None) # can only be set on tasks diff --git a/sdk/python/kfp/components/structures_test.py b/sdk/python/kfp/components/structures_test.py index 3d1766b5888..362af80a787 100644 --- a/sdk/python/kfp/components/structures_test.py +++ b/sdk/python/kfp/components/structures_test.py @@ -441,19 +441,12 @@ def test_from_container_dict_no_placeholders(self): implementation=structures.Implementation( container=structures.ContainerSpecImplementation( image='python:3.7', - command=[ - 'sh', '-c', - '\nif ! [ -x "$(command -v pip)" ]; then\n python3 -m ensurepip || python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1 python3 -m pip install --quiet --no-warn-script-location \'kfp==2.0.0-alpha.2\' && "$0" "$@"\n', - 'sh', '-ec', - 'program_path=$(mktemp -d)\nprintf "%s" "$0" > "$program_path/ephemeral_component.py"\npython3 -m kfp.components.executor_main --component_module_path "$program_path/ephemeral_component.py" "$@"\n', - '\nimport kfp\nfrom kfp import dsl\nfrom kfp.dsl import *\nfrom typing import *\n\ndef concat_message(first: str, second: str) -> str:\n return first + second\n\n' - ], + command=['sh', '-c', 'dummy'], args=[ - '--executor_input', - placeholders.ExecutorInputPlaceholder(), - '--function_to_execute', 'concat_message' + '--executor_input', '{{$}}', '--function_to_execute', + 'func' ], - env=None, + env={'ENV1': 'val1'}, resources=None), graph=None, importer=None), @@ -465,20 +458,19 @@ def test_from_container_dict_no_placeholders(self): outputs={'Output': structures.OutputSpec(type='String')}) container_dict = { 'args': [ - '--executor_input', '{{$}}', '--function_to_execute', 'fail_op' + '--executor_input', '{{$}}', '--function_to_execute', 'func' ], - 'command': [ - 'sh', '-c', - '\nif ! [ -x "$(command -v pip)" ]; then\n python3 -m ensurepip || python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1 python3 -m pip install --quiet --no-warn-script-location \'kfp==2.0.0-alpha.2\' && "$0" "$@"\n', - 'sh', '-ec', - 'program_path=$(mktemp -d)\nprintf "%s" "$0" > "$program_path/ephemeral_component.py"\npython3 -m kfp.components.executor_main --component_module_path "$program_path/ephemeral_component.py" "$@"\n', - '\nimport kfp\nfrom kfp import dsl\nfrom kfp.dsl import *\nfrom typing import *\n\ndef fail_op(message: str):\n """Fails."""\n import sys\n print(message)\n sys.exit(1)\n\n' - ], - 'image': 'python:3.7' + 'command': ['sh', '-c', 'dummy'], + 'image': 'python:3.7', + 'env': { + 'ENV1': 'val1' + }, } loaded_container_spec = structures.ContainerSpecImplementation.from_container_dict( container_dict) + self.assertEqual(component_spec.implementation.container, + loaded_container_spec) def test_raise_error_if_access_artifact_by_itself(self): diff --git a/sdk/python/test_data/pipelines/pipeline_with_env.py b/sdk/python/test_data/pipelines/pipeline_with_env.py index 9e7e807db61..98ebd6d0cbe 100644 --- a/sdk/python/test_data/pipelines/pipeline_with_env.py +++ b/sdk/python/test_data/pipelines/pipeline_with_env.py @@ -35,9 +35,11 @@ def print_env_op(): - -c - | set -e -x + echo "$ENV1" echo "$ENV2" echo "$ENV3" env: + ENV1: val0 ENV2: val0 """) diff --git a/sdk/python/test_data/pipelines/pipeline_with_env.yaml b/sdk/python/test_data/pipelines/pipeline_with_env.yaml index 87eea6b02ba..5c28f7444c5 100644 --- a/sdk/python/test_data/pipelines/pipeline_with_env.yaml +++ b/sdk/python/test_data/pipelines/pipeline_with_env.yaml @@ -15,12 +15,16 @@ deploymentSpec: - -c - 'set -e -x + echo "$ENV1" + echo "$ENV2" echo "$ENV3" ' env: + - name: ENV1 + value: val0 - name: ENV2 value: val2 - name: ENV3 @@ -38,7 +42,7 @@ deploymentSpec: - -c - "\nif ! [ -x \"$(command -v pip)\" ]; then\n python3 -m ensurepip ||\ \ python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1\ - \ python3 -m pip install --quiet --no-warn-script-location 'kfp==2.0.0-beta.8'\ + \ python3 -m pip install --quiet --no-warn-script-location 'kfp==2.0.0-beta.12'\ \ && \"$0\" \"$@\"\n" - sh - -ec @@ -76,4 +80,4 @@ root: taskInfo: name: print-env-op schemaVersion: 2.1.0 -sdkVersion: kfp-2.0.0-beta.8 +sdkVersion: kfp-2.0.0-beta.12 From 65755228200485d8883b6c50b3b44f2baff2d6f4 Mon Sep 17 00:00:00 2001 From: Chen Sun Date: Thu, 23 Feb 2023 21:34:46 +0000 Subject: [PATCH 2/3] release note --- sdk/RELEASE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/RELEASE.md b/sdk/RELEASE.md index a0e2cd0ec0d..5816e3b358d 100644 --- a/sdk/RELEASE.md +++ b/sdk/RELEASE.md @@ -12,6 +12,7 @@ ## Bug fixes and other changes * Enables output definitions when compiling components as pipelines. [\#8848](https://github.com/kubeflow/pipelines/pull/8848) * Fix bug when passing data between tasks using f-strings [\#8879](https://github.com/kubeflow/pipelines/pull/8879) +* Fix environment variable set in component yaml lost during compilation [\#8885](https://github.com/kubeflow/pipelines/pull/8885) ## Documentation updates From e684560ec8579cdb41c63c0b33d9e64700397bba Mon Sep 17 00:00:00 2001 From: Chen Sun Date: Thu, 23 Feb 2023 21:57:11 +0000 Subject: [PATCH 3/3] clean up unnecessary code in test --- sdk/python/kfp/components/structures_test.py | 30 ++++++-------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/sdk/python/kfp/components/structures_test.py b/sdk/python/kfp/components/structures_test.py index 362af80a787..c4bc9068d1b 100644 --- a/sdk/python/kfp/components/structures_test.py +++ b/sdk/python/kfp/components/structures_test.py @@ -436,26 +436,13 @@ def test_env(self): self.assertEqual(obj.env, None) def test_from_container_dict_no_placeholders(self): - component_spec = structures.ComponentSpec( - name='test', - implementation=structures.Implementation( - container=structures.ContainerSpecImplementation( - image='python:3.7', - command=['sh', '-c', 'dummy'], - args=[ - '--executor_input', '{{$}}', '--function_to_execute', - 'func' - ], - env={'ENV1': 'val1'}, - resources=None), - graph=None, - importer=None), - description=None, - inputs={ - 'first': structures.InputSpec(type='String', default=None), - 'second': structures.InputSpec(type='String', default=None) - }, - outputs={'Output': structures.OutputSpec(type='String')}) + expected_container_spec = structures.ContainerSpecImplementation( + image='python:3.7', + command=['sh', '-c', 'dummy'], + args=['--executor_input', '{{$}}', '--function_to_execute', 'func'], + env={'ENV1': 'val1'}, + resources=None) + container_dict = { 'args': [ '--executor_input', '{{$}}', '--function_to_execute', 'func' @@ -469,8 +456,7 @@ def test_from_container_dict_no_placeholders(self): loaded_container_spec = structures.ContainerSpecImplementation.from_container_dict( container_dict) - self.assertEqual(component_spec.implementation.container, - loaded_container_spec) + self.assertEqual(expected_container_spec, loaded_container_spec) def test_raise_error_if_access_artifact_by_itself(self):