-
Notifications
You must be signed in to change notification settings - Fork 349
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 indication of runtime into operation's environment #1668
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
Note: we need a better story for bringing your own schema than placing the custom schema into elyra's installation area under |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, there is still a question to @akchinSTC but otherwise good to go.
Thanks @lresende. Also, note that I did not remove the previous class-hierarchy image that is now unused. Since it is incomplete, I think it might be best to remove it - assuming you can generate an updated copy if needed. If agreed, I'll submit a commit to remove it. |
docs/source/user_guide/pipelines.md
Outdated
@@ -104,7 +104,17 @@ elyra-pipeline submit elyra-pipelines/demo-heterogeneous.pipeline \ | |||
|
|||
The `runtime-config` should be a valid [runtime configuration](/user_guide/runtime-conf.md). | |||
|
|||
#### Detecting the runtime from a component |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to introduce the term component
because afaik we haven't used it prior to 2.3.x in the context of pipeline artifacts. This is also needed because the Pipelines documentation topic starts with "Elyra utilizes its canvas component ", which is semantically completely different and can lead to confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we reference "nodes" I'll use that. Also changing 'running' to 'executing'. Thanks.
@staticmethod | ||
def _collect_envs(operation: Operation) -> Dict: | ||
envs = os.environ.copy() # Make sure this process's env is "available" in the kernel subprocess | ||
envs['ELYRA_RUNTIME_ENV'] = "local" # Special case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be safer to reference the _type
class variable
elyra/elyra/pipeline/processor_local.py
Line 42 in f17143b
_type = 'local' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_type
is not on an OperationProcessor
- which is where "local" runtimes perform env collection. I could pass this as a parameter, but it (at least today) will never be used outside of a local "runtime". Had we treated "local" as a full-fledged runtime (eh um), then we'd be able to use "type" - as I do in RuntimePipelineProcess._collect_envs()
.
User test for kfp/run, kfp/export/yaml, kfp/export/dsl, local/run, airflow/export, airflow/run yielded the expected results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request adds an indication of the current runtime into the operation's environment via the variable
ELYRA_RUNTIME_ENV
. Although the discussion on the issue suggested this value be obtained from the runtime configuration schema, the value is actually a function of the processor implementation corresponding to the runtime and reflected in the abstracttype
property. I believe we should document that the value ofPipelineProcessor.type
should match the schema name of the runtime (as is the case so far with the exception of the special case "local runtime") and is the suggested value for the name of theentrypoint
under which the processor implementation is registered. This way the three entities (runtime schema, pipeline processor implementation and correspondingentrypoint
entry) are logically associated.The current set of valid values are:
These changes also consolidate the collection of envs which helps with testing.
type
property) and document the "built-in" values whichELYRA_RUNTIME_ENV
can have.How was this pull request tested?
The approaches for testing the 'local' case are different than the 'kfp' and 'airflow' cases.
For the 'local' test, the generated pipeline files were amended to assert the presence of the environment variable (
ELYRA_RUNTIME_ENV
) has a value of 'local' during the actual execution of the generated pipeline.For the 'kfp' and 'airflow' scenarios, a
test_collect_envs()
test was added to each set of tests where the new_collect_envs()
method was called on the respective processor, followed by appropriate assertions for the presence of other runtime-specific environment variables.Resolves #1663
Developer's Certificate of Origin 1.1