You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dstandish (via GitHub)" <gi...@apache.org> on 2024/11/16 04:25:24 UTC
[PR] Resolve circular import issue related to task sdk [airflow]
dstandish opened a new pull request, #44086:
URL: https://github.com/apache/airflow/pull/44086
When building official prod image locally and trying to run a task with it, I got circular import error.
This PR shows how to resolve it. But it may not be "the right way". For that I defer to @ashb and @kaxil.
Please don't merge this unless they approve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2486570317
> Yeah but there are actual cross deps too right now with SDK relying on Core and Core relying on SDK -- that is what I am referring to
Yep. Let's just fix it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2483485678
What was the error?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2483507217
Yes, we need to sort out our init files "long" term before we cut Airflow 3 release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2484212265
Once we change the core to airflow-core and have task sdk / core completely isolated (from import POV) — things should be better.
In the coming weeks, Ash & I are also planning to take de-couple Core & SDK completely
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2485681043
>FYI @kaxil . Actually just separating airflow core and sdk is not going to help if we keep aliases in sdk I am afraid. This is not because we mix "airlfow" and "sdk" - this is because we are lazy importing modules nested below sdk as part of sdk.__init__.py.
Yeah but there are actual cross deps too right now with SDK relying on Core and Core relying on SDK -- that is what I am referring to
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2484197265
i don't dispute anything related to the import design discussion. i was just trying to make the point that, all i am doing in this pr, is fixing broken main, essentially. i think the import design issues is really a separate discussion and you might want to transfer the discussion to a discussion, or an issue, or dev list. i'm not involved with AIP-72 at this time -- but i am trying to use breeze for other reasons, and this issue was standing in my way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2483898367
> To be clear, this PR doesn't change anything re lazy imports, or imports from root. This is just using the FQN for internal code.
Of course it does not change it - but it **avoids** the problem by switching to FQN imports from "from airflow.sdk import" ones (this is exactly we implemented pre-commits - to forbid the internal code to use the "from airflow import" to avoid circular dependencies. History repeats itself.
The true problem is that `airflow.sdk` has the same pattern - when we attempt to import some of the sub-packages at top level lazily and because our sub-packages refer to each other, this will only work if the original import is importing the sub-packages in the correct sequence, because it in some cases it will attempt to re-import the `__init__.py` that is still being imported.
This will happen always when: you are importing an internal moduke of airflow (in your case `airflow.cli.cli_config`), that then imports any common object from "airflow.sdk" by some sequence of imports it does (say in your case by a sequence of imports `from airflow.sdk imports DAG` gets imported - which in turn imports `airflow.sdk.definitions.dag`), and then that `airflow.sdk.definitions.dag` - again might not be directly but by a sequence of imports, imports another common object from `airflow.sdk` - say "from airflow.sdk import BaseOperator".
In this case what happens:
1) "airflow.cli_config` is "being imported"
2) it imports from `airflow.sdk import DAG`. At this moment the "airflow.sdk" module is not fully imported yet, beacause the whole "from airlfow.sdk import DAG" will only mark the "airflow.sdk" as imported AFTER that line is fully completed by Python parser - only then it will be added to "airflow.sdk".
3) so - while "airflow.sdk" is being imported, the `airflow.sdk.definitions.dag` is imported (via `__getattr__` - because DAG is in the process of being imported)
4) that - in turn - make "dag" module run (directly or indirectly through other imports run `from airflow.sdk import BaseOperator`
5) and this fails with circular import because `airflow.sdk` module is not yet fully imported and we are trying to effectively import it second time before the "on-going" import is completed and "airflow.sdk" is added to "modules" dictionary..
So yes - you are not changing aliases here, you are avoiding the problem, really. And the lazy imports are not really the root-cause of the problem, they are merely half-working workaround to actual issue we have here.
In this case the root cause is not the "lazy import" itself. But the fact that we are "aliasing" multiple packages and modules in the same "airflow.sdk" package (and those classes have cross-imports).
In this case we alias both "airlfow.definitions.sdk.dag.DAG", and "airflow.definitions.sdk.baseoperator.BaseOperator" to be "airflow.sdk.DAG" and "airflow.sdk.BaseOperator" respectively. This aliasing and the fact that those aliased operators are using other "aliases" in the "airflow.sdk" is the root of the problem.
If we did not have those lazy imports, the whole aliasing thing would simply not work at all - you wil always get circular imports if you try to do normal "non-lazy" imports. So "lazy imports" is an attempt to workaround that circular import.
But the lazy imports are only masking the problem and solving only one variant of it - where imports of aliased classes only happen ONCE in the whole import sequence when running the first import. But if the import tree attempts to import aliases twice - even with lazy imports you get "circular reference".
I am not sure if there is any other solution than **not** having aliases at all, I think not because of the simple fact that "from airflow.sdk import ALIAS" will not make "airflow.sdk" fully imported until DAG is fully imported. And it's just consequence of how Python imports work.
The solution of ours where we never use "aliases" in the internal code only works because it effectively avoids having two aliases imported in the "first time import sequence". If none of our internal code is using "from airlfow.sdk import", then it means that we will never attempt to load an alias from an aliased operator. That can be (similarly as in case of Airlfow) prevented and enforced by `pre-commit`.
But the problem is when one of our users uses a custom code that uses "aliases" in their custom code that is imported by "airflow" code. For example when someone uses custom `SecretsManager` that is loaded by our config and that "SecretsManager" uses "from airlfow.sdk import ALIAS". In this case - depending which class is imported first, we might have another alias being imported first, then custom SecretsManager, and then again another alias from "airlfow.sdk" - which would trigger the circular import. And we cannot prevent or enforce custom code to use those aliases. There are plenty of similar issues with custom code of our users causing circular reference because of that very issue.
And i am not sure if that can be solved in other way than not having the aliases at all.
Unless somoene has a better idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2480398502
IMHO I think those lazy imports (below) are evil.
I think we should not try to repeat the same mistake we did with "airflow.". In theory it's **nice** to have objects like that at the top package, but it only creates confusion to have the same classes importable from multiple places and it precisely causes circular imports, because instead of distributing imports and only importing what's needed, we are effectively importing the whole sub-tree - even if those imports are "lazified".
In Airflow 2 it caused us to have a special pre-commit that forbid to import from top-level when we used it in "core" airflow codebase because it caused exactly the same circular imports problem.
I would very much prefer if we ALWAYS explicitly import from `definitions`. Other than being a little longer, it does not have any other bad effects, and it's actually even better for IDEs because it will automatically import it from the right place. And it's more explicit (i.e. follows Python Zen).
I think we are trying to be too smart with those lazy imports - for no good reason. IMHO in most cases __init__ should contain absolutely nothing, unless it's really necessary, that improves startup time - and avoids the circular imports problems like that.
```
__all__ = [
"BaseOperator",
"DAG",
"EdgeModifier",
"Label",
"TaskGroup",
"dag",
"__version__",
]
__version__ = "1.0.0.dev1"
if TYPE_CHECKING:
from airflow.sdk.definitions.baseoperator import BaseOperator
from airflow.sdk.definitions.dag import DAG, dag
from airflow.sdk.definitions.edges import EdgeModifier, Label
from airflow.sdk.definitions.taskgroup import TaskGroup
__lazy_imports: dict[str, str] = {
"DAG": ".definitions.dag",
"dag": ".definitions.dag",
"BaseOperator": ".definitions.baseoperator",
"TaskGroup": ".definitions.taskgroup",
"EdgeModifier": ".definitions.edges",
"Label": ".definitions.edges",
}
def __getattr__(name: str):
if module_path := __lazy_imports.get(name):
import importlib
mod = importlib.import_module(module_path, __name__)
val = getattr(mod, name)
# Store for next time
globals()[name] = val
return val
raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2480403120
This is what we have in Airflow pre-commit now because of that. I would not want to maintain another pre-commit like that.
```
- id: check-base-operator-usage
language: pygrep
name: Check BaseOperator core imports
description: Make sure BaseOperator is imported from airflow.models.baseoperator in core
entry: "from airflow\\.models import.* BaseOperator\\b"
files: \.py$
pass_filenames: true
exclude: >
(?x)
^airflow/decorators/.*$|
^airflow/hooks/.*$|
^airflow/operators/.*$|
^providers/src/airflow/providers/.*$|
^airflow/sensors/.*$|
^dev/provider_packages/.*$
- id: check-base-operator-usage
language: pygrep
name: Check BaseOperatorLink core imports
description: Make sure BaseOperatorLink is imported from airflow.models.baseoperatorlink in core
entry: "from airflow\\.models import.* BaseOperatorLink"
files: \.py$
pass_filenames: true
exclude: >
(?x)
^airflow/decorators/.*$|
^airflow/hooks/.*$|
^airflow/operators/.*$|
^providers/src/airflow/providers/.*$|
^airflow/sensors/.*$|
^dev/provider_packages/.*$
- id: check-base-operator-usage
language: pygrep
name: Check BaseOperator[Link] other imports
description: Make sure BaseOperator[Link] is imported from airflow.models outside of core
entry: "from airflow\\.models\\.baseoperator(link)? import.* BaseOperator"
pass_filenames: true
files: >
(?x)
^providers/src/airflow/providers/.*\.py$
exclude: ^.*/.*_vendor/|providers/src/airflow/providers/standard/operators/bash.py|providers/src/airflow/providers/standard/operators/python.py
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2483463731
To be clear, this PR doesn't change anything re lazy imports, or imports from root. This is just using the FQN for internal code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2484658641
> i don't dispute anything related to the import design discussion. i was just trying to make the point that, all i am doing in this pr, is fixing broken main, essentially. i think the import design issues is really a separate discussion and you might want to transfer the discussion to a discussion, or an issue, or dev list. i'm not involved with AIP-72 at this time -- but i am trying to use breeze for other reasons, and this issue was standing in my way.
Yes, but this oss is extra explanation - I am not disputing the PR - just explaining in detail what was the root cause of it and why this PR was needed so that it is captured somewhere. I am not sure why I would multiply number of issues or discussions here. I could do that as well, but It's very relevant - to understand what happened here (as it is not at all obvious).
> Once we change the core to airflow-core and have task sdk / core completely isolated (from import POV) — things should be better.
FYI @kaxil . Actually just separating airflow core and sdk is not going to help if we keep aliases in `sdk` I am afraid. This is not because we mix "airlfow" and "sdk" - this is because we are lazy importing a modules nested below sdk as part of "sdk.__init__.py".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on PR #44086:
URL: https://github.com/apache/airflow/pull/44086#issuecomment-2483494507
> What was the error?
```log
Traceback (most recent call last):
File "/home/airflow/.local/bin/airflow", line 8, in <module>
sys.exit(main())
File "/opt/airflow/airflow/__main__.py", line 62, in main
args.func(args)
File "/opt/airflow/airflow/cli/cli_config.py", line 48, in command
func = import_string(import_path)
File "/opt/airflow/airflow/utils/module_loading.py", line 39, in import_string
module = import_module(module_path)
File "/usr/local/lib/python3.9/importlib/__init__.py", line 127, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 850, in exec_module
File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
File "/opt/airflow/airflow/cli/commands/task_command.py", line 46, in <module>
from airflow.models.dag import DAG, _run_inline_trigger
File "/opt/airflow/airflow/models/dag.py", line 87, in <module>
from airflow.models.baseoperator import BaseOperator
File "/opt/airflow/airflow/models/baseoperator.py", line 80, in <module>
from airflow.sdk import DAG, BaseOperator as TaskSDKBaseOperator, EdgeModifier as TaskSDKEdgeModifier
ImportError: cannot import name 'DAG' from 'airflow.sdk' (/opt/airflow/task_sdk/src/airflow/sdk/__init__.py)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve circular import issue related to task sdk [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish merged PR #44086:
URL: https://github.com/apache/airflow/pull/44086
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org