Make multi-valued input fields more user-friendly

[kiersten-stokes]

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:

• [x] frontend: pipeline editor goes blank
• [x] frontend: tooltip for airflow nodes doesn't render properties properly
• [x] frontend: update CSS/styling of new input format
• [x] frontend: update default property propagation for new format
• [x] frontend/backend: integrate UI placeholders (or decide on a title) for each input field
• [x] frontend: update property display on node mouseover
• [x] frontend: remove/do not persist properties with all empty values (may be tackled in followup since backend validation will catch)
• [x] frontend: fix env var parsing and refresh for new format
• [x] ~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
• [x] ~frontend/backend: investigate improving display of Component Source property (#2256) (may be better tackled in a follow-up PR)~ non-trivial, will have to tackle in a follow-up
• [x] frontend: migration
• [x] 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?

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

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[bot]]

Thanks for making a pull request to Elyra!

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

[ptitzler]

If one chooses (or leaves) the default selection, this is persisted in the .pipeline file:

"disallow_cached_output": "Use runtime environment default",

and

        "properties": {
             ...
            "disallow_cached_output": "Use runtime environment default"
          },

Since Use runtime environment default is a UI label that could change over time, it'd be safer to persist null instead to indicate that the user did not choose an explicit property value.

[ptitzler]

I edited a pipeline file when the VPE failed. The .pipeline file can no longer be opened due to

Console log:

3935.4159b022aa6d82e44127.js?v=4159b022aa6d82e44127:2 TypeError: Cannot set properties of undefined (setting 'ui:placeholder')
    at index.js:1:37929
    at Proxy.forEach (<anonymous>)
    at p (index.js:1:37718)
    at index.js:1:38167
    at Immer.produce (immer.cjs.development.js:792:1)
    at t.propagatePipelineDefaultProperties (index.js:1:37339)
    at t.getAllPaletteNodes (index.js:1:37213)
    at index.js:1:67826
    at Nu (3935.4159b022aa6d82e44127.js?v=4159b022aa6d82e44127:2:61501)
    at uo (3935.4159b022aa6d82e44127.js?v=4159b022aa6d82e44127:2:69686)

[ptitzler]

Generic nodes: the runtime image property is now rendered as the last property in the list, even though it is the most important one:

Pipeline properties:

Node properties:

[ptitzler]

Kubernetes pod annotations widget:

• Validation doesn't detect missing annotation value

  As a result the invalid key/value combination is persisted in the .pipeline file and exported, producing an invalid annotation.

[ptitzler]

Environment variables widget:

• need to add placeholder values (ENV_VAR, value) that are consistent with other widgets

• if no key/value pair is enters a fatal error occurs during validation

  

• If no value is specified, a cryptic validation error is raised:

  

  Property has an improperly formatted env variable key value pair.
  

  In previous releases an empty string was considered valid. I believe we have to retain that behavior because otherwise the environment variable detection code ("refresh" button) would produce invalid list entries if the analyzed source code does not set a default value.

• "Refresh" button doesn't work - nothing is displayed in the env variable list

• spaces in environment variable names should not be allowed. while it might be possible to process such variables programmatically, it's generally a major pain in shell environments/scripts

[ptitzler]

data volumes widget:

• Validation error metadata is inconsistent across widgets. One would expect the same validation error to be raised if a constraint violation is detected. For example, assume widget X requires input of a key/value pair and widget Y also requires input of a key/value pair. If the user doesn't provide a value, validation should return messages that are consistent. For data volumes the validation error metadata is

Note the use of "<missing required value>"

• no validation error is raised if only a pvc name is specified, but not a mount path

  

[ptitzler]

Kubernetes tolerations widget:

• key is not a required property. the previously implemented validation code should already have the complete logic built that verifies the constraints that need to be enforced -> reference: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/ ("An empty key with operator Exists matches all keys, values and effects which means this will tolerate everything.")

• the initial selected value for operator should be Equal to properly model the Kubernetes behavior

• there's a problem with how properties are assigned. note the discrepancy between the error metadata (effects -> Exists) and the inputs (effects -> no input)

  

• operator property is not properly recognized (note Exists -> missing required value)

• effect property should be an select list enum ["", "NoExecute","NoSchedule","PreferNoSchedule")

  • the empty value is valid ("An empty effect matches all effects with key key1.") -> previous validation implementation should have accepted this already as valid input

• the property order is different (pipeline defaults (lhs) vs node property (rhs))

  

[kiersten-stokes]

@ptitzler thanks again for the review! I believe I've addressed everything that I could from the above comments. Details below

Since Use runtime environment default is a UI label that could change over time, it'd be safer to persist null instead to indicate that the user did not choose an explicit property value.

I've changed this such that only True and False are listed. The empty string is persisted, which is handled during property conversion in the DisallowCachedOutput class (though full disclosure, I haven't been able to thoroughly test this due to the pipeline editor white-outs).

Generic nodes: the runtime image property is now rendered as the last property in the list, even though it is the most important one

I've removed the RuntimeImage ElyraProperty class and added the property manually in the generic and pipeline properties templates (as it was before), restoring it to it's rightful place in the property panel 😄

Validation doesn't detect missing annotation value

Addressed and added tiny test case to the existing test for invalid annotations.

[environment variables] Need to add placeholder values (ENV_VAR, value)that are consistent with other widgets

I haven't quite been able to figure this one out. The placeholders exist in the properties JSON the same way as for the others but aren't showing up. @marthacryan (or @ajbozarth) I think I might need your help on this one. Here's the originating comment.

if no key/value pair is enters a fatal error occurs during validation

Fixed for all properties!

[environment variables] If no value is specified, a cryptic validation error is raised:

Now when no value is specified, instances are silently removed from the list (same behavior as current main).

"Refresh" button doesn't work - nothing is displayed in the env variable list

Yeah this has been on my radar, and I believe after looking into it that it is a frontend task - tagging @ajbozarth since I think he implemented the functionality. Here's the originating comment.

spaces in environment variable names should not be allowed. 

Fixed with below message and added test case to existing test

no validation error is raised if only a pvc name is specified, but not a mount path

Fixed with below message and added test case to existing test

validation - replace with what the actual entered value is

Done! As seen in the above screenshots

[kubernetes tolerations] key is not a required property. the previously implemented validation code should already have the complete logic built that verifies the constraints that need to be enforced

Empty key, value, and effect are now all allowed in the correct circumstances

[kubernetes tolerations] the initial selected value for operator should be Equal to properly model the Kubernetes behavior [kubernetes tolerations] operator property is not properly recognized (note Exists -> missing required value)

I've updated this on the backend such that "Equal" is set as the default in the schema. I believe the second part, however, will require a frontend change. Or at least feedback as to what I might need to change in the schema. Basically, the value is only persisted once it's changed for the first time. @marthacryan let me know if there's something I could change to fix this! Originating comment here

effect property should be an select list enum ["", "NoExecute", "NoSchedule", "PreferNoSchedule"]

Done

the property order is different (pipeline defaults (lhs) vs node property (rhs))

Fixed! As seen in screenshot immediately above

[ptitzler]

We should tweak the CSS. The current label padding for select lists (see highlighted areas) on top is much smaller than on the bottom. As a result the label is rendered closer to the previous element than the one that the label applies to.

[ptitzler]

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

• proper rendering
• proper labels
• persistence in .pipeline file
• persistence in exported DAG
• pod properties verified

pipeline properties:

• [ ] data volumes
• [ ] annotations
• [ ] environment variables
• [ ] tolerations
• [ ] secrets

node properties (KFP)

• [ ] data volumes
• [ ] annotations
• [ ] environment variables
• [ ] tolerations
• [ ] secrets

node properties (Airflow)

• [ ] data volumes
• [ ] annotations
• [ ] environment variables
• [ ] tolerations
• [ ] secrets

[ptitzler]

Tolerations widget (node properties): missing "" in enum. the widget should render the same no matter where it is displayed (node properties vs pipeline defaults)

[ptitzler]

Since Use runtime environment default is a UI label that could change over time, it'd be safer to persist null instead to indicate that the user did not choose an explicit property value.

I've changed this such that only True and False are listed. The empty string is persisted, which is handled during property conversion in the DisallowCachedOutput class (though full disclosure, I haven't been able to thoroughly test this due to the pipeline editor white-outs).

Observed behavior is:

• label not shown in UI (but should be shown for clarity)
• empty string is not persisted (which is what should make the most sense, or null)
• property value is stored as string ("True", "False") but should be JSON boolean (true, false)

[ptitzler]

Volume mount widget:

• if no value is provided for the mount path the validation message metadata lists / instead of ""

  "data": {
    "nodeID": "cf9a1a8e-6d75-4c28-a9fb-41ac0ad9d6e5",
    "nodeName": "Untitled.ipynb",
    "propertyName": "mounted_volumes",
    "value": {
      "path": "/",
      "pvc_name": ""
    }
  }
  

Caused by https://github.com/elyra-ai/elyra/blob/01b3b7cbc7bcdb7f06d83a77b367b11e31fcf506/elyra/pipeline/component_parameter.py#L387 Not sure what the purpose of this manipulation is, but it shouldn't be performed if the provided path is an empty string

[ptitzler]

Environment variables widget :

Environment variables that are detected in the notebook/script

are not properly populated

[ptitzler]

Added bug fix for https://github.com/elyra-ai/elyra/pull/2927#issuecomment-1253461505 in https://github.com/elyra-ai/elyra/pull/2927/commits/d96cb11255c4f2f33ced53c39b73d0b0dcd00d0b and addressed two other issues not listed above

[ptitzler]

Added https://github.com/elyra-ai/elyra/pull/2927/commits/1d2875ef0f8f9691b8391e01201de977487be7b4, which updates section "Node properties reference" in the pipeline documenation.

[ptitzler]

https://github.com/elyra-ai/elyra/pull/2927/commits/2b358416d8a246718d6adc160ea13597456034ee renames all occurrences and variations of "disallow cached output" to "disable node caching" in the source code (variables, methods, etc)

[ajbozarth]

I just pushed the update that add migration. If you created a pipeline on this branch previously the migration will error, but you can just update the version in the json to 8 and it should be valid

[ajbozarth]

just pushed updates to address @ptitzler issues discussed in scrum today

[ptitzler]

just pushed updates to address @ptitzler issues discussed in scrum today

Thank you! I confirmed

• the width is now consistent

• label padding for select lists is now more balanced

  

[ptitzler]

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.

[ptitzler]

Bug in line https://github.com/kiersten-stokes/elyra/blob/property-refactor/elyra/pipeline/component_parameter.py#L294 - needs to be elif

[ajbozarth]

just pushed to update to point at the RC release of pipeline editor now that https://github.com/elyra-ai/pipeline-editor/pull/201 is merged and released