-
Notifications
You must be signed in to change notification settings - Fork 350
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 support Runtime types #2263
Conversation
…kiersten-registry-updates
…/elyra into registry-updates
…#2274) Enables building the Elyra docker image (dev tag) from the current checked-out source code Fixes elyra-ai#2116
Enables the user to choose an authentication type for Kubeflow Runtime configurations Closes elyra-ai#2240 Closes elyra-ai#2107 Closes elyra-ai#2108
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.
Some initial review on the front end code, just some clarifying questions. I still need to check this out and test locally
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.
I found a very small missing piece during review. The backend code to create the processor response (here) sends the runtime type, whereas the response dialog on the frontend (created here) is checking for the runtime name. No error is thrown but the github repo url is missing in the dialog box when submitting a pipeline.
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.
A few more things I've found during testing:
- I get a
KeyError
for 'Generic' when I try toRun as Pipeline
on a notebook or script.This might not even be specific to this PR, I'll test it out.(it is specific to this PR). Output below:
Log Output
Traceback (most recent call last): File "/Users/kierstenstokes/repos/elyra/elyra/pipeline/pipeline_definition.py", line 119, in type RuntimeProcessorType.get_instance_by_name(runtime_type) File "/Users/kierstenstokes/repos/elyra/elyra/pipeline/runtime_type.py", line 41, in get_instance_by_name return RuntimeProcessorType.__members__[name] KeyError: 'Generic'During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/kierstenstokes/miniconda3/envs/dev1/lib/python3.9/site-packages/tornado/web.py", line 1704, in _execute
result = await result
File "/Users/kierstenstokes/repos/elyra/elyra/pipeline/handlers.py", line 110, in post
response = await PipelineValidationManager.instance().validate(pipeline=pipeline_definition)
File "/Users/kierstenstokes/repos/elyra/elyra/pipeline/validation.py", line 136, in validate
pipeline_type = pipeline_definition.primary_pipeline.type # generic, kfp, airflow
File "/Users/kierstenstokes/repos/elyra/elyra/pipeline/pipeline_definition.py", line 121, in type
raise ValueError(f'Unsupported pipeline runtime: {runtime_type}')
ValueError: Unsupported pipeline runtime: Generic
- Editing a component catalog does not trigger an update of the components right away. Looks like the below line is causing an exception because the
get_processor
arg isprocessor_name
:
processor = PipelineProcessorRegistry.instance().get_processor(processor_type=processor_type)
Good catches - thanks. The Generic/Local stuff is a nightmare. |
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! All my review comment issues have been addressed
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.
Tested as part of my changes with datax
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. Note the following front-end observations for the runtimes metadata UI
-
MUST FIX in a follow-up 3.3 PR (RUNTIME_TYPE should not be rendered OR at least not be an editable field) -> Add support for const types in metadata editor #2269
-
Should fix: there are several places in the UI (export pipeline, run notebook, run script, etc) that still display the user friendly runtime types (e.g. 'Apache Airflow'). This is likely an indication that the value is still hard coded and not the value that is assigned to the corresponding runtime type.) I would have expected to see KUBEFLOW_PIPELINES and APACHE_AIRFLOW throughout the UI until code is delivered that resolves those names properly.
|
@akchinSTC @ptitzler - my last commit should address the instances where the schema's display_name/title was used instead of the type. |
This pull request adds support for runtime (processor) types. Currently, only the name of a runtime processor is used to determine the platform type. This is insufficient when it comes to the Component Catalog changes introduced in #2241 (on which this PR depends) since today's
kfp
andairflow
names will not necessarily exist and may be supplanted by other implementations of the same runtime platform.The primary change is the introduction of a
RuntimeProcessorType
enumeration class (which we may want to convert to a custom class down the road - one that can deliver the necessary icons, for example). The enumerated names (i.e., constants) are then referenced (as constants) within the Runtimes schemas and their instances (this PR addresses the migration of existing instances).There are a number of changes still pending (mostly front-end related) - and thus the draft status:
runtime
inapp_data
. E.g.app_data.runtime_type
.runtime_type
be reflected in the pipeline JSONconst
) such that the value is pre-filled from the schema and the field is read-only. Or, just not displayed, since the service will seed the field to the constant value anyway. (Will be addressed in Add support for const types in metadata editor #2269)Fixes #2258 #2261
Developer's Certificate of Origin 1.1