elyra-ai / elyra

Elyra extends JupyterLab with an AI centric approach.
https://elyra.readthedocs.io/en/stable/
Apache License 2.0
1.86k stars 344 forks source link

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

Open fresende opened 1 year ago

fresende commented 1 year ago

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.
shalberd commented 1 year ago

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

lresende commented 1 year ago

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 commented 1 year ago

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. :smile:

shalberd commented 1 year ago

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 commented 1 year ago

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 https://github.com/elyra-ai/elyra/discussions/2550

lresende commented 10 months ago

@fresende Could you please update/rebase this one?