-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Set up process for sharing code between different components. #53149
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
Set up process for sharing code between different components. #53149
Conversation
6fd5e04 to
e2376cd
Compare
4cb4080 to
fbd6e5f
Compare
Yes. We agreed I will write it as follow-up @uranusjr |
I think that should be ruff rule. Everything "in" task-sdk should use "_shared" or ("relative" if inside _shared already). For evryhing "using" "task.sdk" it should use the facade. The question is can we somehow forbid (or discourage) importing from `_shared" from outside ? |
+1 -> See the other comment from @amoghrajesh , but I think |
potiuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fantastic! It's great we could combine all the ideas and get something really nice and developer-friendly :) . There are few comments - most of them require more a of discussion, agreement and follow-up than changing the PR.
|
Cases like this in the providers: from airflow.utils.timezone import convert_to_utcI think I will leave as is for now, in this PR and do a single provider-only follow up PR that does the right thing (try sdk.timezone, except import utils.timezone) |
4b26389 to
80816e3
Compare
Yeah longer term (as in, not in this PR) I agree that is the right place, but we can't do that until we remove everything else form airflow.utils out of the airflow-core dist I don't think? |
80816e3 to
67308a0
Compare
We could potentially do the legacy namespace pathlib trick in both |
Even if temporarily - that would likely work nicely - we could remove it before release. |
|
(╯°□°)╯︵ ┻━┻ |
e0d811c to
bed3bd8
Compare
And move `airflow.utils.timezone` into a shared library as the first example of it working. In this change we have now setled on an approach using symlinks, but we did explore other options (see the GH PR for discussion and previous versions, notably one built upon the `vendoring` tool) A lot of the reasoning and mode of operation of this is detailed in shared/README.md in this PR, hence why this description is so short. Currently various places in TaskSDK and Airflow Core both use these utility functions, and while in this specific case they are small enough that they could just be copied and the duplication wouldn't hurt us long term, this changes shows a way in which we can have a single source of truth, but have it included automatically in built dists. Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
bed3bd8 to
636f97f
Compare
|
#protm Lot of good collaboration between various Airflow wizards and impact wise this one unlocks ability for client server separation. |
|
Let me get the pre-commit now :) |
And move
airflow.utils.timezoneinto a shared library as the first example of it working.In this change we have now setled on an approach using symlinks, but we did
explore other options (see the GH PR for discussion and previous versions,
notably one built upon the
vendoringtool)A lot of the reasoning and mode of operation of this is detailed in shared/README.md in this PR, hence why this description is so short.
Currently various places in TaskSDK and Airflow Core both use these utility functions, and while in this specific case they are small enough that they could just be copied and the duplication wouldn't hurt us long term, this changes shows a way in which we can have a single source of truth, but have it included automatically in built dists.
(For posterity, please see b6ae6a9 for the previous vendoring-based approach)
And thanks to Jarek for the help in making the symlink version work, it's much simpler!