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

Make multi-valued input fields more user-friendly #2927

Merged
merged 186 commits into from
Sep 27, 2022

Conversation

kiersten-stokes
Copy link
Member

@kiersten-stokes kiersten-stokes commented Sep 14, 2022

Fixes #2750
Fixes #2738
Fixes #2923
Fixes #2906

Corresponds to elyra-ai/pipeline-editor#201.

Excuse all the extraneous commits - I was building and merging with the pipeline-rjsf branch so often and for so long that I didn't want to mess with the history by cherry-picking. The diff, however, is accurate.

Note that the pipeline editor may go blank when trying to edit multi-valued fields at this point during development. Front end changes required before things are fully functional.

What changes were proposed in this pull request?

This PR re-factors a significant portion of backend node and pipeline properties handling in order to centralize and simplify validation and processing of Elyra-owned properties (which includes properties such as mounted_volumes, kubernetes_secrets, etc. that are not properties considered a part of the operator/component definition itself).

TODOs:

  • frontend: pipeline editor goes blank
  • frontend: tooltip for airflow nodes doesn't render properties properly
  • frontend: update CSS/styling of new input format
  • frontend: update default property propagation for new format
  • frontend/backend: integrate UI placeholders (or decide on a title) for each input field
  • frontend: update property display on node mouseover
  • frontend: remove/do not persist properties with all empty values (may be tackled in followup since backend validation will catch)
  • frontend: fix env var parsing and refresh for new format
  • frontend: frontend validation (may require schema updates on backend) backend validation will handle most cases; frontend validation can be added in a follow-up if required
  • frontend/backend: investigate improving display of Component Source property (Add a control for dictionary types in the node properties panel #2256) (may be better tackled in a follow-up PR) non-trivial, will have to tackle in a follow-up
  • frontend: migration
  • documentation Update node properties reference topic in pipelines topic (for many properties we currently discuss the string input format)
  • pipeline editor release + merge

How was this pull request tested?

  • Fix backend tests
  • Fix integration tests
  • Reviewed output of make docs (section "Node properties reference" in the pipelines documentation topic)

Manual test checklist. If checked, end-to-end test was successful:

  • node properties are rendered correctly (this includes validation for missing and invalid property input)
  • node tooltip on canvas renders properties correctly
  • properties are persisted as expected in .pipeline file
  • properties are persisted as expected in exported DAG
  • properties are properly applied to pod

pipeline properties:

  • data volumes
  • annotations
  • environment variables
  • tolerations
  • secrets
  • disable node caching

node properties (KFP)

  • data volumes - generic components
  • data volumes - custom components
  • annotations - generic components
  • annotations - custom components
  • environment variables - generic components
  • tolerations - generic components
  • tolerations - custom components
  • secrets - generic components
  • disable node caching - custom components

node properties (Airflow)

  • data volumes - generic components
  • data volumes - custom components
  • annotations - generic components
  • annotations - custom components
  • environment variables - generic components
  • tolerations - generic components
  • tolerations - custom components
  • secrets - generic components

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.

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.

Found 19 potential problems in the proposed changes. Check the Files changed tab for more details.

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.

Found 19 potential problems in the proposed changes. Check the Files changed tab for more details.

@ptitzler
Copy link
Member

Currently properties for custom nodes are visually grouped into categories:

  • Inputs
  • Outputs
  • Additional properties
  • Component source

We should do the same for generic nodes (either in this PR or a follow-up). Doing so would provide for a more uniform user experience and prepare for the migration of the current one-off implementation to one that utilize a genuine custom component to run noteboooks and scripts.

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.

Found 19 potential problems in the proposed changes. Check the Files changed tab for more details.

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.

Found 19 potential problems in the proposed changes. Check the Files changed tab for more details.

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.

Found 15 potential problems in the proposed changes. Check the Files changed tab for more details.

@ptitzler

This comment was marked as resolved.

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.

Found 15 potential problems in the proposed changes. Check the Files changed tab for more details.

ajbozarth added a commit to elyra-ai/pipeline-editor that referenced this pull request Sep 27, 2022
…201)

Addresses issues in elyra-ai/elyra#2927

Co-authored-by: Alex Bozarth <ajbozart@us.ibm.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.

Found 15 potential problems in the proposed changes. Check the Files changed tab for more details.

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.

Found 15 potential problems in the proposed changes. Check the Files changed tab for more details.

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! Very nice work. Gone are the days of esoteric input!

@ajbozarth
Copy link
Member

just pushed to update to point at the RC release of pipeline editor now that elyra-ai/pipeline-editor#201 is merged and released

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.

Found 15 potential problems in the proposed changes. Check the Files changed tab for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:documentation Improvements or additions to documentation component:pipeline-editor pipeline editor impact:requires-pipeline-editor-release PR is dependent on a pipeline editor release that needs to be published first. impact:requires-pipeline-migration kind:enhancement New feature or request
Projects
None yet
6 participants