Improve prek hook to check for proper imports in shared distributions #58825

potiuk · 2025-11-28T17:43:32Z

Since we are quite close to having a completed separation between
distributions, this PR will be rebased until we will fix all the
issues - we might want to extract things from this PR as we
rebase it and move the code between distributions.

The following checks were added:

The shared distributions (neither in src nor tests) should never
import from airflow.
The distributions that are using shared code "airlfow-core",
"task-sdk" should never import anything from either
"airflow_shared" or from "_shared" import coming from another
distribution.
Neither "apache-airflow", "apache-airflow-task-sdk", "apache-airflow-core"
should be added as dependency in a shared distribution.
The "devel-common" should also not refer to airflow.* - it should only
import from airflow_shared.*

^ 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 airflow-core/newsfragments.

jscheffl

Cool!

potiuk · 2025-11-28T23:14:33Z

Small thing - this one should fail. I restarted it with full-tests-needed.

potiuk · 2025-11-28T23:20:31Z

OK. Figured out why it did not run - added "scripts/ci/prek" to also trigger full tests.

potiuk · 2025-11-29T13:03:57Z

@amoghrajesh @xBis7 - I solved the "module_loading" import issue (I created a new shared distribution "import_utils") and added even more checks in the prek hook. It nicely shows now all the remaining issues that we have to solve (basically settings that already had some TODOs and issues).

I will keep on extracting things from this PR to separate PRs (for example extraction of import_utils can be done as a separate PR) - and I think that might be a good idea for other changes - to rebase them on top of this one, because this one immediately shows now violations of imports that we still have :)

potiuk · 2025-11-29T13:06:00Z

And IMMEDIATELY we got benefit of the common.compat # use next version detection @jscheffl @vincbeck !!!!!!

shared/module_loading/src/airflow_shared/module_loading/__init__.py

potiuk · 2025-12-01T21:18:45Z

@amoghrajesh @xBis7 I rebased my change and added some changes:

the "import_utils.module_loading" module is no longer "import_utils" - it's just module_loading - I modified the code quite a bit to move the shared methods to __int__.py rather than have a "import_utls.module_loading` - i think that can largely alleviate those concerns @amoghrajesh had about too many "utils" - I think it's really nice one and simpler than before.
I also realised we need one more check (and turned out we have quite a bit of work to do still). The devel-common module needs a bit of polish, refactor - and I think we will have to add all the_shared modules as dependencies to it. Simply devel_common should NEVER import anything from airflow. nor airflow.sdk - because it can be use in either of the two (and other disstributions) - so it should import everything from airflow_shared.* and have those packages it imports from as dependencies.

amoghrajesh · 2025-12-02T06:56:18Z

I also realised we need one more check (and turned out we have quite a bit of work to do still). The devel-common module needs a bit of polish, refactor - and I think we will have to add all the_shared modules as dependencies to it. Simply devel_common should NEVER import anything from airflow. nor airflow.sdk - because it can be use in either of the two (and other disstributions) - so it should import everything from airflow_shared.* and have those packages it imports from as dependencies.

Good find, yes that will likely be needed. Let is be independent as and when possible

amoghrajesh

Some observations and comments, looks good otherwise.

airflow-core/docs/authoring-and-scheduling/serializers.rst

amoghrajesh · 2025-12-02T06:59:18Z

airflow-core/src/airflow/serialization/serde.py

 import attr

 import airflow.serialization.serializers
+from airflow._shared.module_loading import import_string, iter_namespace, qualname


Note to self: revisit this occurence as part of Move over serde library to task sdk

amoghrajesh · 2025-12-02T07:00:00Z

airflow-core/src/airflow/settings.py

    core_masker.sensitive_variables_fields = list(sensitive_fields)
    core_masker.secret_mask_adapter = secret_mask_adapter

+    # TODO: this should be moved out when settings are moved to `shared`


I dont think I follow this one

airflow-core/tests/unit/always/test_project_structure.py

providers/apache/kafka/src/airflow/providers/apache/kafka/hooks/consume.py

scripts/ci/prek/check_shared_distributions_structure.py

shared/configuration/src/airflow_shared/configuration/parser.py

amoghrajesh · 2025-12-02T07:02:56Z

shared/module_loading/src/airflow_shared/module_loading/__init__.py

-
    Raise ImportError if the import failed.
    """
+    # TODO: Add support for nested classes. Currently, it only works for top-level classes.


Do we need to? I dont see a use case for it

Nope. I just copied it from the original code.

potiuk · 2025-12-06T17:14:59Z

Rebased it after observability was added :)

Shared packages should not refer to airflow or airflow.sdk. This is the last "airflow" import remaining in the observability package. Fixed during implementation of apache#58825

Shared packages should not refer to airflow or airflow.sdk. This is the last "airflow" import remaining in the observability package. Fixed during implementation of #58825

Shared packages should not refer to airflow or airflow.sdk. This is the last "airflow" import remaining in the observability package. Fixed during implementation of apache#58825

Since we are quite close to having a completed separation between distributions, this PR will be rebased until we will fix all the issues - we might want to extract things from this PR as we rebase it and move the code between distributions. The following checks were added: * The shared distributions (neither in src nor tests) should never import `from airflow.` * The distributions that are using shared code "airlfow-core", "task-sdk" should never import anything from either "airflow_shared" or from "_shared" import coming from another distribution. * Neither "apache-airflow", "apache-airflow-task-sdk", "apache-airflow-core" should be added as dependency in a shared distribution. * The "devel-common" should also not refer to airflow.* - it should only import from airflow_shared.*

potiuk requested review from amoghrajesh, ashb, bugraoz93, gopidesupavan, jason810496, jscheffl and kaxil as code owners November 28, 2025 17:43

boring-cyborg bot added area:dev-tools backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch labels Nov 28, 2025

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch 2 times, most recently from ee1c3ca to 915a85f Compare November 28, 2025 18:09

jscheffl approved these changes Nov 28, 2025

View reviewed changes

potiuk added the full tests needed We need to run full set of tests for this PR to merge label Nov 28, 2025

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch from 915a85f to 90412d0 Compare November 28, 2025 23:14

potiuk removed the full tests needed We need to run full set of tests for this PR to merge label Nov 28, 2025

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch from 90412d0 to 7e2ba2e Compare November 28, 2025 23:20

potiuk requested review from ephraimbuddy and jedcunningham as code owners November 28, 2025 23:20

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch 2 times, most recently from 0b0881d to b537ffe Compare November 29, 2025 12:57

potiuk requested review from XD-DENG, bolkedebruin, dstandish, hussein-awala, mobuchowski, o-nikolas and pierrejeambrun as code owners November 29, 2025 12:57

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch from b537ffe to 79426a7 Compare November 29, 2025 13:01

potiuk changed the title ~~Add prek hook to check for "airflow" imports in shared distributions~~ Improve prek hook to check for proper imports in shared distributions Nov 29, 2025

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch 2 times, most recently from b9d66dc to 8de0d2a Compare November 29, 2025 13:53

amoghrajesh reviewed Nov 29, 2025

View reviewed changes

shared/module_loading/src/airflow_shared/module_loading/__init__.py Show resolved Hide resolved

amoghrajesh mentioned this pull request Nov 29, 2025

Unclog shared conf library tests from importing from airflow core #58851

Merged

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch 4 times, most recently from 17fa532 to e52d2e0 Compare December 1, 2025 21:13

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch from e52d2e0 to 5b99a82 Compare December 1, 2025 22:17

amoghrajesh approved these changes Dec 2, 2025

View reviewed changes

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch from 5b99a82 to 2e8b15b Compare December 6, 2025 17:14

potiuk mentioned this pull request Dec 6, 2025

Extract shared "module_loading" distribution #59139

Merged

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch from 2e8b15b to ccd119e Compare December 6, 2025 17:28

This was referenced Dec 6, 2025

Copy Self to observability shared package #59140

Merged

Remove imports of airflow and airflow.sdk from shared secrets_backend #59141

Open

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch 2 times, most recently from a5b108f to 4752c49 Compare December 8, 2025 21:06

potiuk mentioned this pull request Dec 18, 2025

Remove core references in secrets backend logic in sdk #59597

Merged

potiuk force-pushed the add-prek-hook-for-airflow-imports-in-shared branch from 4752c49 to b34e1b0 Compare December 20, 2025 14:20

Improve prek hook to check for proper imports in shared distributions #58825

Are you sure you want to change the base?

Improve prek hook to check for proper imports in shared distributions #58825

Conversation

potiuk commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jscheffl left a comment

Choose a reason for hiding this comment

Uh oh!

potiuk commented Nov 28, 2025

Uh oh!

potiuk commented Nov 28, 2025

Uh oh!

potiuk commented Nov 29, 2025

Uh oh!

potiuk commented Nov 29, 2025

Uh oh!

Uh oh!

potiuk commented Dec 1, 2025

Uh oh!

amoghrajesh commented Dec 2, 2025

Uh oh!

amoghrajesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amoghrajesh Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

amoghrajesh Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amoghrajesh Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

potiuk Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

potiuk commented Dec 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

potiuk commented Nov 28, 2025 •

edited

Loading