shahar1 commented on code in PR #69859:
URL: https://github.com/apache/airflow/pull/69859#discussion_r3597515227
##########
airflow-core/src/airflow/dag_processing/bundles/base.py:
##########
@@ -350,6 +352,19 @@ def initialize(self) -> None:
bundle_path,
)
+ def _log_context(self) -> dict:
+ """Return extra structlog context fields for this bundle. Override in
subclasses."""
+ return {}
+
+ @property
+ def _log(self):
+ """Lazy structlog logger bound with common bundle context plus
subclass-specific fields."""
+ if self.__log is None:
+ self.__log = structlog.get_logger(type(self).__module__).bind(
+ bundle_name=self.name, version=self.version,
**self._log_context()
+ )
+ return self.__log
Review Comment:
**Blocking** — this makes `_log` a read-only data descriptor, so any
subclass that still does `self._log = ...` in `__init__` now raises
`AttributeError: property '_log' of 'X' object has no setter`.
That's not hypothetical: it's exactly what the currently-released `git`,
`amazon`, and `google` providers do. This PR removes those assignments from
sources, but released wheels pin only `apache-airflow>=3.0.0`, so
`airflow-core` from `main` + today's released git provider = `GitDagBundle`
unusable. Same for any third-party bundle, since `BaseDagBundle` is a public
extension point.
A setter keeps the lazy default and every existing subclass working:
```suggestion
@property
def _log(self):
"""Lazy structlog logger bound with common bundle context plus
subclass-specific fields."""
if self.__log is None:
self.__log = structlog.get_logger(type(self).__module__).bind(
bundle_name=self.name, version=self.version,
**self._log_context()
)
return self.__log
@_log.setter
def _log(self, value) -> None:
"""Allow subclasses to bind their own logger (kept for bundles
released before the lazy property)."""
self.__log = value
```
Worth a test that instantiating a bundle which assigns `self._log` still
works — that's the case that regresses.
##########
airflow-core/src/airflow/dag_processing/bundles/base.py:
##########
@@ -350,6 +352,19 @@ def initialize(self) -> None:
bundle_path,
)
+ def _log_context(self) -> dict:
Review Comment:
Two nits on this hook:
1. **Name** — AGENTS.md asks for verbs on methods: *"Name functions and
methods with action verbs: `get_`, `extract_`, `find_`, `compute_`, `build_`,
etc. Avoid noun-only names like `_serialize_keys` or `_base_names` — they read
as attributes, not callables."* `_get_log_context` or `_build_log_context`
would fit. Cheap to do now, before the hook has implementers outside this repo.
2. **Annotation** — `-> dict[str, Any]` (`Any` is already imported here) is
more precise than bare `dict`, and the `_log` property below has no return
annotation.
Related: `self.__log = None` at line 313 types the attribute as `None`; the
assignment at 363 only type-checks because `structlog.get_logger()` returns
`Any`. An explicit `self.__log: Any = None` (or a `BindableLogger | None`)
makes the intent survive a future stricter stub.
##########
providers/common/compat/tests/unit/common/compat/bundles/test_dag_bundle.py:
##########
@@ -0,0 +1,129 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from unittest import mock
+
+import pytest
+
+pytest.importorskip("airflow.dag_processing.bundles", reason="Requires Airflow
3+")
+
+from airflow.dag_processing.bundles.base import BaseDagBundle as
CoreBaseDagBundle
+from airflow.providers.common.compat.bundles import BaseDagBundle
+
+
+class TestDagBundleCompat:
+ def test_base_dag_bundle_is_importable(self):
+ assert BaseDagBundle is not None
+
+ def test_base_dag_bundle_is_subclass_of_core(self):
+ assert issubclass(BaseDagBundle, CoreBaseDagBundle)
+
+ def test_has_log_property(self):
+ assert hasattr(BaseDagBundle, "_log")
+
+ def test_log_property_on_subclass_instance(self, tmp_path):
+ """_log returns a bound structlog logger on concrete bundle
subclasses."""
+ from pathlib import Path
+
+ from tests_common.test_utils.config import conf_vars
+
+ class _DummyBundle(BaseDagBundle):
+ @property
+ def path(self) -> Path:
+ return tmp_path
+
+ def initialize(self) -> None:
+ pass
+
+ def get_current_version(self):
+ return None
+
+ def refresh(self) -> None:
+ pass
+
+ with conf_vars({("dag_processor", "dag_bundle_storage_path"):
str(tmp_path)}):
+ bundle = _DummyBundle(name="test-bundle")
+
+ log = bundle._log
+ assert log is not None
+ # Calling _log twice returns the same (cached) logger.
+ assert bundle._log is log
+
+ def test_log_is_bound_with_context(self, tmp_path):
+ """_log includes bundle_name and version in its bound variables."""
+ from pathlib import Path
+
+ from tests_common.test_utils.config import conf_vars
+
+ class _DummyBundle(BaseDagBundle):
+ @property
+ def path(self) -> Path:
+ return tmp_path
+
+ def initialize(self) -> None:
+ pass
+
+ def get_current_version(self):
+ return None
+
+ def refresh(self) -> None:
+ pass
+
+ with conf_vars({("dag_processor", "dag_bundle_storage_path"):
str(tmp_path)}):
+ bundle = _DummyBundle(name="my-bundle", version="abc123")
+
+ log = bundle._log
+ # structlog bound loggers expose their bindings via _context
+ ctx = log._context if hasattr(log, "_context") else {}
+ assert ctx.get("bundle_name") == "my-bundle"
+ assert ctx.get("version") == "abc123"
+
+ def test_compat_subclass_provides_log_when_missing(self, tmp_path):
+ """Even when the installed BaseDagBundle has no _log, the compat class
fills it in."""
+ from pathlib import Path
+
+ from tests_common.test_utils.config import conf_vars
Review Comment:
Function-level imports — `.github/instructions/code-review.instructions.md`:
*"**Flag any `from` or `import` statement inside a function or method body.**
Imports must be at the top of the file. The only valid exceptions are: (1)
circular import avoidance, (2) lazy loading for worker isolation, (3)
`TYPE_CHECKING` blocks."*
`pathlib.Path`, `tests_common.test_utils.config.conf_vars`, and
`importlib.reload` are all safe at module level — the `pytest.importorskip` at
line 23 already guards the Airflow-3-only imports below it, and none of these
three depend on it.
Same applies to the copies in `test_log_property_on_subclass_instance` and
`test_log_is_bound_with_context`. While you're here: `_DummyBundle` is defined
three times, identical apart from its base class — a single module-level
factory would drop roughly half this file.
##########
providers/common/compat/tests/unit/common/compat/bundles/test_dag_bundle.py:
##########
@@ -0,0 +1,129 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+from unittest import mock
+
+import pytest
+
+pytest.importorskip("airflow.dag_processing.bundles", reason="Requires Airflow
3+")
+
+from airflow.dag_processing.bundles.base import BaseDagBundle as
CoreBaseDagBundle
+from airflow.providers.common.compat.bundles import BaseDagBundle
+
+
+class TestDagBundleCompat:
+ def test_base_dag_bundle_is_importable(self):
+ assert BaseDagBundle is not None
+
+ def test_base_dag_bundle_is_subclass_of_core(self):
+ assert issubclass(BaseDagBundle, CoreBaseDagBundle)
+
+ def test_has_log_property(self):
+ assert hasattr(BaseDagBundle, "_log")
+
+ def test_log_property_on_subclass_instance(self, tmp_path):
+ """_log returns a bound structlog logger on concrete bundle
subclasses."""
+ from pathlib import Path
+
+ from tests_common.test_utils.config import conf_vars
+
+ class _DummyBundle(BaseDagBundle):
+ @property
+ def path(self) -> Path:
+ return tmp_path
+
+ def initialize(self) -> None:
+ pass
+
+ def get_current_version(self):
+ return None
+
+ def refresh(self) -> None:
+ pass
+
+ with conf_vars({("dag_processor", "dag_bundle_storage_path"):
str(tmp_path)}):
+ bundle = _DummyBundle(name="test-bundle")
+
+ log = bundle._log
+ assert log is not None
+ # Calling _log twice returns the same (cached) logger.
+ assert bundle._log is log
+
+ def test_log_is_bound_with_context(self, tmp_path):
+ """_log includes bundle_name and version in its bound variables."""
+ from pathlib import Path
+
+ from tests_common.test_utils.config import conf_vars
+
+ class _DummyBundle(BaseDagBundle):
+ @property
+ def path(self) -> Path:
+ return tmp_path
+
+ def initialize(self) -> None:
+ pass
+
+ def get_current_version(self):
+ return None
+
+ def refresh(self) -> None:
+ pass
+
+ with conf_vars({("dag_processor", "dag_bundle_storage_path"):
str(tmp_path)}):
+ bundle = _DummyBundle(name="my-bundle", version="abc123")
+
+ log = bundle._log
+ # structlog bound loggers expose their bindings via _context
+ ctx = log._context if hasattr(log, "_context") else {}
+ assert ctx.get("bundle_name") == "my-bundle"
+ assert ctx.get("version") == "abc123"
+
+ def test_compat_subclass_provides_log_when_missing(self, tmp_path):
+ """Even when the installed BaseDagBundle has no _log, the compat class
fills it in."""
+ from pathlib import Path
+
+ from tests_common.test_utils.config import conf_vars
+
+ # Simulate an older Airflow version by temporarily hiding _log from
+ # the core class; the compat class must still expose it.
+ with mock.patch.object(CoreBaseDagBundle, "_log", None, create=True):
+ from importlib import reload
+
+ import airflow.providers.common.compat.bundles as mod
+
+ reload(mod)
+ CompatBase = mod.BaseDagBundle
Review Comment:
This `reload()` leaks. When the `mock.patch.object` block exits,
`CoreBaseDagBundle._log` is restored — but
`sys.modules["airflow.providers.common.compat.bundles"]` still holds the
reloaded module, so its `BaseDagBundle` stays the compat fallback subclass for
the remainder of the session. Anything importing it later (including
`test_base_dag_bundle_is_subclass_of_core` in this class, under `-p
no:randomly` reordering or `--lf`) silently exercises the wrong branch.
Reloading back once the patch is lifted keeps it contained:
```python
@pytest.fixture
def legacy_compat_base():
import airflow.providers.common.compat.bundles as mod
with mock.patch.object(CoreBaseDagBundle, "_log", None, create=True):
reload(mod)
yield mod.BaseDagBundle
reload(mod)
```
The `yield` inside the patch also lets the dummy subclass be built while the
fallback is genuinely active, which is closer to what the test claims to check.
--
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]
