You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kjh0623 (via GitHub)" <gi...@apache.org> on 2026/07/17 22:51:20 UTC
[PR] Add refresh_on_initialize option to GitDagBundle [airflow]
kjh0623 opened a new pull request, #70058:
URL: https://github.com/apache/airflow/pull/70058
**What this adds**
An opt-in `refresh_on_initialize` kwarg (default `True`, no behavior change for existing users) on `GitDagBundle`, for deployments where many ephemeral processes initialize the same bundle over shared storage:
```
GitDagBundle(tracking_ref="main", refresh_on_initialize=False)
```
Today, unversioned `initialize()` always ends in a remote fetch. That is the right default for a dag processor keeping the bundle fresh, but with KubernetesExecutor every worker pod initializing the bundle pays that fetch on startup — for our repository this adds ~15s of cold-start to every single task, even though the shared volume already holds a tracking repo the dag processor refreshes continuously. Shallow/sparse clones shave the cost down but every pod still pays a network round-trip.
With `refresh_on_initialize=False`, `initialize()` reuses the tracking repo already on disk as-is and skips all network operations. Scope guards:
- the repository is still cloned (and fetched) when nothing is on disk yet,
- explicit `refresh()` calls still fetch as usual (both repo handles are assigned in the reuse path),
- version-pinned initialization is unaffected (it has its own reuse paths already),
- any failure reusing the on-disk repo falls back to the full initialization path.
This generalizes what we currently run in production as a small `GitDagBundle` subclass (workers reuse the PVC-resident tracking repo, only the dag processor fetches). Related context on the worker cold-start motivation was discussed in #70003.
**Included**
- `refresh_on_initialize` kwarg + docstring
- Unit tests: reuse path serves the on-disk state and skips the fetch while explicit `refresh()` still fetches; default behavior unchanged (second initialize fetches latest)
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
---
##### Was generative AI tooling used to co-author this PR?
- [x] Yes (please specify the tool below)
Generated-by: Claude Code 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] Add refresh_on_initialize option to GitDagBundle [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #70058:
URL: https://github.com/apache/airflow/pull/70058#issuecomment-5942829175
Who besides your deployment needs this? `refresh_on_initialize=False` only helps with versioning disabled and shared storage, and as the new docs say, it gains nothing for the default per-pod layout and serves stale code indefinitely if nothing else refreshes the storage. It's a permanent `GitDagBundle` kwarg for that one layout. Could workers on shared storage point a `LocalDagBundle` at the shared checkout instead? Is there an issue or another user asking for this? Converting to draft until there is.
--
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] Add refresh_on_initialize option to GitDagBundle [airflow]
Posted by "Sanjays2402 (via GitHub)" <gi...@apache.org>.
Sanjays2402 commented on code in PR #70058:
URL: https://github.com/apache/airflow/pull/70058#discussion_r3610110970
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -195,6 +205,38 @@ def _initialize(self):
self.repo = repo
return
+ if (
+ not self.version
+ and not self.refresh_on_initialize
+ and (self.repo_path / ".git").exists()
+ and self.bare_repo_path.exists()
+ ):
+ # The tracking repo is already on disk and the caller opted out of refreshing on
+ # initialize - reuse it as-is and skip all network operations. Fall through to the
+ # full initialization path if the on-disk repos turn out to be unusable. Both repo
+ # handles are still assigned so a later explicit refresh() works as usual.
+ try:
+ repo = Repo(self.repo_path)
+ try:
+ repo.git.checkout(self.tracking_ref)
+ bare_repo = Repo(self.bare_repo_path)
+ except Exception:
+ repo.close()
+ raise
+ except (InvalidGitRepositoryError, NoSuchPathError, GitCommandError) as e:
Review Comment:
the inner block re-raises non-git errors (the bare `raise` after close), but this outer except only catches InvalidGitRepositoryError/NoSuchPathError/GitCommandError. so a checkout failing with something like a PermissionError or OSError on the on-disk repo escapes here instead of falling through to full initialization, which defeats the "reuse it as-is, fall back if unusable" intent in the comment. intended?
--
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] Add refresh_on_initialize option to GitDagBundle [airflow]
Posted by "kjh0623 (via GitHub)" <gi...@apache.org>.
kjh0623 commented on PR #70058:
URL: https://github.com/apache/airflow/pull/70058#issuecomment-5103773894
Yeah, I've been running this on our on-prem K8s cluster for a while now — it's actually already deployed in our production Airflow and has been working without issues.
In our setup the DAG processor keeps the tracking repo fresh on the shared volume, so having every worker pod re-fetch on init was pure overhead. With refresh_on_initialize=False that ~15s per-pod cold start on task startup basically goes away, since it just reuses what's already on disk. The clone-if-missing and explicit refresh() paths still behave the same, so we haven't seen any staleness problems in practice.
Happy to share more details about the environment if that'd help.
--
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] Add refresh_on_initialize option to GitDagBundle [airflow]
Posted by "kjh0623 (via GitHub)" <gi...@apache.org>.
kjh0623 commented on PR #70058:
URL: https://github.com/apache/airflow/pull/70058#issuecomment-6166497606
Thanks, that's fair. You're right that this only pays off in one fairly specific layout, with versioning disabled and bundle storage shared between the Dag processor and workers. I don't have other users asking for it, so a permanent `GitDagBundle` kwarg is hard to justify. I'll close this PR and keep running it as a small subclass on our side, where it's been working fine.
Thanks for the review, and thanks @Sanjays2402 for catching the exception handling gap.
--
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] Add refresh_on_initialize option to GitDagBundle [airflow]
Posted by "kjh0623 (via GitHub)" <gi...@apache.org>.
kjh0623 closed pull request #70058: Add refresh_on_initialize option to GitDagBundle
URL: https://github.com/apache/airflow/pull/70058
--
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] Add refresh_on_initialize option to GitDagBundle [airflow]
Posted by "kjh0623 (via GitHub)" <gi...@apache.org>.
kjh0623 commented on PR #70058:
URL: https://github.com/apache/airflow/pull/70058#issuecomment-5018450339
Good catch — you're right, that's a real gap, not intended.
The inner block catches everything and re-raises after `repo.close()` (so the handle doesn't leak), but the outer `except` only listed the three git errors, so a re-raised `OSError`/`PermissionError` from the checkout escapes instead of falling through to full initialization — exactly contradicting the "reuse it as-is, fall back if unusable" comment. And it matters most for the target use case here (ephemeral pods over shared bundle storage), where a filesystem hiccup on the on-disk repo is the likely failure.
Fixed in 08b321e by broadening the outer `except` to `Exception`, so any unusable on-disk repo falls back to full initialization as documented. Falling back broadly is safe here because the fallback is the normal init path (a genuine problem would resurface there), and it's logged at info with the exception. `BaseException` (KeyboardInterrupt/SystemExit) is intentionally left to propagate.
Happy to add a regression test that forces a non-git error on the reuse checkout and asserts the fallback, if you'd like it in this PR.
--
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] Add refresh_on_initialize option to GitDagBundle [airflow]
Posted by "kjh0623 (via GitHub)" <gi...@apache.org>.
kjh0623 commented on code in PR #70058:
URL: https://github.com/apache/airflow/pull/70058#discussion_r3622418592
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -195,6 +205,38 @@ def _initialize(self):
self.repo = repo
return
+ if (
+ not self.version
+ and not self.refresh_on_initialize
+ and (self.repo_path / ".git").exists()
+ and self.bare_repo_path.exists()
+ ):
+ # The tracking repo is already on disk and the caller opted out of refreshing on
+ # initialize - reuse it as-is and skip all network operations. Fall through to the
+ # full initialization path if the on-disk repos turn out to be unusable. Both repo
+ # handles are still assigned so a later explicit refresh() works as usual.
+ try:
+ repo = Repo(self.repo_path)
+ try:
+ repo.git.checkout(self.tracking_ref)
+ bare_repo = Repo(self.bare_repo_path)
+ except Exception:
+ repo.close()
+ raise
+ except (InvalidGitRepositoryError, NoSuchPathError, GitCommandError) as e:
Review Comment:
Fixed in 08b321e, and the regression test I offered is now in a479772.
It injects a `PermissionError` when the on-disk tracking repo is opened and asserts initialization falls through to the full path instead of propagating. The assertion is deliberately indirect: it checks the bundle ends up on a commit that only the fallback could have picked up (the fallback fetches, the reuse path does not), so the test still fails if the error is swallowed and stale state is served.
--
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