Skip to content
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

Merged
merged 89 commits into from
Nov 10, 2021
Merged

Conversation

kevin-bates
Copy link
Member

@kevin-bates kevin-bates commented Oct 31, 2021

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 and airflow 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:

  • Reflect the runtime type in the pipeline JSON file as a sibling to runtime in app_data. E.g. app_data.runtime_type.
  • Equate the runtime platform icons to the runtime-type value rather than the runtime-name.
    • In the Launcher
    • In the Components list on the left-hand pane of the pipeline editor
    • Probably other locations as well
  • Address the TODOs in the validation code. This requires that runtime_type be reflected in the pipeline JSON
  • Update tests as needed
  • The metadata editor needs to handle constants (const) 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

   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.

kiersten-stokes and others added 30 commits October 18, 2021 14:50
lresende and others added 4 commits November 8, 2021 09:32
…#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
Copy link
Member

@ajbozarth ajbozarth left a 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

Copy link
Member

@kiersten-stokes kiersten-stokes left a 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.

Copy link
Member

@kiersten-stokes kiersten-stokes left a 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 to Run 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 is processor_name:
processor = PipelineProcessorRegistry.instance().get_processor(processor_type=processor_type)

@kevin-bates
Copy link
Member Author

Good catches - thanks. The Generic/Local stuff is a nightmare.

Copy link
Member

@kiersten-stokes kiersten-stokes left a 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

Copy link
Member

@akchinSTC akchinSTC left a 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

Copy link
Member

@ptitzler ptitzler left a 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

    image

  • 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
Copy link
Member

  • 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.

image

@kevin-bates
Copy link
Member Author

@akchinSTC @ptitzler - my last commit should address the instances where the schema's display_name/title was used instead of the type.

@akchinSTC akchinSTC merged commit 25bdb73 into elyra-ai:master Nov 10, 2021
@kevin-bates kevin-bates deleted the runtime-types branch November 17, 2021 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] Introduce the notion of Runtime Processor Type
6 participants