WithParams

[kevinbache]

This PR adds WithParams support.

---

This change is 

[kevinbache]

/retest

[Ark-kun]

How does Argo resolve {{item.a}} when the key a is missing?

[Ark-kun]

/lgtm

[elikatsis]

Hello everyone,
@kevinbache nice job there!

There is just one small issue I'd like to address.
If a user passes a mixed list of static values (non-dict) and PipelineParams, then the output looks like this

withItems:
- 5
- !!python/object:kfp.dsl._pipeline_param.PipelineParam
  name: my-pipe-param
  op_name: null
  param_type: null
  pattern: '{{pipelineparam:op=;name=my-pipe-param}}'
  value: null

This should definitely be avoided. It can be easily achieved by performing some checks and raising ValueError() with a proper message.

Similarly, when users pass a mixed list of dicts and PipelineParams they either get

Traceback (most recent call last):
  File "withitem_basic.py", line 59, in <module>
    print(compiler.Compiler().compile(pipeline, package_path=None))
  File "/home/elias/git/hub/arrikto/kubeflow-pipelines/sdk/python/kfp/compiler/compiler.py", line 785, in compile
    workflow = self._compile(pipeline_func)
  File "/home/elias/git/hub/arrikto/kubeflow-pipelines/sdk/python/kfp/compiler/compiler.py", line 715, in _compile
    pipeline_func(*args_list)
  File "withitem_basic.py", line 34, in pipeline
    with dsl.ParallelFor(loop_args) as item:
  File "/home/elias/git/hub/arrikto/kubeflow-pipelines/sdk/python/kfp/dsl/_ops_group.py", line 183, in __init__
    loop_args = _for_loop.LoopArguments(loop_args, code)
  File "/home/elias/git/hub/arrikto/kubeflow-pipelines/sdk/python/kfp/dsl/_for_loop.py", line 43, in __init__
    if not set(item.keys()) == subvar_names:
AttributeError: 'PipelineParam' object has no attribute 'keys'

or

withItems:
- !!python/object:kfp.dsl._pipeline_param.PipelineParam
  name: my-pipe-param
  op_name: null
  param_type: null
  pattern: '{{pipelineparam:op=;name=my-pipe-param}}'
  value: null
- a: 1
  b: 2
- a: 10
  b: 20

All in all, I believe there should be errors and proper messages checking and informing for the following:
ParallelFor may get as arguments either:

• one PipelineParam,
• one primitive value,
• one dict, or
• a list of primitive values or a list of dicts with the same keys

And check for mixed list of dicts & primitives is missing, but I should have seen and mentioned it in the PR adding withItems. It can be fixed in a separate PR.

[Ark-kun]

If a user passes a mixed list of static values (non-dict) and PipelineParams, then the output looks like this

Nice catch. I think we should not allow PipelineParams in withItems. It should either be a list of constant strings/dicts or a single PipelineParam

• !!python/object:kfp.dsl._pipeline_param.PipelineParam

All values need to be converted to strings before inserting into the workflow dict.

pattern: '{{pipelineparam:op=;name=my-pipe-param}}'

This one would have been caught by #2055

[kevinbache]

/retest

[kevinbache]

@elikatsis thanks for the catches! the dict keys parity check in the LoopArguments constructor should catch the mixed list of dicts+singletons case that you mentioned. feel free to let me know if you have any other questions and thanks again.

/ping for re-approves

[kevinbache]

responding

[Ark-kun]

/lgtm

[Ark-kun]

We should probably simplify some of the scratch parts of the test files in another PR.
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• sdk/OWNERS [Ark-kun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kevinbache]

/retest