Cope with Pycharm's oddity in detecting namespace packages by ashb · Pull Request #43116 · apache/airflow

ashb · 2024-10-17T13:01:51Z

Without the __path__ assignment being the very first line of the file
(including before any import) it doesn't detect this correctly.

Without the `__path__` assignment being the very first line of the file (including before any import) it doesn't detect this correctly.

ashb · 2024-10-17T13:02:11Z

@shahar1 @kaxil Could one of you test if this fixes it please?

kaxil · 2024-10-17T13:27:26Z

@shahar1 @kaxil Could one of you test if this fixes it please?

Yup, works now

kaxil · 2024-10-17T13:29:22Z

airflow/__init__.py

+# And it _has_ to be the first code line too. Sad panda
 __path__ = __import__("pkgutil").extend_path(__path__, __name__)  # type: ignore

+from __future__ import annotations  # noqa: F404


Although:

Python mandates that future-imports must appear in the module before any other code except docstrings

https://peps.python.org/pep-0008/#module-level-dunder-names

Question to the audience - can we live with it (or similar workaround) for now until we find a better alternative?

Partial answer - mypy and some static checks don't like it...

This really does feel like a massive bug in pycharm :(

Yeah, for now I think we can live with that bug.

Once we move a good number of things from Airflow to SDK, maybe the airflow/__init__.py file won't have much content left to keep from __future__ import annotations!

shahar1 · 2024-10-17T13:42:23Z

providers/src/airflow/providers/__init__.py

 # Make `airflow` a namespace package, supporting installing
 # airflow.providers.* in different locations (i.e. one in site, and one in user
 # lib.)  This is required by some IDEs to resolve the import paths.
+# And it _has_ to be the first code line too. Sad panda


shahar1 · 2024-10-17T13:47:27Z

airflow/__init__.py

+# And it _has_ to be the first code line too. Sad panda
 __path__ = __import__("pkgutil").extend_path(__path__, __name__)  # type: ignore

+from __future__ import annotations  # noqa: F404


Question to the audience - can we live with it (or similar workaround) for now until we find a better alternative?

Partial answer - mypy and some static checks don't like it...

vincbeck · 2024-10-17T14:18:53Z

Yeah I tried in #43081 but could not make it work. Mypy/Python is not happy about it and raise a SyntaxError

ashb · 2024-10-17T20:45:53Z

https://youtrack.jetbrains.com/issue/PY-46690/Allow-all-imports-from-the-same-named-packages-if-path-manipulation-in-init-is-detected is the closest current issue I've found, but not exactly the same

potiuk · 2024-10-18T00:13:46Z

Let's just exclude airflow.__init__.py from automatically adding from __future__ import annotations and convert it back to use "regular" typing.

We've done that for Pydantic classes before and still doing it for some test files https://github.com/apache/airflow/blob/main/pyproject.toml#L355

potiuk · 2024-10-18T17:52:11Z

I added alternative implementation - and it seems to work for me #43173 - just by removing from future and adding ignore rules from ruff seem to do the job as I suspected. Actually airflow/__init__.py did not require any change beceause it does not use postponed evaluation of types PEP563 at all.

Cleans-up airflow and providers `__init__.py" files in order to get providers import work again. This is done by excluding the two `__init__.py` files from automated ruff isort rules adding `from __future__ import annotations`. Also removed the `__init__.py` file from "providers" directory, it is not needed there, because "providers" is just a folder where we keep provider files, it's not a Python package. That should finally get rid of the Intellij teething import problem that has been introduced in apache#42505. There were earlier - unsuccesful - attempts to fix it in the apache#43116 and apache#43081 that followed apache#42951 - but the key is that Pycharm requires the namespace's extend_path to be first "real" line of code in the `__init__.py` to understand that the package is an "explicit" namespace package - and it conflicts with the requirement of "from __future__ import annotations" to be the first line of Python code. Also this PR fixes a few other teething problems with setup of tests that were introcuded in apache#42505 and apache#43802 "masked" by having `__init__.py` added in providers package: * common.sql interface pre-commit used wrong path to generated files * openlineage extractor test that should not expect "providers.tests.*" but "tests.*" package * common_sql_api_stubs wrongly calculating generated path for stub-generated files * pytest_plugin expecting .asf.yml in "airflow" sources - even during compatibility tests with older version of airflow (where the .asf.yml is not present)

…3173) Cleans-up airflow and providers `__init__.py" files in order to get providers import work again. This is done by excluding the two `__init__.py` files from automated ruff isort rules adding `from __future__ import annotations`. That should finally get rid of the Intellij teething import problem that has been introduced in #42505. There were earlier - unsuccessful - attempts to fix it in the #43116 and #43081 that followed #42951 - but the key is that Pycharm requires the namespace's extend_path to be first "real" line of code in the `__init__.py` to understand that the package is an "explicit" namespace package - and it conflicts with the requirement of "from __future__ import annotations" to be the first line of Python code. Also this PR fixes following problem: * pytest_plugin expecting .asf.yml in "airflow" sources - even during compatibility tests with older version of airflow (where the .asf.yml is not present) --------- Co-authored-by: Kaxil Naik <[email protected]>

…ache#43173) Cleans-up airflow and providers `__init__.py" files in order to get providers import work again. This is done by excluding the two `__init__.py` files from automated ruff isort rules adding `from __future__ import annotations`. That should finally get rid of the Intellij teething import problem that has been introduced in apache#42505. There were earlier - unsuccessful - attempts to fix it in the apache#43116 and apache#43081 that followed apache#42951 - but the key is that Pycharm requires the namespace's extend_path to be first "real" line of code in the `__init__.py` to understand that the package is an "explicit" namespace package - and it conflicts with the requirement of "from __future__ import annotations" to be the first line of Python code. Also this PR fixes following problem: * pytest_plugin expecting .asf.yml in "airflow" sources - even during compatibility tests with older version of airflow (where the .asf.yml is not present) --------- Co-authored-by: Kaxil Naik <[email protected]>

potiuk · 2024-10-29T21:19:01Z

Closing this one. It's already fixed.

…ache#43173) Cleans-up airflow and providers `__init__.py" files in order to get providers import work again. This is done by excluding the two `__init__.py` files from automated ruff isort rules adding `from __future__ import annotations`. That should finally get rid of the Intellij teething import problem that has been introduced in apache#42505. There were earlier - unsuccessful - attempts to fix it in the apache#43116 and apache#43081 that followed apache#42951 - but the key is that Pycharm requires the namespace's extend_path to be first "real" line of code in the `__init__.py` to understand that the package is an "explicit" namespace package - and it conflicts with the requirement of "from __future__ import annotations" to be the first line of Python code. Also this PR fixes following problem: * pytest_plugin expecting .asf.yml in "airflow" sources - even during compatibility tests with older version of airflow (where the .asf.yml is not present) --------- Co-authored-by: Kaxil Naik <[email protected]>

Cope with Pycharm's oddity in detecting namespace packages

5f87183

Without the `__path__` assignment being the very first line of the file (including before any import) it doesn't detect this correctly.

boring-cyborg bot added the area:providers label Oct 17, 2024

kaxil approved these changes Oct 17, 2024

View reviewed changes

kaxil reviewed Oct 17, 2024

View reviewed changes

shahar1 reviewed Oct 17, 2024

View reviewed changes

potiuk mentioned this pull request Oct 18, 2024

Remove "from __future__" from airflow/providers __init__.py #43173

Merged

potiuk closed this Oct 29, 2024

potiuk deleted the pycarm-ns-pakage-fix branch January 11, 2025 19:44

Conversation

ashb commented Oct 17, 2024

Uh oh!

ashb commented Oct 17, 2024

Uh oh!

kaxil commented Oct 17, 2024

Uh oh!

kaxil Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

shahar1 Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashb Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

kaxil Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

shahar1 Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

shahar1 Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vincbeck commented Oct 17, 2024

Uh oh!

ashb commented Oct 17, 2024

Uh oh!

potiuk commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Oct 18, 2024

Uh oh!

potiuk commented Oct 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shahar1 Oct 17, 2024 •

edited

Loading

shahar1 Oct 17, 2024 •

edited

Loading

potiuk commented Oct 18, 2024 •

edited

Loading