kaxil commented on code in PR #71199:
URL: https://github.com/apache/airflow/pull/71199#discussion_r3761960439
##########
airflow-core/src/airflow/utils/helpers.py:
##########
@@ -313,11 +313,12 @@ def __getattr__(name: str):
except KeyError:
raise AttributeError(f"module '{__name__}' has no attribute '{name}'")
from None
+ import importlib
import warnings
warnings.warn(
f"{__name__}.{name} is deprecated. Use {modpath}.{name} instead.",
DeprecationWarning,
stacklevel=2,
)
- return getattr(__import__(modpath), name)
+ return getattr(importlib.import_module(modpath), name)
Review Comment:
Can you add a regression test?
`airflow-core/tests/unit/utils/test_helpers.py` already imports `helpers`, so
it's a few lines:
```python
@pytest.mark.parametrize(
"name", ["render_template_as_native", "render_template_to_string",
"prevent_duplicates"]
)
def test_deprecated_imports_resolve(name):
with pytest.warns(DeprecationWarning):
assert getattr(helpers, name) is not None
```
Nothing in the tree touches `__deprecated_imports` today, which is why all
three entries were broken rather than just one.
##########
airflow-core/src/airflow/utils/helpers.py:
##########
@@ -313,11 +313,12 @@ def __getattr__(name: str):
except KeyError:
raise AttributeError(f"module '{__name__}' has no attribute '{name}'")
from None
+ import importlib
import warnings
warnings.warn(
f"{__name__}.{name} is deprecated. Use {modpath}.{name} instead.",
DeprecationWarning,
Review Comment:
`stacklevel=2` attributes the warning to the caller's module, so a DAG
author doing `from airflow.utils.helpers import render_template_as_native` gets
a `DeprecationWarning` reported against their DAG file, which Python's default
`ignore` filter drops. The `action="default"` filter Airflow installs in
`configuration.py:58` is scoped to `module="airflow"` and doesn't match either.
I confirmed locally that after your fix the shim resolves correctly but the
notice never reaches stderr.
We already have `airflow.utils.deprecation_tools.add_deprecated_classes` for
this, and it gets both parts right: `importlib.import_module` plus
`DeprecatedImportWarning`, which subclasses `FutureWarning` and so is shown by
default. `airflow/io/__init__.py` uses the same `__name__: {...}` form. Worth
replacing the hand-rolled block with:
```python
from airflow.utils.deprecation_tools import add_deprecated_classes
add_deprecated_classes(
{
__name__: {
"render_template_as_native":
"airflow.sdk.definitions.context.render_template_as_native",
"render_template_to_string":
"airflow.sdk.definitions.context.render_template_to_string",
"prevent_duplicates":
"airflow.sdk.definitions.mappedoperator.prevent_duplicates",
}
},
package=__name__,
)
```
which lets both `__getattr__` and `__deprecated_imports` go away.
##########
airflow-core/src/airflow/utils/helpers.py:
##########
@@ -313,11 +313,12 @@ def __getattr__(name: str):
except KeyError:
raise AttributeError(f"module '{__name__}' has no attribute '{name}'")
from None
+ import importlib
Review Comment:
Minor: `importlib` is part of the import machinery and always loaded, so
this can sit at the top of the file with the rest of the imports. Moot if you
take the `add_deprecated_classes` route above.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
