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

Fix KFP code generation issues for pipeline parameters #3093

Merged
merged 5 commits into from
Feb 7, 2023

Conversation

ptitzler
Copy link
Member

@ptitzler ptitzler commented Jan 27, 2023

Code generation produces incorrect results (and in some scenarios fails) for some pipeline parameter scenarios. Examples below, as observed in the generated Python DSL:

  • the parameter description contains the " character (YAML parser chokes)
     - {name: a_quote, type: String, description: 'description with a "', default: "string default contains \" anywhere", optional: true}
    
  • the parameter value contains the " character for parameter type String
  • no default values are specified, e.g.
     i2: int = ,
    
  • a default value of zero is defined for numeric parameter types (input spec is missing this value), e.g.
      - {name: i3, type: Integer, description: 'an int with default value of 0', optional: true}
    
  • a default value of False for Bool-typed parameters is missing , e.g.
    - {name: b1, type: Boolean, description: 'a boolean with default value False', optional: true}
    

Some of the problems were caused by the fact that 0 and <empty-string> were considered to be equivalent (or not equivalent) to None. To avoid these issues the PipelineParameter class now explicitly assigns None to parameter values and default values that are set to the empty string by the UI.

Signed-off-by: Patrick Titzler ptitzler@us.ibm.com

What changes were proposed in this pull request?

  • Fix incorrect implementation

How was this pull request tested?

To test create and export a KFP pipeline that utilizes pipeline parameters, covering the scenarios listed in the problem description. The generated Python DSL code should properly reflect the pipeline parameters, their values, and types.

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.

Signed-off-by: Patrick Titzler <ptitzler@us.ibm.com>
@ptitzler ptitzler added kind:bug Something isn't working component:pipeline-editor pipeline editor platform: pipeline-Kubeflow Related to usage of Kubeflow Pipelines as pipeline runtime labels Jan 27, 2023
generic_component_template = Environment(
loader=PackageLoader("elyra", "templates/kubeflow/v1")
).get_template("generic_component_definition_template.jinja2")
template_env = Environment(loader=PackageLoader("elyra", "templates/kubeflow/v1"))

Check warning

Code scanning / CodeQL

Jinja2 templating with autoescape=False

Using jinja2 templates with autoescape=False can potentially allow XSS attacks.
@ptitzler ptitzler added the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Jan 30, 2023
Signed-off-by: Patrick Titzler <ptitzler@us.ibm.com>
@ptitzler ptitzler changed the title KFP code generation: escape double quote character in pipeline parameters Fix KFP code generation issues for pipeline parameters Jan 31, 2023
Signed-off-by: Patrick Titzler <ptitzler@us.ibm.com>
@ptitzler
Copy link
Member Author

ptitzler commented Feb 1, 2023

Linting errors and subsequent test failures are unrelated to the changes introduced by this PR. A new major version of black was published yesterday. We currently have no upper version cap in place. #3096

@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 Feb 1, 2023
@ptitzler ptitzler added this to the 3.14.3 milestone Feb 2, 2023
Copy link
Member

@akchinSTC akchinSTC left a comment

Choose a reason for hiding this comment

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

thumbs up LGTM

@akchinSTC akchinSTC merged commit 321516a into elyra-ai:main Feb 7, 2023
@ptitzler ptitzler deleted the fix-parm-description branch February 7, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:pipeline-editor pipeline editor kind:bug Something isn't working platform: pipeline-Kubeflow Related to usage of Kubeflow Pipelines as pipeline runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants