You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/08/19 07:24:29 UTC
[GitHub] [airflow] Taragolis opened a new pull request, #25810: Avoid cyclic import problems when instantiating AWS SM backend
Taragolis opened a new pull request, #25810:
URL: https://github.com/apache/airflow/pull/25810
Fix cyclic import problems when instantiating AWS SecretsManagerBackend in `apache-airflow-providers-amazon==5.0.0`
**before**
```shell
❯ export AIRFLOW__SECRETS__BACKEND=airflow.providers.amazon.aws.secrets.secrets_manager.SecretsManagerBackend
❯ airflow version
cannot import name 'STATE_COLORS' from partially initialized module 'airflow.settings' (most likely due to a circular import) (/Users/taragolis/Projects/common/airflow/airflow/settings.py)
Traceback (most recent call last):
File "/Users/taragolis/Projects/common/airflow/airflow/configuration.py", line 689, in getimport
return import_string(full_qualified_path)
File "/Users/taragolis/Projects/common/airflow/airflow/utils/module_loading.py", line 32, in import_string
module = import_module(module_path)
File "/Users/taragolis/.pyenv/versions/3.9.10/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 "/Users/taragolis/Projects/common/airflow/airflow/providers/amazon/aws/secrets/secrets_manager.py", line 27, in <module>
from airflow.models.connection import Connection
File "/Users/taragolis/Projects/common/airflow/airflow/models/__init__.py", line 22, in <module>
from airflow.models.baseoperator import BaseOperator, BaseOperatorLink
File "/Users/taragolis/Projects/common/airflow/airflow/models/baseoperator.py", line 75, in <module>
from airflow.models.mappedoperator import OperatorPartial, validate_mapping_kwargs
File "/Users/taragolis/Projects/common/airflow/airflow/models/mappedoperator.py", line 71, in <module>
from airflow.models.pool import Pool
File "/Users/taragolis/Projects/common/airflow/airflow/models/pool.py", line 26, in <module>
from airflow.ti_deps.dependencies_states import EXECUTION_STATES
File "/Users/taragolis/Projects/common/airflow/airflow/ti_deps/dependencies_states.py", line 18, in <module>
from airflow.utils.state import State
File "/Users/taragolis/Projects/common/airflow/airflow/utils/state.py", line 22, in <module>
from airflow.settings import STATE_COLORS
ImportError: cannot import name 'STATE_COLORS' from partially initialized module 'airflow.settings' (most likely due to a circular import) (/Users/taragolis/Projects/common/airflow/airflow/settings.py)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/taragolis/.pyenv/versions/3.9.10/envs/airflow-dev-env-39/bin/airflow", line 33, in <module>
sys.exit(load_entry_point('apache-airflow', 'console_scripts', 'airflow')())
File "/Users/taragolis/.pyenv/versions/3.9.10/envs/airflow-dev-env-39/bin/airflow", line 25, in importlib_load_entry_point
return next(matches).load()
File "/Users/taragolis/.pyenv/versions/3.9.10/lib/python3.9/importlib/metadata.py", line 77, in load
module = import_module(match.group('module'))
File "/Users/taragolis/.pyenv/versions/3.9.10/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 972, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
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 "/Users/taragolis/Projects/common/airflow/airflow/__init__.py", line 35, in <module>
from airflow import settings
File "/Users/taragolis/Projects/common/airflow/airflow/settings.py", line 35, in <module>
from airflow.configuration import AIRFLOW_HOME, WEBSERVER_CONFIG, conf # NOQA F401
File "/Users/taragolis/Projects/common/airflow/airflow/configuration.py", line 1649, in <module>
secrets_backend_list = initialize_secrets_backends()
File "/Users/taragolis/Projects/common/airflow/airflow/configuration.py", line 1571, in initialize_secrets_backends
custom_secret_backend = get_custom_secret_backend()
File "/Users/taragolis/Projects/common/airflow/airflow/configuration.py", line 1546, in get_custom_secret_backend
secrets_backend_cls = conf.getimport(section='secrets', key='backend')
File "/Users/taragolis/Projects/common/airflow/airflow/configuration.py", line 692, in getimport
raise AirflowConfigException(
airflow.exceptions.AirflowConfigException: The object could not be loaded. Please check "backend" key in "secrets" section. Current value: "airflow.providers.amazon.aws.secrets.secrets_manager.SecretsManagerBackend"
```
**after**
```shell
❯ export AIRFLOW__SECRETS__BACKEND=airflow.providers.amazon.aws.secrets.secrets_manager.SecretsManagerBackend
❯ airflow version
2.4.0.dev0
```
cc: @o-nikolas @ferruzzi @vincbeck @potiuk
--
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
[GitHub] [airflow] Taragolis commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25810:
URL: https://github.com/apache/airflow/pull/25810#discussion_r949910732
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
return conn_string
- def get_connection(self, conn_id: str) -> Optional[Connection]:
+ def get_connection(self, conn_id: str) -> Optional["Connection"]:
if not self.full_url_mode:
+ # Avoid circular import problems when instantiating the backend during configuration.
+ from airflow.models.connection import Connection
Review Comment:
Personally I have no idea how to catch such an issue, rather than just manually run some simple command for all known SecretsBackends:
```shell
❯ export AIRFLOW__SECRETS__BACKEND=full.qualified.name.to.SB
❯ export AIRFLOW__SECRETS__BACKEND_KWARGS='{}'
❯ export airflow version
```
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25810:
URL: https://github.com/apache/airflow/pull/25810#discussion_r950265535
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
return conn_string
- def get_connection(self, conn_id: str) -> Optional[Connection]:
+ def get_connection(self, conn_id: str) -> Optional["Connection"]:
if not self.full_url_mode:
+ # Avoid circular import problems when instantiating the backend during configuration.
+ from airflow.models.connection import Connection
Review Comment:
Yeah. We definitely import too much in some core packages.
And we should indeed add secrets manager tests. But I think #24486 is not the solution to this problem. It is a good approach but we have a problem with internal architecture, not with imports.
The real problem is not importing stuff, the problem is that we actually HAVE a circular dependency here and it's because our configuration approach is pretty badly intertwined with other parts of the system in this case we have:
* configuration can read values by delegating to secrets manager
* secrets manager uses configuration to configure itself
While I agree local imports and TYPE_CHECKING can provide some solution to the problem, they are very prone to trigger similar problems.
I think we should finally do something about it - I think ti's a bad approach that two internal components of systems are depending bi-directionally on each other :). The dependency between those should be one way - either conf is using secrets backend or secrets backend using conf. Basically we have `secrets -> conf -> secret` dependency chain which makes it very easy to turn it into circular import problem.
Probably the best solution is to split out the part that secret REALLY need from conf and make it an "internal conf" (and then `conf -> secret -> "internal conf"` will be the correct dependency chain that will not lead to accidental triggering of such cricular imports.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25810:
URL: https://github.com/apache/airflow/pull/25810#discussion_r950265535
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
return conn_string
- def get_connection(self, conn_id: str) -> Optional[Connection]:
+ def get_connection(self, conn_id: str) -> Optional["Connection"]:
if not self.full_url_mode:
+ # Avoid circular import problems when instantiating the backend during configuration.
+ from airflow.models.connection import Connection
Review Comment:
Yeah. We definitely import too much in some core packages.
And we should indeed add secrets manager tests. But I think #24486 is not the solution to this problem. It is a good approach but we have a problem with internal architecture, not with imports.
The real problem is not thmodes limporting stuff, the problem is that we actually HAVE a circular dependency here and it's because our configuration approach is pretty badly intertwined with other parts of the system in this case we have:
* configuration can read values by delegating to secrets manager
* secrets manager uses configuration to configure itself
While I agree local imports and TYPE_CHECKING can provide some solution to the problem, they are very prone to trigger similar problems.
I think we should finally do something about it - I think ti's a bad approach that two independent parts of systems are depending on each other :). The dependency between those should be one way - either conf is using secrets backend or secrets backend using conf. Basically we have `secrets -> conf -> secret` dependency chain which makes it very easy to turn it into circular import problem.
Probably the best solution is to split out the part that secret REALLY need from conf and make it an "internal conf" (and then `conf -> secret -> "internal conf"` will be the correct dependency chain that will not lead to accidental triggering of such cricular imports.
--
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
[GitHub] [airflow] Taragolis commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25810:
URL: https://github.com/apache/airflow/pull/25810#discussion_r949910732
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
return conn_string
- def get_connection(self, conn_id: str) -> Optional[Connection]:
+ def get_connection(self, conn_id: str) -> Optional["Connection"]:
if not self.full_url_mode:
+ # Avoid circular import problems when instantiating the backend during configuration.
+ from airflow.models.connection import Connection
Review Comment:
Personally I have no idea how to catch such an issue, rather than just manually run some simple command for all known SecretsBackends:
```
❯ export AIRFLOW__SECRETS__BACKEND=full.qualified.name.to.SB
❯ export AIRFLOW__SECRETS__BACKEND_KWARGS='{}'
❯ export airflow version
```
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25810:
URL: https://github.com/apache/airflow/pull/25810#discussion_r949945663
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
return conn_string
- def get_connection(self, conn_id: str) -> Optional[Connection]:
+ def get_connection(self, conn_id: str) -> Optional["Connection"]:
if not self.full_url_mode:
+ # Avoid circular import problems when instantiating the backend during configuration.
+ from airflow.models.connection import Connection
Review Comment:
There’s not really a good way to catch this, mainly due to `airflow.models` too eagerly imports everything. #24486 would be a good general solution to the problem in general.
--
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
[GitHub] [airflow] potiuk merged pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25810:
URL: https://github.com/apache/airflow/pull/25810
--
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
[GitHub] [airflow] Taragolis commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #25810:
URL: https://github.com/apache/airflow/pull/25810#discussion_r949910732
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
return conn_string
- def get_connection(self, conn_id: str) -> Optional[Connection]:
+ def get_connection(self, conn_id: str) -> Optional["Connection"]:
if not self.full_url_mode:
+ # Avoid circular import problems when instantiating the backend during configuration.
+ from airflow.models.connection import Connection
Review Comment:
Personally I have no idea how to catch such an issue, rather than just manually run some simple command for all known SecretsBackends:
```shell
❯ export AIRFLOW__SECRETS__BACKEND=full.qualified.name.to.SB
❯ export AIRFLOW__SECRETS__BACKEND_KWARGS='{}'
❯ airflow version
```
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25810:
URL: https://github.com/apache/airflow/pull/25810#discussion_r950265535
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
return conn_string
- def get_connection(self, conn_id: str) -> Optional[Connection]:
+ def get_connection(self, conn_id: str) -> Optional["Connection"]:
if not self.full_url_mode:
+ # Avoid circular import problems when instantiating the backend during configuration.
+ from airflow.models.connection import Connection
Review Comment:
Yeah. We definitely import too much in some core packages.
And we should indeed add secrets manager tests. But I think #24486 is not the solution to this problem. It is a good approach but we have a problem with internal architecture, not with imports.
The real problem is not importing stuff, the problem is that we actually HAVE a circular dependency here and it's because our configuration approach is pretty badly intertwined with other parts of the system in this case we have:
* configuration can read values by delegating to secrets manager
* secrets manager uses configuration to configure itself
While I agree local imports and TYPE_CHECKING can provide some solution to the problem, they are very prone to trigger similar problems.
I think we should finally do something about it - I think ti's a bad approach that two independent parts of systems are depending on each other :). The dependency between those should be one way - either conf is using secrets backend or secrets backend using conf. Basically we have `secrets -> conf -> secret` dependency chain which makes it very easy to turn it into circular import problem.
Probably the best solution is to split out the part that secret REALLY need from conf and make it an "internal conf" (and then `conf -> secret -> "internal conf"` will be the correct dependency chain that will not lead to accidental triggering of such cricular imports.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25810:
URL: https://github.com/apache/airflow/pull/25810#discussion_r950265535
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
return conn_string
- def get_connection(self, conn_id: str) -> Optional[Connection]:
+ def get_connection(self, conn_id: str) -> Optional["Connection"]:
if not self.full_url_mode:
+ # Avoid circular import problems when instantiating the backend during configuration.
+ from airflow.models.connection import Connection
Review Comment:
Yeah. We definitely import too much in some core packages.
And we should indeed add secrets manager tests. But I think #24486 is not the solution to this problem. It is a good approach but we have a problem with internal architecture, not with imports.
The real problem is not thmodes limporting stuff, the problem is that we actually HAVE a circular dependency here and it's because our configuration approach is pretty badly intertwined with other parts of the system in this case we have:
* configuration can read values by delegating to secrets manager
* secrets manager uses configuration to configure itself
While I agree local imports and TYPE_CHECKING can provide some solution to the problem, they are very prone to trigger similar problems.
I think we should finally do something about it - I think ti's a bad approach that two independent parts of systems are dependeing each other :). The dependency between those should be one way - either conf is using secrets backend or secrets backend using conf. Basically we have `secrets -> conf -> secret` dependency chain which makes it very easy to turn it into circular import problem.
Probably this is the best solution - is to split out the part that secret REALLY need from conf and make it an "internal conf" (and then `conf -> secret -> "internal conf"` will be the correct dependency chain that will not lead to accidental triggering of such cricular imports.
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #25810: Avoid circular import problems when instantiating AWS SM backend
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25810:
URL: https://github.com/apache/airflow/pull/25810#discussion_r950265535
##########
airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -193,8 +196,11 @@ def _format_uri_with_extra(secret, conn_string: str) -> str:
return conn_string
- def get_connection(self, conn_id: str) -> Optional[Connection]:
+ def get_connection(self, conn_id: str) -> Optional["Connection"]:
if not self.full_url_mode:
+ # Avoid circular import problems when instantiating the backend during configuration.
+ from airflow.models.connection import Connection
Review Comment:
Yeah. We definitely import too much in some core packages.
And we should indeed add secrets manager tests. But I think #24486 is not the solution to this problem. It is a good approach but we have a problem with internal architecture, not with imports.
The real problem is not thmodes limporting stuff, the problem is that we actually HAVE a circular dependency here and it's because our configuration approach is pretty badly intertwined with other parts of the system in this case we have:
* configuration can read values by delegating to secrets manager
* secrets manager uses configuration to configure itself
While I agree local imports and TYPE_CHECKING can provide some solution to the problem, they are very prone to trigger similar problems.
I think we should finally do something about it - I think ti's a bad approach that two independent parts of systems are depending on each other :). The dependency between those should be one way - either conf is using secrets backend or secrets backend using conf. Basically we have `secrets -> conf -> secret` dependency chain which makes it very easy to turn it into circular import problem.
Probably this is the best solution - is to split out the part that secret REALLY need from conf and make it an "internal conf" (and then `conf -> secret -> "internal conf"` will be the correct dependency chain that will not lead to accidental triggering of such cricular imports.
--
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