You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dabla (via GitHub)" <gi...@apache.org> on 2026/07/14 08:12:27 UTC
[PR] Centralise structlog initialisation in BaseDagBundle [airflow]
dabla opened a new pull request, #69859:
URL: https://github.com/apache/airflow/pull/69859
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: http://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: [#67016](https://github.com/apache/airflow/pull/67016)
-->
`BaseDagBundle` now owns the structlog logger via a lazy `_log` property,
so subclasses no longer need to construct and bind it themselves in their
constructors. I saw all Dag bundles extending this class re-implemented
the same logic regarding the structlog initialization when reviewing PR
[#67016](https://github.com/apache/airflow/pull/67016), hence why I
created this PR to make the code more DRY.
### What changed
- `BaseDagBundle` gains a `_log` lazy property that creates a structlog
logger on first access, bound with `bundle_name`, `version`, and
whatever `_log_context()` returns.
- A `_log_context(self) -> dict` template-method hook (returns `{}` by
default) lets each subclass declare its own extra log fields without
duplicating the boilerplate `structlog.get_logger(...).bind(...)` call.
- `GCSDagBundle`, `S3DagBundle`, and `GitDagBundle` each implement
`_log_context()` and drop their manual `self._log = log.bind(...)` init
blocks and the now-unused module-level / local `structlog.get_logger`
calls.
- `LocalDagBundle` needs no change — the default `_log_context()` returns
`{}`, which is the correct behaviour for a bundle with no extra context.
### Why lazy?
`BaseDagBundle.__init__` is called via `super().__init__()` *before*
subclass-specific attributes (e.g. `bucket_name`, `bare_repo_path`) are
set. Evaluating `_log_context()` at base-`__init__` time would therefore
fail or produce incomplete context. The lazy property defers evaluation
until first use, by which point all `__init__` work across the hierarchy
is complete.
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ x ] Yes (please specify the tool below)
Claude Sonnet 4.6
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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] Centralise structlog initialisation in BaseDagBundle [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #69859:
URL: https://github.com/apache/airflow/pull/69859#issuecomment-5381733804
> **[uranusjr](/uranusjr) ** commented
The approach is essentially the same philosophy as `LoggingMixin.log`, a lazy property that defers creation to first access, except it also binds bundle-specific structured context fields. A plain module-level logger could work but would require every single `self.log.info(...) `call site to manually pass `bundle_name=`, `bucket_name=`, etc. as keyword args, which is exactly the repetition the PR is trying to eliminate.
--
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] Centralise structlog initialisation in BaseDagBundle [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Centralise structlog initialisation in BaseDagBundle [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #69859:
URL: https://github.com/apache/airflow/pull/69859#issuecomment-5340618425
I’m not personally a fan of attaching a logger to the class. Logger objects belong to modules for me. Automatically naming the logger after the class name is also unnecessary cruft in my opinion.
--
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] Centralise structlog initialisation in BaseDagBundle [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #69859:
URL: https://github.com/apache/airflow/pull/69859#issuecomment-6187271885
@VladaZakharova thanks, you were right: `GitDagBundle.__init__` logged through `self._log`, which built and cached the logger from `_get_log_context()` before a subclass had set its own attributes, so the subclass got an `AttributeError` or an incomplete context it kept for good.
Fixed in 438284d9e1 by taking your first option, not calling the overridable hook from the constructor: the three construction messages now go through a logger bound with `GitDagBundle._get_log_context(self)`, which calls `GitDagBundle`'s own context by class name instead of dispatching to the override. `self._log` is therefore first built after construction and includes the subclass's fields. I did not take the "finalize after construction" route, because knowing when the most-derived `__init__` has returned would need a metaclass on `BaseDagBundle`, a public extension point, for a problem only constructors that log have.
`test_subclass_log_context_uses_attributes_set_after_construction` subclasses `GitDagBundle` with an attribute set after `super().__init__()` and checks it ends up in the bound context; it fails with `AttributeError: 'TeamGitDagBundle' object has no attribute 'team'` without the fix.
The branch is also rebased on the latest `main` (conflicts in `git.py` and the amazon `pyproject.toml`).
--
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