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 initial support for rjsf in pipeline properties #2780

Merged
merged 106 commits into from
Sep 13, 2022

Conversation

marthacryan
Copy link
Member

@marthacryan marthacryan commented Jun 9, 2022

Corresponding to elyra-ai/pipeline-editor#197.

Fixes #2759

Todo:

  • basic pipeline properties support
  • basic generic node properties support
  • basic "canvas properties" support
  • finish airflow support
  • link in custom components
  • link in validation
  • add in required fields
  • add inputpath as oneof
  • frontend: pipeline migration
    • backend: double check that relevant test pipelines are migrated properly (once migration is finalized)
  • go through code to make sure all utilities and edits etc. are all corresponding to the updated schema

Testing Follow Up TODOs:

  • section headers: description
  • backend validation requires updates (pending some frontend changes)
  • pipeline JSON occasionally gets corrupted (e.g. one node's properties/values find their way into another node's component_parameters stanza) when connected 2 nodes (no longer seeing; uncheck if not the case)
  • frontend: properties don't always save if you navigate away or sometimes disappear after connecting 2 nodes - possibly related to above task
  • frontend: schema shows up in pipeline JSON
  • frontend: headers appear in pipeline properties app_data.properties.pipeline_defaults stanza
  • frontend: headers appear in node properties component_parameters stanzas
    • backend: code can be updated (validation.py:493-500) after this change is made
  • frontend: properties not listed as required in the schema show as required in properties panel
  • frontend: change look of some items (see comment)
  • frontend: pipeline editor canvas goes blank occasionally when editing array values (haven't been able to reproduce recently)
  • inputpath structure in pipeline JSON needs fixed
  • drag and drop of notebooks/scripts
  • pipeline properties - placeholders for list-valued properties
  • [ ] user-friendly key-value inputs (env variables etc all) - pushing to next release
  • pipeline default property propagation (including display in node properties)
  • pipeline defaults: section headers (node defaults, generic node defaults)
  • custom components: component description missing
  • generic components: [node] label description missing
  • certain string property validation (frontend) erroneously require objects after connecting to another node
  • properties with empty strings as defaults erroneously pass frontend validation when first dragging component into pipeline
  • component_source parameter should be moved out of component_parameters stanza of pipeline JSON (some frontend work required in order to still display the property in properties panel)
  • frontend validation fails for node runtime image even if a pipeline default is supplied
  • frontend: properties keep the value when changing input type (e.g. when changing from raw value to file input type, the raw value default is still shown in the value field)
  • frontend: airflow component tool tips not displaying all properties

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 9, 2022

Thanks for making a pull request to Elyra!

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

@akchinSTC akchinSTC modified the milestones: 3.10.0, 3.11.0 Jul 5, 2022
@ptitzler ptitzler added impact:requires-pipeline-migration status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. impact:blocker labels Jul 21, 2022
@ptitzler

This comment was marked as resolved.

@ptitzler

This comment was marked as resolved.

@ptitzler

This comment was marked as resolved.

@kiersten-stokes

This comment was marked as resolved.

@marthacryan

This comment was marked as resolved.

@ptitzler ptitzler added the component:pipeline-editor pipeline editor label Sep 12, 2022
@ptitzler

This comment was marked as resolved.

@ptitzler

This comment was marked as resolved.

@ptitzler
Copy link
Member

Tooltip for custom nodes in Airflow are now looking good ...
image

For property values that are provided via input files (URL in node download file in the example below), can we prepend the file name with a string that indicates that we are displaying a reference, not the value? The count rows node is an example of a property that is also defined via reference, specifically a reference to the output of an upstream node:

2022-09-13_10-16-26 (1)

@kiersten-stokes

This comment was marked as resolved.

@ptitzler
Copy link
Member

The validation message is invalid if one does not specify a file as string input:

Good point! I've fixed validation to allow an empty string for file input types for non-required properties. From here, the processor behaves the same way as an empty string for raw input as well.

Thank you! Looking good. It appears to preserve default values, which I would expect to happen since the user didn't provide a property value.

ajbozarth added a commit to elyra-ai/pipeline-editor that referenced this pull request Sep 13, 2022
Corresponds to elyra-ai/elyra#2780

Co-authored-by: Alex Bozarth <ajbozart@us.ibm.com>
@kiersten-stokes kiersten-stokes merged commit 9caea32 into elyra-ai:main Sep 13, 2022
@kevin-bates
Copy link
Member

Congratulations (and thank you ❤️) to everyone that worked on this - it's one for the ages! 🎉

@thesuperzapper
Copy link
Member

I would also love to thank everyone for their amazing effort!!!

❤️

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.

allow setting component inputs from files (KFP & Airflow)
7 participants