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

Tolerate runtime configuration instances named 'local' #2968

Merged
merged 2 commits into from Oct 18, 2022

Conversation

kevin-bates
Copy link
Member

@kevin-bates kevin-bates commented Oct 12, 2022

When submitting pipelines to Kubeflow or Airflow runtimes, Elyra runs the pipeline locally if the corresponding runtime configuration is named "local" as described in #2964. This is because the server looks at the runtime_config field in the pipeline for either no value (None) or the name "local". This pull request implements changes such that only invocations for local processing will submit pipelines with no value for runtime_config.

Should we decide to promote the LOCAL runtime to a full-fledged runtime (where there's an associated schema and support for runtime configuration instances), most of these changes should still work since there will be a value for runtime_config in these cases, which will then determine the target pipeline processor from the referenced schema name within the runtime configuration instance. We should only then need to make changes where we set the runtime_config to None during submission (and in both the UI and CLI applications).

Still to do:

  • Set runtime_config to null (or None) in the UI when submitting pipelines for local execution

Resolves: #2964

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

elyra-bot bot commented Oct 12, 2022

Thanks for making a pull request to Elyra!

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

@kevin-bates kevin-bates marked this pull request as draft October 12, 2022 19:06
@lresende
Copy link
Member

@kevin-bates could you add to the description how the info will be submitted (or expected) when local and non-local? (e.g. runtime-config and runtime-schema)

@kevin-bates
Copy link
Member Author

@kevin-bates could you add to the description how the info will be submitted (or expected) when local and non-local? (e.g. runtime-config and runtime-schema)

From the description...

This pull request implements changes such that only invocations for local processing will submit pipelines with no value for runtime_config.

The schema for the runtime (and thus which processor to use) comes from the runtime configuration's instance referenced by runtime_config.

@karlaspuldaro
Copy link
Member

@kevin-bates I believe this is where you would set the runtime_config value
https://github.com/elyra-ai/elyra/blob/main/packages/pipeline-editor/src/PipelineEditorWidget.tsx#L762

@kevin-bates
Copy link
Member Author

Thanks @karlaspuldaro - that's the very location I had identified as well, so it's great reassurance!

@kevin-bates kevin-bates marked this pull request as ready for review October 12, 2022 22:22
@ptitzler ptitzler added this to the 3.13.0 milestone Oct 13, 2022
@ptitzler ptitzler added kind:bug Something isn't working component:pipeline-runtime issues related to pipeline runtimes e.g. kubeflow pipelines labels Oct 13, 2022
Copy link
Member

@karlaspuldaro karlaspuldaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me :)

@ptitzler ptitzler self-requested a review October 13, 2022 17:44
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 based on spot testing

@lresende
Copy link
Member

Please give me till tomorrow to review it, thanks.

@lresende
Copy link
Member

All the confusion described in #2964 seems to be related to the UI only propagating the runtime_config which is the key to finding the runtime configuration profile, and when an existing runtime tries to use it as local it becomes problematic.

How about, instead of using workarounds, we don't actually also propagate the runtime type, which is how the ui finds all the runtime configuration available for each runtime type thus we just need to make sure the backend uses both runtime type and runtime config to find the proper processor to execute the pipeline?

NOT A CONTRIBUTION

@kevin-bates
Copy link
Member Author

Thanks for your review @lresende.

Adding runtime_type to the API will certainly work, but I disagree that it's any better. Aside from an API change, this will only add to the confusion since there will be two sources for determining the type of runtime. What's the point of having runtime_type as part of the runtime schema in the first place?

The "workaround" is due to the fact that we use a "magic config name" named 'local' to indicate that the pipeline is run locally. The source code was already using a "None" or "local" approach when checking runtime_config and this pull request simplifies that further to only allow None for that specification. This nicely positions us to introduce true runtime configuration for local processing - which is the previous decision (to not do that) we're constantly hacking our way around. If local runtimes are ever introduced, this PR will require zero changes since, then, the runtime name will be passed and the code will simply bypass the checks against None and use whatever runtime is specified in the runtime_config schema - just like all non-local runtimes work today. In addition, non-local runtimes can choose any name they want, including "local".

@akchinSTC akchinSTC merged commit 881c1f2 into elyra-ai:main Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:pipeline-runtime issues related to pipeline runtimes e.g. kubeflow pipelines kind:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime configuration named 'local' is treated as local runtime
5 participants