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

Add support for pipeline parameters #3001

Merged
merged 95 commits into from
Dec 13, 2022

Conversation

kiersten-stokes
Copy link
Member

@kiersten-stokes kiersten-stokes commented Nov 7, 2022

This PR, once out of draft, will address all bullet points in #2994 with the exception of the CLI support.

Related: discussion #2970

What changes were proposed in this pull request?

This PR includes both the frontend and backend changes required to support pipeline parameters.

  • a new API endpoint and handler to fetch pipeline parameter schema /pipeline/{processor_type}/parameters
  • a new class to represent a well-defined PipelineParameter object (one for each runtime type that supports parameters) and supply the schema with runtime-specific input types
  • add plumbing to support a fourth "input type" (aka "widget type") called parameter for relevant custom component properties
  • add an extra property pipeline_parameters to the generic component properties
  • add support for converting and passing parameters to the Pipeline object after submission
  • add support for adding pipeline parameters to the generated DSL
  • add support for passing pipeline parameters as arguments to the KFP bootstrapper and setting them as environment variables in the bootstrapped container

NOTE: After some discussion, it was decided that generic pipelines should support the set of properties/parameters/etc that are supported by all runtimes (ie an intersection). While changes will be required to make this true of how properties are handled, this pipeline parameter support PR does follow this directive.

TODO

  • backend: rename variables and files that include the substring parameter to use property instead (where applicable)
  • backend: implement processing and validation logic for custom and generic components
  • backend: fix KFP icon rendering
  • backend: add backend tests
  • backend: address all TODOs
  • backend: add validation and/or trap error during compile for mismatched KFP types
  • backend/frontend: add description to individual fields for each parameter
  • backend/frontend: only allow selection of parameter type(s) that match the given component-defined type - may be better in a follow-up PR
  • frontend: add support for new third "Pipeline Parameters" panel tab
  • frontend: fix style for tooltip
  • frontend: fix validation issues
  • frontend: add types to submit/export dialog
  • frontend: (nice to have) display custom message in above tab for runtimes that do not support parameters
  • frontend: add params to the appropriate location in the pipeline JSON
  • frontend: add new widget type named parameter; component properties will appear as below in the pipeline JSON
    • "url": {
        "widget": "parameter",
        "value": "user_defined_param_1"
      },
  • frontend: add ability to change pipeline parameter default values on pipeline submit
  • frontend: add ability to change pipeline parameter default values on pipeline export
  • frontend: implement parameter widget selection for custom components
  • frontend: implement parameter property for generic components
  • documentation: add pipeline parameters content to docs or open another issue/PR to address -> Add documentation for pipeline parameters #3046

How was this pull request tested?

Manual testing.
Added new backend tests.

NOTE: To test this PR as it relates to generic components, you will have to update the ELYRA_BOOTSTRAP_SCRIPT_URL env var to point to this branch https://raw.githubusercontent.com/kiersten-stokes/elyra/pipeline-params/elyra/kfp/bootstrapper.py

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@elyra-bot
Copy link

elyra-bot bot commented Nov 7, 2022

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link: Binder

@kiersten-stokes kiersten-stokes added this to the 3.14.0 milestone Nov 7, 2022
@kiersten-stokes kiersten-stokes added kind:enhancement New feature or request component:pipeline-editor pipeline editor labels Nov 7, 2022
elyra/pipeline/pipeline_definition.py Fixed Show fixed Hide fixed
elyra/pipeline/component.py Fixed Show fixed Hide fixed
elyra/pipeline/pipeline.py Fixed Show fixed Hide fixed
@ptitzler ptitzler self-requested a review November 7, 2022 23:42
@akchinSTC akchinSTC added the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Nov 8, 2022
@marthacryan marthacryan self-assigned this Nov 8, 2022
kiersten-stokes and others added 6 commits November 9, 2022 18:35
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Martha Cryan and others added 6 commits December 8, 2022 13:51
Signed-off-by: Martha Cryan <Martha.Cryan@ibm.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
@kiersten-stokes
Copy link
Member Author

Note: the CodeQL warnings on this PR are 'the usual suspects' and have not been introduced by any changes made in this PR. (I think they may be triggered by the renaming of the component_parameter.py file to properties.py.)

@ptitzler
Copy link
Member

ptitzler commented Dec 9, 2022

Note: the CodeQL warnings on this PR are 'the usual suspects' and have not been introduced by any changes made in this PR. (I think they may be triggered by the renaming of the component_parameter.py file to properties.py.)

I dismissed them.

@kiersten-stokes kiersten-stokes marked this pull request as ready for review December 9, 2022 22:57
@ptitzler ptitzler self-requested a review December 12, 2022 16:36
Martha Cryan and others added 6 commits December 12, 2022 11:35
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Martha Cryan <Martha.Cryan@ibm.com>
@ptitzler ptitzler added the impact:requires-pipeline-editor-release PR is dependent on a pipeline editor release that needs to be published first. label Dec 12, 2022
kiersten-stokes and others added 3 commits December 12, 2022 17:25
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
Signed-off-by: Kiersten Stokes <kierstenstokes@gmail.com>
@ptitzler ptitzler removed the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Dec 13, 2022
Copy link
Member

@ptitzler ptitzler left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks to you both for an awesome new feature!

Signed-off-by: Alan Chin <akchin@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:pipeline-editor pipeline editor impact:requires-pipeline-editor-release PR is dependent on a pipeline editor release that needs to be published first. kind:enhancement New feature or request sizing: L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants