elyra
elyra copied to clipboard
Make multi-valued input fields more user-friendly
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.
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.
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)
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:
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.
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
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
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 beEqual
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 (noteExists
-> 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))
@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.
![Screen Shot 2022-09-20 at 3 06 31 PM](https://user-images.githubusercontent.com/31816267/191354251-e2878996-76a1-4b5d-b169-536c5e772f0b.png)
[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
![Screen Shot 2022-09-20 at 3 16 05 PM](https://user-images.githubusercontent.com/31816267/191356005-87662047-f2c9-4711-992a-4e28f863fa98.png)
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
![Screen Shot 2022-09-20 at 3 23 27 PM](https://user-images.githubusercontent.com/31816267/191357302-c44c5c90-979c-4aad-81f3-5d0e23c99c10.png)
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
![Screen Shot 2022-09-20 at 3 31 05 PM](https://user-images.githubusercontent.com/31816267/191358525-da15bd1b-3d6a-4327-8e1d-f47b489620f1.png)
the property order is different (pipeline defaults (lhs) vs node property (rhs))
Fixed! As seen in screenshot immediately above
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.
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
Tolerations widget (node properties): missing "" in enum. the widget should render the same no matter where it is displayed (node properties vs pipeline defaults)
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
)
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
Environment variables widget :
Environment variables that are detected in the notebook/script
are not properly populated
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
Added https://github.com/elyra-ai/elyra/pull/2927/commits/1d2875ef0f8f9691b8391e01201de977487be7b4, which updates section "Node properties reference" in the pipeline documenation.
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)
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
just pushed updates to address @ptitzler issues discussed in scrum today
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
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.
Bug in line https://github.com/kiersten-stokes/elyra/blob/property-refactor/elyra/pipeline/component_parameter.py#L294 - needs to be elif
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