Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Feb 16, 2025


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk potiuk force-pushed the avoid-imports-from-provider branch from ad97e84 to ce66399 Compare February 16, 2025 12:37
@potiuk potiuk force-pushed the avoid-imports-from-provider branch from ce66399 to bad077d Compare February 16, 2025 12:44
@kxepal
Copy link
Member

kxepal commented Feb 16, 2025

Sorry, but you start to use some weird tests_common package in your imports which is not well defined as test dependency or else. You better to define it as some subpackadge of airflow package or create airflow.testing package - the best idea to share common test utils, or to use pytest fixtures, but never ever import something from tests into tests.

@potiuk potiuk force-pushed the avoid-imports-from-provider branch from bad077d to 39ff875 Compare February 16, 2025 12:49
@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2025

Sorry, but you start to use some weird tests_common package in your imports which is not well defined as test dependency or else. You better to define it as some subpackadge of airflow package or create airflow.testing package - the best idea to share common test utils, or to use pytest fixtures, but never ever import something from tests into tests.

No. We do not "start" using it - we already use it for a long time. And the plan is that tests_common will be turned as dependent package in pyproject.toml files of the providers, so it won't be "weird" this will be a shared package in our workspace.

@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2025

BTW. @jscheffl => I had to move the load_* files to tests_common, because we used "microsoft" provider test code from "hive" one. And the standard way to handle that is to move the code to "tests_common".

As explained to @kxepal -> one of the next steps in the cleanup will be to make "tests_common" a shared distribution that will be used in our workspace by all providers and airflow code. I am going to do it next as apparently it causes confusion now.

@kxepal
Copy link
Member

kxepal commented Feb 16, 2025

Sorry, but you start to use some weird tests_common package in your imports which is not well defined as test dependency or else. You better to define it as some subpackadge of airflow package or create airflow.testing package - the best idea to share common test utils, or to use pytest fixtures, but never ever import something from tests into tests.

No. We do not "start" using it - we already use it for a long time. And the plan is that tests_common will be turned as dependent package in pyproject.toml files of the providers, so it won't be "weird" this will be a shared package in our workspace.

Ок, sorry for that. Still, better to extract it into separate project with a better name to avoid any confusion (:
I would like to use some test fixtures from airflow in my test cases, but have to copy-paste them just because of lacking of package to depend on it.

@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2025

Ок, sorry for that. Still, better to extract it into separate project with a better name to avoid any confusion (:
I would like to use some test fixtures from airflow in my test cases, but have to copy-paste them just because of lacking of package to depend on it.

See the email which I just sent to devlist - explaining what is the next step in cleaning up our structure: https://lists.apache.org/thread/jonodvjnycv01qcsnlqczpvrjhfsx04g - chime in if needed.

@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2025

I would like to use some test fixtures from airflow in my test cases, but have to copy-paste them just because of lacking of package to depend on it.

BTW. It's extremely unlikely we are going to release and publish those fixtures in any way. They will continue to be in development-only-distribution and they will be treated as "internal detail".

If we decide to release and publish them, we will have to maintain backwards compatibility and account for our users (like you) using them for their own purpose. That would block us or make it very difficult to make breaking changes in them.

So while you will be free to continue copying the whole distribution and use it in your tests as you want (our licence allows that) - I seriously doubt we will ever release and publish it in "reusable form" with "compatibility guarantees". It's far more efficient if people like you just copy them and are aware that they can change any time.

Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Still OK :-D

@kxepal
Copy link
Member

kxepal commented Feb 16, 2025

@potiuk thank you, but no block is needed just because of me. I'm fine to adjust to the changes. But you raised an important topic about sharing test fixtures. Let's see how it goes. My current argument for this PR was just about wild import inside project tests - from my experience a better code separation and better dependencies could simplify development a lot, but I also understand that I could late for the party and some things could be already handled. But still simple and straight things always win (:

@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2025

@potiuk thank you, but no block is needed just because of me. I'm fine to adjust to the changes. But you raised an important topic about sharing test fixtures. Let's see how it goes. My current argument for this PR was just about wild import inside project tests - from my experience a better code separation and better dependencies could simplify development a lot, but I also understand that I could late for the party and some things could be already handled. But still simple and straight things always win (:

Yes. This whole project starting from separating 90 provider's projects 3 weeks ago, is just a beginning of making things more "standard"

if you look at the devlist discussions, we've always wanted to do it (first POC done more than 2 years ago) but we needed to wait until packaging standards and tooling catches up, otherwise people would have to manually manage multiple packages. This is now fully automateable with uv's workspaces and dependency groups. Basically single uv sync will install what is needed from all sub-projects that are needed.

This project is in a full swing now and I relentlessly, incrementally go step-by-step to get us to the world of "simple".

@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2025

And BTW - when we finish the packaging story, we will have a bit more than 100 distributions in Airflow. More separation, dependencies and splitting is coming. That discussion is yet ahead of us, but we already have a vision written down.

@potiuk potiuk force-pushed the avoid-imports-from-provider branch from 39ff875 to 80fe569 Compare February 16, 2025 13:50
@potiuk potiuk force-pushed the avoid-imports-from-provider branch from 80fe569 to 33f82b9 Compare February 16, 2025 14:29
@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2025

Finally green :). It required a few more "touches" :D

@potiuk potiuk merged commit 4e17ecd into apache:main Feb 16, 2025
89 checks passed
@potiuk potiuk deleted the avoid-imports-from-provider branch February 16, 2025 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants