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

Refactor pipeline implementation to use common pattern for file naming #3160

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fresende
Copy link
Contributor

What changes were proposed in this pull request?

Refactor pipeline implementation to use common pattern for file naming

How was this pull request tested?

By existing pyTests

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.

@fresende fresende force-pushed the refactor branch 2 times, most recently from e514029 to d28b77e Compare June 15, 2023 01:26
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

These name changes seem a bit arbitrary given there was already a "common pattern" but I also prefer the adjective to precede the noun so have no immediate issues.

That said, this will break anyone that has written code against these modules, either via subclasses or otherwise, so is this slated for a major release? Should "deprecation shims" be introduced that essentially alias the old names to the new names?

elyra/pipeline/component.py Outdated Show resolved Hide resolved
@shalberd
Copy link
Contributor

@kevin-bates agreed, it is a very major change. Definitely in favor of deprecation shims.

@lresende
Copy link
Member

I was the one suggesting @fresende to work on these updates.

@kevin-bates I was under the impression that we were moving towards Elyra 4.0 where breaking changes were welcomed.

Also, I would say this would have very little impact on users, as these are internal APIs and mostly only used by folks that are extending Elyra with new processors. @shalberd is this your case?

@kevin-bates
Copy link
Member

I was under the impression that we were moving towards Elyra 4.0 where breaking changes were welcomed.

Ok. That should be noted somewhere in this PR. I figured this was just another PR. If a 4.0 release is near, folks should be informed that this kind of breaking change is coming so they can, for now, cap Elyra < 4.

This just seems like an itch that wanted to be scratched and nothing more. Yes, the names flow a bit better but is it worth the heartburn? The PR stats using a "common pattern", but there was already a common pattern in place, just with the adjectives following the nouns rather than the other way.

I would say this would have very little impact on users, as these are internal APIs and mostly only used by folks that are extending Elyra with new processors.

True, although I think is is more of an issue with anyone subclassing existing processors. Folks creating their own (independent) processors should not run into issues.

I'd love to hear @shalberd's thoughts as well.

Since I really don't have the bandwidth to spend time on this project, I think making a best effort at communicating that the next major release will break folks extending the existing processor packages is sufficient, and you get your itch scratched at the same time. 😄

@shalberd
Copy link
Contributor

I am on vacation so briefly put, I also think it is more of an issue with a new processor subclassing existing ones. For Airflow 2.0 replacing long gone Airflow 1, how shall I proceed? We have the guidelines and hints for Elyra 4 aleady?

@lresende lresende added impact:breaking change Delivery introduces a change that is not backward compatible component:pipeline-runtime issues related to pipeline runtimes e.g. kubeflow pipelines platform: pipeline-Kubeflow Related to usage of Kubeflow Pipelines as pipeline runtime platform: pipeline-Airflow Related to usage of Apache Airflow as pipeline runtime platform: pipeline-local Related to usage of the local environment as pipeline runtime labels Jul 22, 2023
@lresende
Copy link
Member

I am on vacation so briefly put, I also think it is more of an issue with a new processor subclassing existing ones. For Airflow 2.0 replacing long gone Airflow 1, how shall I proceed? We have the guidelines and hints for Elyra 4 aleady?

Some discussions in #2550

@lresende
Copy link
Member

@fresende Could you please update/rebase this one?

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 impact:breaking change Delivery introduces a change that is not backward compatible platform: pipeline-Airflow Related to usage of Apache Airflow as pipeline runtime platform: pipeline-Kubeflow Related to usage of Kubeflow Pipelines as pipeline runtime platform: pipeline-local Related to usage of the local environment as pipeline runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants