Skip to content

Commit 0ab85f9

Browse files
TomcliTomcli
authored
fix(sdk): fix nested param generation issue (kubeflow#1089)
* fix nested param generation issue * change logic to only apply to parent counter
1 parent a7f2b04 commit 0ab85f9

17 files changed

+348
-82
lines changed

‎sdk/python/kfp_tekton/compiler/_tekton_handler.py

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -419,8 +419,12 @@ def process_inline_cr_field(field_name):
419419
# global param level.
420420
for task in custom_task_crs:
421421
if task['metadata']['name'] in all_nested_loop:
422-
if task['spec'].get('iterateNumeric'):
423-
nested_loop_counter_params.append(task['spec'].get('iterateNumeric'))
422+
# add additional pipelinerun controller generated params for the compiler to ignore for upward passing
423+
iterate_param_list = ['iterateNumeric', 'iterateParam', 'iterateParamStringSeparator', 'IterationNumberParam']
424+
for iterate_name in iterate_param_list:
425+
if task['spec'].get(iterate_name):
426+
nested_loop_counter_params.append(task['spec'].get(iterate_name))
427+
424428
for task in tasks:
425429
if task['name'] == nested_custom_task['root_ct']:
426430
task['params'].extend(copy.deepcopy(nested_custom_task_special_params))
@@ -431,8 +435,7 @@ def process_inline_cr_field(field_name):
431435
task['params'] = sorted(task['params'], key=lambda k: k['name'])
432436
if task['name'] in all_nested_loop:
433437
for param in task['params']:
434-
if '$(params.' in param['value'] and 'subvar-' not in param['value'] and \
435-
param['name'] not in nested_loop_counter_params:
438+
if '$(params.' in param['value'] and 'subvar-' not in param['value']:
436439
global_task_values.add(param['value'])
437440
# Add any pipeline global params to the nested loop layers
438441
all_params = []
@@ -446,7 +449,7 @@ def process_inline_cr_field(field_name):
446449
'type': 'string'}
447450
)
448451
for task in tasks:
449-
if task['name'] == nested_custom_task['father_ct']:
452+
if task['name'] == nested_custom_task['father_ct'] and global_task_value not in global_task_values:
450453
task['params'].append(
451454
{'name': re.findall('\$\(params.([^ \t\n.:,;\{\}]+)\)', global_task_value)[0],
452455
'value': global_task_value}

‎sdk/python/tests/compiler/compiler_tests.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,13 @@ def test_nested_loop_counter_workflow(self):
225225
from .testdata.nested_loop_counter import loop_3_range
226226
self._test_pipeline_workflow(loop_3_range, 'nested_loop_counter.yaml')
227227

228+
def test_nested_loop_param_workflow(self):
229+
"""
230+
Test compiling nested loop param in workflow to verify parameters are generated correctly.
231+
"""
232+
from .testdata.nested_loop_param import loop_3_range
233+
self._test_pipeline_workflow(loop_3_range, 'nested_loop_param.yaml')
234+
228235
def test_loop_with_step_workflow(self):
229236
"""
230237
Test compiling a numeric loop with steps in workflow.

‎sdk/python/tests/compiler/testdata/loop_with_enumerate_withitem_multi_nested.yaml

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,8 @@ spec:
5050
params:
5151
- name: loop-item-param-2
5252
value: '[{"a": 1, "b": 2}, {"a": 10, "b": 20}]'
53-
- name: my_pipe_param
54-
value: $(params.my_pipe_param)
5553
- name: my_pipe_param-loop-item
5654
value: $(params.my_pipe_param)
57-
- name: my_pipe_param3
58-
value: $(params.my_pipe_param3)
5955
- name: my_pipe_param3-loop-item
6056
value: $(params.my_pipe_param3)
6157
taskSpec:
@@ -144,8 +140,6 @@ spec:
144140
value: $(params.loop-item-param-2-subvar-b)
145141
- name: my_pipe_param-loop-item
146142
value: $(params.my_pipe_param-loop-item)
147-
- name: my_pipe_param3
148-
value: $(params.my_pipe_param3)
149143
- name: my_pipe_param3-loop-item
150144
value: $(params.my_pipe_param3-loop-item)
151145
taskSpec:
@@ -205,8 +199,6 @@ spec:
205199
value: $(params.loop-item-param-2-subvar-b)
206200
- name: loop-item-param-7
207201
value: '[4, 5]'
208-
- name: my_pipe_param3
209-
value: $(params.my_pipe_param3)
210202
- name: my_pipe_param3-loop-item
211203
value: $(params.my_pipe_param3-loop-item)
212204
taskSpec:

‎sdk/python/tests/compiler/testdata/loop_with_enumerate_withitem_multi_nested_noninlined.yaml

Lines changed: 18 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -58,20 +58,20 @@ metadata:
5858
{"name": "loop-item-param-2-subvar-a", "value": "$(params.loop-item-param-2-subvar-a)"},
5959
{"name": "loop-item-param-2-subvar-b", "value": "$(params.loop-item-param-2-subvar-b)"},
6060
{"name": "my_pipe_param-loop-item", "value": "$(params.my_pipe_param-loop-item)"},
61-
{"name": "my_pipe_param3", "value": "$(params.my_pipe_param3)"}, {"name": "my_pipe_param3-loop-item",
62-
"value": "$(params.my_pipe_param3-loop-item)"}], "taskRef": {"apiVersion": "custom.tekton.dev/v1alpha1",
63-
"kind": "PipelineLoop", "name": "withitem-multiple-nesting-pipeline-for-loop-5"}}]}}},
64-
{"apiVersion": "custom.tekton.dev/v1alpha1", "kind": "PipelineLoop", "metadata":
65-
{"name": "withitem-multiple-nesting-pipeline-for-loop-5"}, "spec": {"iterateParam":
66-
"my_pipe_param-loop-item", "pipelineSpec": {"params": [{"name": "iteration-number-4",
67-
"type": "string"}, {"name": "loop-item-param-2-subvar-a", "type": "string"},
68-
{"name": "loop-item-param-2-subvar-b", "type": "string"}, {"name": "my_pipe_param-loop-item",
69-
"type": "string"}, {"name": "my_pipe_param3", "type": "string"}, {"name": "my_pipe_param3-loop-item",
70-
"type": "string"}], "tasks": [{"name": "my-1st-inner-coop", "params": [{"name":
71-
"loop-item-param-2-subvar-a", "value": "$(params.loop-item-param-2-subvar-a)"},
72-
{"name": "my_pipe_param-loop-item", "value": "$(params.my_pipe_param-loop-item)"}],
73-
"taskSpec": {"metadata": {"annotations": {"pipelines.kubeflow.org/component_spec_digest":
74-
"{\"name\": \"my-1st-inner-coop\", \"outputs\": [], \"version\": \"my-1st-inner-coop@sha256=feaa8a218c3caaa76fa1154bcca9fc485286e72814986c35d08edcd28d2a50b9\"}"},
61+
{"name": "my_pipe_param3-loop-item", "value": "$(params.my_pipe_param3-loop-item)"}],
62+
"taskRef": {"apiVersion": "custom.tekton.dev/v1alpha1", "kind": "PipelineLoop",
63+
"name": "withitem-multiple-nesting-pipeline-for-loop-5"}}]}}}, {"apiVersion":
64+
"custom.tekton.dev/v1alpha1", "kind": "PipelineLoop", "metadata": {"name": "withitem-multiple-nesting-pipeline-for-loop-5"},
65+
"spec": {"iterateParam": "my_pipe_param-loop-item", "pipelineSpec": {"params":
66+
[{"name": "iteration-number-4", "type": "string"}, {"name": "loop-item-param-2-subvar-a",
67+
"type": "string"}, {"name": "loop-item-param-2-subvar-b", "type": "string"},
68+
{"name": "my_pipe_param-loop-item", "type": "string"}, {"name": "my_pipe_param3",
69+
"type": "string"}, {"name": "my_pipe_param3-loop-item", "type": "string"}],
70+
"tasks": [{"name": "my-1st-inner-coop", "params": [{"name": "loop-item-param-2-subvar-a",
71+
"value": "$(params.loop-item-param-2-subvar-a)"}, {"name": "my_pipe_param-loop-item",
72+
"value": "$(params.my_pipe_param-loop-item)"}], "taskSpec": {"metadata": {"annotations":
73+
{"pipelines.kubeflow.org/component_spec_digest": "{\"name\": \"my-1st-inner-coop\",
74+
\"outputs\": [], \"version\": \"my-1st-inner-coop@sha256=feaa8a218c3caaa76fa1154bcca9fc485286e72814986c35d08edcd28d2a50b9\"}"},
7575
"labels": {"pipelines.kubeflow.org/cache_enabled": "true"}}, "params": [{"name":
7676
"loop-item-param-2-subvar-a", "type": "string"}, {"name": "my_pipe_param-loop-item",
7777
"type": "string"}], "steps": [{"args": ["set -e\necho \"op1-1\" \"$0\" \"$1\"\n",
@@ -80,11 +80,10 @@ metadata:
8080
{"name": "withitem-multiple-nesting-pipeline-for-loop-8", "params": [{"name":
8181
"iteration-number-4", "value": "$(params.iteration-number-4)"}, {"name": "loop-item-param-2-subvar-b",
8282
"value": "$(params.loop-item-param-2-subvar-b)"}, {"name": "loop-item-param-7",
83-
"value": "[4, 5]"}, {"name": "my_pipe_param3-loop-item", "value": "$(params.my_pipe_param3-loop-item)"},
84-
{"name": "my_pipe_param3", "value": "$(params.my_pipe_param3)"}], "taskRef":
85-
{"apiVersion": "custom.tekton.dev/v1alpha1", "kind": "PipelineLoop", "name":
86-
"withitem-multiple-nesting-pipeline-for-loop-8"}}]}}}, {"apiVersion": "custom.tekton.dev/v1alpha1",
87-
"kind": "PipelineLoop", "metadata": {"name": "withitem-multiple-nesting-pipeline-for-loop-8"},
83+
"value": "[4, 5]"}, {"name": "my_pipe_param3-loop-item", "value": "$(params.my_pipe_param3-loop-item)"}],
84+
"taskRef": {"apiVersion": "custom.tekton.dev/v1alpha1", "kind": "PipelineLoop",
85+
"name": "withitem-multiple-nesting-pipeline-for-loop-8"}}]}}}, {"apiVersion":
86+
"custom.tekton.dev/v1alpha1", "kind": "PipelineLoop", "metadata": {"name": "withitem-multiple-nesting-pipeline-for-loop-8"},
8887
"spec": {"iterateParam": "loop-item-param-7", "iterationNumberParam": "iteration-number-9",
8988
"pipelineSpec": {"params": [{"name": "iteration-number-4", "type": "string"},
9089
{"name": "iteration-number-9", "type": "string"}, {"name": "loop-item-param-2-subvar-b",
@@ -145,11 +144,7 @@ spec:
145144
params:
146145
- name: loop-item-param-2
147146
value: '[{"a": 1, "b": 2}, {"a": 10, "b": 20}]'
148-
- name: my_pipe_param
149-
value: $(params.my_pipe_param)
150147
- name: my_pipe_param-loop-item
151148
value: $(params.my_pipe_param)
152-
- name: my_pipe_param3
153-
value: $(params.my_pipe_param3)
154149
- name: my_pipe_param3-loop-item
155150
value: $(params.my_pipe_param3)

‎sdk/python/tests/compiler/testdata/nested_loop_counter.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ spec:
4848
params:
4949
- name: loop-item-param-1
5050
type: string
51+
- name: loop-item-param-3
52+
type: string
5153
tasks:
5254
- name: loop-3-range-for-loop-4
5355
params:

‎sdk/python/tests/compiler/testdata/nested_loop_counter_noninlined.yaml

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,18 @@ metadata:
3131
tekton.dev/resource_templates: '[{"apiVersion": "custom.tekton.dev/v1alpha1",
3232
"kind": "PipelineLoop", "metadata": {"name": "loop-3-range-for-loop-2"}, "spec":
3333
{"iterateNumeric": "loop-item-param-1", "pipelineSpec": {"params": [{"name":
34-
"loop-item-param-1", "type": "string"}], "tasks": [{"name": "loop-3-range-for-loop-4",
35-
"params": [{"name": "from", "value": "1"}, {"name": "loop-item-param-1", "value":
36-
"$(params.loop-item-param-1)"}, {"name": "to", "value": "2"}], "taskRef": {"apiVersion":
37-
"custom.tekton.dev/v1alpha1", "kind": "PipelineLoop", "name": "loop-3-range-for-loop-4"}}]}}},
38-
{"apiVersion": "custom.tekton.dev/v1alpha1", "kind": "PipelineLoop", "metadata":
39-
{"name": "loop-3-range-for-loop-4"}, "spec": {"iterateNumeric": "loop-item-param-3",
40-
"pipelineSpec": {"params": [{"name": "loop-item-param-1", "type": "string"},
41-
{"name": "loop-item-param-3", "type": "string"}], "tasks": [{"name": "loop-3-range-for-loop-6",
42-
"params": [{"name": "from", "value": "1"}, {"name": "loop-item-param-1", "value":
43-
"$(params.loop-item-param-1)"}, {"name": "loop-item-param-3", "value": "$(params.loop-item-param-3)"},
34+
"loop-item-param-1", "type": "string"}, {"name": "loop-item-param-3", "type":
35+
"string"}], "tasks": [{"name": "loop-3-range-for-loop-4", "params": [{"name":
36+
"from", "value": "1"}, {"name": "loop-item-param-1", "value": "$(params.loop-item-param-1)"},
4437
{"name": "to", "value": "2"}], "taskRef": {"apiVersion": "custom.tekton.dev/v1alpha1",
38+
"kind": "PipelineLoop", "name": "loop-3-range-for-loop-4"}}]}}}, {"apiVersion":
39+
"custom.tekton.dev/v1alpha1", "kind": "PipelineLoop", "metadata": {"name": "loop-3-range-for-loop-4"},
40+
"spec": {"iterateNumeric": "loop-item-param-3", "pipelineSpec": {"params": [{"name":
41+
"loop-item-param-1", "type": "string"}, {"name": "loop-item-param-3", "type":
42+
"string"}], "tasks": [{"name": "loop-3-range-for-loop-6", "params": [{"name":
43+
"from", "value": "1"}, {"name": "loop-item-param-1", "value": "$(params.loop-item-param-1)"},
44+
{"name": "loop-item-param-3", "value": "$(params.loop-item-param-3)"}, {"name":
45+
"to", "value": "2"}], "taskRef": {"apiVersion": "custom.tekton.dev/v1alpha1",
4546
"kind": "PipelineLoop", "name": "loop-3-range-for-loop-6"}}]}}}, {"apiVersion":
4647
"custom.tekton.dev/v1alpha1", "kind": "PipelineLoop", "metadata": {"name": "loop-3-range-for-loop-6"},
4748
"spec": {"iterateNumeric": "loop-item-param-5", "pipelineSpec": {"params": [{"name":

‎sdk/python/tests/compiler/testdata/nested_loop_global_param.yaml

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,8 +119,6 @@ spec:
119119
pipelines.kubeflow.org/cache_enabled: "true"
120120
- name: empty-loop-for-loop-3
121121
params:
122-
- name: param
123-
value: $(params.param)
124122
- name: param-loop-item
125123
value: $(params.param)
126124
taskSpec:

‎sdk/python/tests/compiler/testdata/nested_loop_global_param_noninlined.yaml

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,5 +99,3 @@ spec:
9999
params:
100100
- name: param-loop-item
101101
value: $(params.param)
102-
- name: param
103-
value: $(params.param)

‎sdk/python/tests/compiler/testdata/nested_loop_param.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
# Copyright 2022 kubeflow.org
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
15+
from kfp import dsl
16+
from kfp.components import load_component_from_text
17+
from kfp_tekton.tekton import Loop
18+
from kfp_tekton.compiler import TektonCompiler
19+
20+
21+
class Coder:
22+
def empty(self):
23+
return ""
24+
25+
26+
TektonCompiler._get_unique_id_code = Coder.empty
27+
28+
29+
def PrintOp(name: str, msg: str = None):
30+
if msg is None:
31+
msg = name
32+
print_op = load_component_from_text(
33+
"""
34+
name: %s
35+
inputs:
36+
- {name: input_text, type: String, description: 'Represents an input parameter.'}
37+
outputs:
38+
- {name: output_value, type: String, description: 'Represents an output paramter.'}
39+
implementation:
40+
container:
41+
image: alpine:3.6
42+
command:
43+
- sh
44+
- -c
45+
- |
46+
set -e
47+
echo $0 > $1
48+
- {inputValue: input_text}
49+
- {outputPath: output_value}
50+
""" % (name)
51+
)
52+
return print_op(msg)
53+
54+
55+
@dsl.pipeline("loop-3-range")
56+
def loop_3_range():
57+
with Loop.from_string('a,b,c', separator=',') as it0:
58+
with Loop.from_string('[A,B,C]') as it1:
59+
with Loop.from_string('1,2,3', separator=',') as it2:
60+
PrintOp("print", f"print {it0} {it1} {it2}")
61+
62+
63+
if __name__ == '__main__':
64+
TektonCompiler().compile(loop_3_range, __file__.replace('.py', '.yaml'))

0 commit comments

Comments
 (0)