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

Update kfp component files to include optional parameter #1854

Merged
merged 5 commits into from Jul 12, 2021

Conversation

akchinSTC
Copy link
Member

@akchinSTC akchinSTC commented Jun 30, 2021

  • Update the kfp component definitions to align with best practices
    and include descriptions and source license information
  • Set directory as source when reading local custom components
  • Remove runtime type from reader abstract method

What changes were proposed in this pull request?

How was this pull request tested?

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 Jun 30, 2021

Thanks for making a pull request to Elyra!

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

@ptitzler
Copy link
Member

Could we add a comment at the beginning of each example component file:

  • Since Elyra re-uses existing components that were published by somebody else, we should document the raw URL where the YAML file originally came from , e.g.
    # Component source location: https://...
    
  • Often there is not enough information in the specification for the user to make an informed decision about valid values. If one exists, we should reference a web page (if applicable) that contains more details. For example, https://github.com/kubeflow/pipelines/blob/master/components/kubeflow/kfserving/README.md
    # Component details: https://
    

@lresende
Copy link
Member

@akchinSTC Could you please update the license and list the origin + license of these component files

@lresende
Copy link
Member

lresende commented Jul 7, 2021

@akchinSTC @kiersten-stokes Could you please resolve conflicts.

Copy link
Member

@kiersten-stokes kiersten-stokes left a comment

Choose a reason for hiding this comment

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

I think the linting issue below is the only outstanding thing for this PR (I say as if I didn't introduce the problem...)

elyra/pipeline/component_parser_kfp.py Show resolved Hide resolved
Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

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

We need to make the registry aware of the additional location where components are located either by convention and/or by explicitly adding things on the registry JSON with the proper type and not let a reader calculate the location based on the type of the runtime, because then that reader looses its flexibility

elyra/pipeline/component.py Outdated Show resolved Hide resolved
Copy link
Member

@kiersten-stokes kiersten-stokes left a comment

Choose a reason for hiding this comment

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

LGTM! Tested an example for each runtime and each succeeded

@akchinSTC akchinSTC added this to the 3.0.0 milestone Jul 8, 2021
akchinSTC and others added 5 commits July 9, 2021 09:33
Update the kfp component definitions to align with best practices
and include descriptions and source license information
- Omit new fields from component properties in pipeline parser
- Change integration test to reference new component name
- Remove runtime type from reader abstract method
Copy link
Member

@lresende lresende left a comment

Choose a reason for hiding this comment

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

I am not sure if for file-based directory of components we need to list one by one in the registry, but for this PR I guess it's ok to avoid delays, but we will need to revisit this in the future.

@lresende lresende merged commit bb6e05e into elyra-ai:master Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants