Add support for runtime image and env vars in pipeline default properties #2656

kiersten-stokes · 2022-04-11T20:34:01Z

Upstream PR: elyra-ai/pipeline-editor#190

Fixes #1130; fixes #1768; fixes #1843; fixes #2268

Move https://github.com/elyra-ai/elyra/blob/master/packages/pipeline-editor/src/pipelineProperties.ts to backend
Add new API endpoint to elyra/api/elyra.yaml
Display global list items in node properties
Use constants for global properties e.g. PIPELINE_PROPERTY.RUNTIME_IMAGE (tentative name should describe that its global)

Documentation updates in follow up PR #2668

What changes were proposed in this pull request?

This PR adds support for global pipeline properties. A new function (propagate_global_properties) is added to the PipelineDefinition object and called during PipelineDefinition __init__. This function first grabs the global properties from the primary pipeline app_data/properties stanza. For each property, all nodes (of all pipelines) are checked, and those nodes that 1. are generic, and 2. do not specify a value of their own have their value for that property replaced with the value of the global property.

This PR is working off the assumption that all global properties are grouped in the primary pipeline's app_data/properties stanza as shown below. Once we are sure of the final structure of the pipeline JSON sent from the frontend, changes can be made here as required.

"app_data":{
  "name":"read_and_write",
  "source":"read_and_write.pipeline",
  "properties":{
    "name":"read_and_write",
    "runtime":"Generic",
    "pipeline_defaults":{
      "runtime_image":"ex",
      "env_vars":[
        "val1=1",
        "val2=2"
      ]
    }
  },

This PR also adds a new API endpoint to serve the pipeline properties JSON (previously hardcoded on the frontend here). The new endpoint is elyra/pipeline/components/{runtime_type}/properties.

How was this pull request tested?

Add tests for global property propagation
Add handler test for new pipeline properties endpoint

Fixes #2268

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 · 2022-04-11T20:34:03Z

Thanks for making a pull request to Elyra!

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

akchinSTC · 2022-04-11T20:39:16Z

Questions:

Will we ever need to support sub-pipeline/supernode "global" variables? - Deferred until the need arises
When a property contains a list or some other "container" type, how should the values be resolved ? - Deferred, working on runtime_image first.

elyra/pipeline/pipeline_definition.py

elyra/pipeline/handlers.py

lresende · 2022-04-12T03:11:30Z

elyra/pipeline/pipeline_definition.py

@@ -432,6 +436,23 @@ def validate(self) -> list:

        return validation_issues

+    def propagate_global_properties(self):


Is there any difference between "propagating" vs "if not local, read global value"?

The approach using propagation isolates the changes to one location, otherwise, you'd have to know the locations where these kinds of properties are used (and probably all the way down to actual submission). I think validation would be easier as well and it minimizes the risk of errors.

In a side-discussion, you mentioned a use-case where we might need to know whether a given value was inherited from the globals or not. If that need were to arise, one could compare the node's value to the global's value and, if the same, assume inherited, otherwise not. The only case that doesn't solve is when the node redundantly defines a property and it's technically not inherited, but that strikes me as fairly "edgy".

ajbozarth · 2022-04-12T16:18:26Z

After discussion in scrum I have pushed the front end changes to this PR and updated the title.

The front-end currently points to the code sandbox for elyra-ai/pipeline-editor#190 and only supports the initially runtime images global property (or more specifically it supports global enum properties)

ptitzler · 2022-04-12T16:43:58Z

After some quick testing of the new UI for the global runtime image property, I believe we should change the default entry that's shown in the node properties from 'select' to the global setting, if one was specified. The reason is that the current behavior does not reduce the number of steps the user needs to complete to configure generic components. For example:

3.7 workflow:
- user adds 20 nodes
- user must select a runtime image for 20 nodes
- ... other config steps unrelated to runtime images
current PR workflow:
- user configures global runtime image
- user adds 20 nodes
- user selects 'global' runtime image for 20 nodes
- ... other config steps unrelated to runtime images
proposed PR workflow:
- user configures global runtime image
- user adds 20 nodes
- ... other config steps unrelated to runtime images

(does not add actual support for those env vars)

ajbozarth · 2022-04-12T16:49:16Z

user selects 'global' runtime image for 20 nodes

This step isn't necessary, the global value is used by default. The proposed workflow IS the current workflow

elyra/templates/pipeline/pipeline_properties_template.jinja2

ajbozarth · 2022-04-12T16:52:45Z

FYI I added env_vars to the global properties definition (palette). This allows users to add global env_vars, but until @akchinSTC or @kiersten-stokes add the backend support they won't actually be used yet.

AFAIK since we plan to "simply merge" list/array global properties with node properties there is no further front end work required since the backend will handle that. So unless we want to visualize any global env vars to the user in the node properties, the front end work is "done"

ptitzler · 2022-04-12T16:53:34Z

user selects 'global' runtime image for 20 nodes

This step isn't necessary, the global value is used by default. The proposed workflow IS the current workflow

How would the user know?

Edit: is you are saying that the pre-selected local entry is ... (global) then I encountered a bug because it's set to select ....

ajbozarth · 2022-04-12T16:57:25Z

How would the user know?

The global property is "shown" in the dropdown for any node's property that isn't set it if the global property is set. I'm not sure how else we would surface it to the user. Maybe if they don't understand globals they might open the node properties for each node and check it themselves, but from a UX perspective they don't have to do anything.

ajbozarth · 2022-04-12T16:58:40Z

Edit: is you are saying that the pre-selected local entry is ... (global) then I encountered a bug because it's set to select ...

Ah that would be a bug then. If the global is set then the global should be shown there. (either way it would still work, this would just be a UI bug)

rather than docker images specifically

ptitzler · 2022-04-12T17:10:40Z

Edit: is you are saying that the pre-selected local entry is ... (global) then I encountered a bug because it's set to select ...

Ah that would be a bug then. If the global is set then the global should be shown there. (either way it would still work, this would just be a UI bug)

I am no longer able to recreate my initially observed behavior. Will try provide steps if I encounter this again. I did confirm that run/export appear to work as expected (including validation).

ptitzler · 2022-04-12T17:27:29Z

Perhaps we should use (pipeline default) instead of (global) since it's more descriptive?

elyra/templates/pipeline/pipeline_properties_template.jinja2

ptitzler

~~LGTM~~

ajbozarth · 2022-04-19T17:20:51Z

I fixed the bug with lists in the tooltip that @ptitzler showed in scrum.

Once this gets final approval I'll merge and release the upstream pr and update here

kevin-bates · 2022-04-19T17:51:29Z

@ajbozarth - Was there previously an interim build that could be used to verify this PR and, if so, could the same thing happen so we can capture the single-character fix in elyra-ai/pipeline-editor@cf36bb0 and avoid having to wait for its full approval/merge/release cycle?

kevin-bates

This looks good. Just had the one question about the code scan alert. Since its medium severity, it's probably something we address later so I'm approving. Should we create an issue to track it?

EDIT: Forgot to add that I ran a multi-node pipeline through its paces relative to the defaults.

kevin-bates · 2022-04-18T22:17:47Z

elyra/pipeline/handlers.py

+
+        # Get pipeline properties json
+        jinja_loader = PackageLoader("elyra", "templates/pipeline")
+        template = Environment(loader=jinja_loader).get_template("pipeline_properties_template.jinja2")


Are there plans to address this in this PR?

akchinSTC · 2022-04-19T21:15:48Z

This looks good. Just had the one question about the code scan alert. Since its medium severity, it's probably something we address later so I'm approving. Should we create an issue to track it?

EDIT: Forgot to add that I ran a multi-node pipeline through its paces relative to the defaults.

Im going to leave this in for now or suppress it. The safe filter in jinja doesn't seem to satisfy the requirements of the checker. Turning on the escape feature is a no-go since it will essentially corrupt the template with encoded characters.

ptitzler

LGTM!

Add support for global properties as introduced in elyra-ai/elyra#2656

Propagate global variables in PipelineDefinition

29c3065

kiersten-stokes added this to the 3.8.0 milestone Apr 11, 2022

kevin-bates reviewed Apr 11, 2022

View reviewed changes

elyra/pipeline/pipeline_definition.py Outdated Show resolved Hide resolved

akchinSTC and others added 2 commits April 11, 2022 13:47

Move global retrieval into separate def

e9bd09c

Conditionally propagate global properties

7f85aec

kevin-bates reviewed Apr 11, 2022

View reviewed changes

elyra/pipeline/pipeline_definition.py Outdated Show resolved Hide resolved

Add new API endpoint for pipeline properties

23a98ab

lresende reviewed Apr 12, 2022

View reviewed changes

elyra/pipeline/handlers.py Show resolved Hide resolved

lresende reviewed Apr 12, 2022

View reviewed changes

ajbozarth mentioned this pull request Apr 12, 2022

Support global properties elyra-ai/pipeline-editor#190

Merged

3 tasks

ajbozarth and others added 2 commits April 12, 2022 11:13

Add front end

f4cbb82

Update api definitions

5744e98

ajbozarth changed the title ~~Add backend support for global pipeline properties~~ Add support for global pipeline properties Apr 12, 2022

ajbozarth assigned akchinSTC, ajbozarth and kiersten-stokes Apr 12, 2022

Add global_envs prop to palette

647abe1

(does not add actual support for those env vars)

ptitzler reviewed Apr 12, 2022

View reviewed changes

elyra/templates/pipeline/pipeline_properties_template.jinja2 Outdated Show resolved Hide resolved

update reference to runtime images as containers

4b2c82d

rather than docker images specifically

ajbozarth reviewed Apr 16, 2022

View reviewed changes

elyra/templates/pipeline/pipeline_properties_template.jinja2 Outdated Show resolved Hide resolved

akchinSTC and others added 3 commits April 18, 2022 10:33

Add some error checking for env vars

a71775a

Refactor pipeline def tests to remove propagate_properties var

5081e32

Simplify mocked pytest fixture

24ce015

kiersten-stokes mentioned this pull request Apr 18, 2022

Add support for storing pipeline parameters and COS prefixes #2669

Merged

akchinSTC requested review from kevin-bates, akchinSTC, ptitzler, ajbozarth and lresende April 18, 2022 21:34

kevin-bates added 3 commits April 18, 2022 16:24

Capture python versions after lint and build server steps

3338ee6

Fix tab in Makefile

d85c2d1

Revert temporary capture of versions

0077ddd

kevin-bates mentioned this pull request Apr 19, 2022

Test-dependencies side-affect linting in server development environments #2670

Closed

ptitzler approved these changes Apr 19, 2022

View reviewed changes

ptitzler self-requested a review April 19, 2022 15:57

kevin-bates mentioned this pull request Apr 19, 2022

Remove hacking dependency, capture version snapshot prior to CI testing #2671

Merged

fix bug in upstream pr

1969cf3

kevin-bates approved these changes Apr 19, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

fix issues

25ee90b

ptitzler approved these changes Apr 20, 2022

View reviewed changes

ajbozarth added a commit to elyra-ai/pipeline-editor that referenced this pull request Apr 20, 2022

Support global properties (#190)

0e54723

Add support for global properties as introduced in elyra-ai/elyra#2656

akchinSTC approved these changes Apr 20, 2022

View reviewed changes

ajbozarth added 2 commits April 20, 2022 12:29

use rc release of pipeline-editor

052cf26

Merge branch 'master' into global-env-vars

159930c

akchinSTC merged commit f5260db into elyra-ai:master Apr 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for runtime image and env vars in pipeline default properties #2656

Add support for runtime image and env vars in pipeline default properties #2656

kiersten-stokes commented Apr 11, 2022 •

edited

Loading

elyra-bot bot commented Apr 11, 2022

akchinSTC commented Apr 11, 2022 •

edited

Loading

lresende Apr 12, 2022

kevin-bates Apr 12, 2022

ajbozarth commented Apr 12, 2022

ptitzler commented Apr 12, 2022 •

edited

Loading

ajbozarth commented Apr 12, 2022 •

edited

Loading

ajbozarth commented Apr 12, 2022

ptitzler commented Apr 12, 2022 •

edited

Loading

ajbozarth commented Apr 12, 2022

ajbozarth commented Apr 12, 2022

ptitzler commented Apr 12, 2022

ptitzler commented Apr 12, 2022

ptitzler left a comment •

edited

Loading

ajbozarth commented Apr 19, 2022

kevin-bates commented Apr 19, 2022

kevin-bates left a comment •

edited

Loading

kevin-bates Apr 18, 2022

akchinSTC commented Apr 19, 2022

This comment was marked as resolved.

ptitzler left a comment

		@@ -432,6 +436,23 @@ def validate(self) -> list:

		return validation_issues

		def propagate_global_properties(self):

Add support for runtime image and env vars in pipeline default properties #2656

Add support for runtime image and env vars in pipeline default properties #2656

Conversation

kiersten-stokes commented Apr 11, 2022 • edited Loading

What changes were proposed in this pull request?

How was this pull request tested?

elyra-bot bot commented Apr 11, 2022

akchinSTC commented Apr 11, 2022 • edited Loading

lresende Apr 12, 2022

Choose a reason for hiding this comment

kevin-bates Apr 12, 2022

Choose a reason for hiding this comment

ajbozarth commented Apr 12, 2022

ptitzler commented Apr 12, 2022 • edited Loading

ajbozarth commented Apr 12, 2022 • edited Loading

ajbozarth commented Apr 12, 2022

ptitzler commented Apr 12, 2022 • edited Loading

ajbozarth commented Apr 12, 2022

ajbozarth commented Apr 12, 2022

ptitzler commented Apr 12, 2022

ptitzler commented Apr 12, 2022

ptitzler left a comment • edited Loading

Choose a reason for hiding this comment

ajbozarth commented Apr 19, 2022

kevin-bates commented Apr 19, 2022

kevin-bates left a comment • edited Loading

Choose a reason for hiding this comment

kevin-bates Apr 18, 2022

Choose a reason for hiding this comment

akchinSTC commented Apr 19, 2022

This comment was marked as resolved.

ptitzler left a comment

Choose a reason for hiding this comment

kiersten-stokes commented Apr 11, 2022 •

edited

Loading

akchinSTC commented Apr 11, 2022 •

edited

Loading

ptitzler commented Apr 12, 2022 •

edited

Loading

ajbozarth commented Apr 12, 2022 •

edited

Loading

ptitzler commented Apr 12, 2022 •

edited

Loading

ptitzler left a comment •

edited

Loading

kevin-bates left a comment •

edited

Loading