Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sdk.v2): Parse pipeline param in argument values #5183

Merged

Conversation

chensun
Copy link
Member

@chensun chensun commented Feb 24, 2021

Description of your changes:
To support string concatenation in pipeline params, for instance:

@dsl.pipeline(name='pipeline-with-pipelineparam-containing-format')
def my_pipeline(name: str = 'KFP'):
  print_op('Hello {}'.format(name))

Checklist:

@google-oss-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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:

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

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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:

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

@chensun chensun changed the title [Draft] parse pipeline param in argument values feat(sdk.v2): Parse pipeline param in argument values Mar 16, 2021
@chensun chensun marked this pull request as ready for review March 16, 2021 07:48
@chensun chensun requested a review from neuromage March 16, 2021 07:49
@chensun chensun force-pushed the parse-pipeline-param-in-arguments branch from 1d6dfdf to 04955fe Compare March 16, 2021 08:02
@chensun chensun force-pushed the parse-pipeline-param-in-arguments branch from 04955fe to fe7f409 Compare March 16, 2021 08:02
# argument_value contains PipelineParam placeholders which needs to be
# replaced. And the input needs to be added to the task spec.
for param in pipeline_params:
additional_input_name = '{}-{}'.format(input_name, param.full_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this format for parameter name by convention? Should this be a shared method instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming here is only for one special scenario -- compiler injects an input parameter, which so far only happens in this specific code location. i.e. if it's extracted into a shared method, that method will only be called here. So probably not necessarily to make a method for this?

That said, there're two requirements for the naming conversion:

  1. The name needs to be unique among all injected inputs for the current component. (This is guaranteed because param.full_name is unique)
  2. The name also shouldn't collide with any existing user defined input for the current component.

I added some comments in the code, and changed to use double dashes to form the name, so hopefully it should rarely collide with user defined inputs. I also added a check, in case name collision happens, we at least throw an error here.

Of course, we can have some logic to iterate through all the existing input names, and always generate a unique name. But I feel it's a bit overkill, with the double dash and collision check, it should be good for most cases. WDYT?

@neuromage
Copy link
Contributor

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants