You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "onlyarnav (via GitHub)" <gi...@apache.org> on 2026/07/20 13:25:17 UTC
[PR] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
onlyarnav opened a new pull request, #70130:
URL: https://github.com/apache/airflow/pull/70130
### Description
This PR introduces an option to opt out of forwarding Dag-level parameters in the `DatabricksRunNowOperator`.
In PR #66613 (for issue #39002), parameter forwarding was introduced such that the operator's `params` dict is automatically forwarded as `job_parameters` when no `job_parameters` are specified. However, this changes the payload for Databricks jobs and breaks execution for jobs whose entry points do not expect or accept additional parameters.
To resolve this compatibility issue, this PR:
1. Adds a new boolean parameter `forward_dag_params` (default: `True`) to `DatabricksRunNowOperator`.
2. Checks `self.forward_dag_params` when constructing the payload inside `_build_run_now_payload` before merging `self.params`.
3. Adds unit tests to verify that Dag-level parameters are not forwarded when `forward_dag_params=False`.
4. Fixes minor Windows-specific issues (e.g. file encoding, `os.register_at_fork`, and a mock `fcntl`) to enable executing the local test suite on Windows development hosts.
closes: #70121
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (Google Antigravity)
Generated-by: Google Antigravity following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "onlyarnav (via GitHub)" <gi...@apache.org>.
onlyarnav commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3615207791
##########
scripts/ci/prek/common_prek_utils.py:
##########
Review Comment:
This was also a local environment workaround to prevent UnicodeDecodeError when running pre-commit checks on a Windows machine. I have reverted it from 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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "onlyarnav (via GitHub)" <gi...@apache.org>.
onlyarnav commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3656967557
##########
providers/databricks/tests/unit/databricks/operators/test_databricks.py:
##########
@@ -2925,6 +2925,28 @@ def test_run_now_does_not_override_existing_job_parameters(self, db_mock_class):
actual = db_mock.run_now.call_args.args[0]
assert actual["job_parameters"] == {"explicit": "value"}
+ @mock.patch("airflow.providers.databricks.operators.databricks.DatabricksHook")
+ def test_run_now_does_not_inject_airflow_params_when_forward_dag_params_is_false(self, db_mock_class):
Review Comment:
Done! I've added a parametrized test test_run_now_skips_param_injection_with_legacy_param_slots covering all 6 conflicting parameter slots to verify that job_parameters is not injected when any of them are present
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3615162643
##########
providers/databricks/tests/conftest.py:
##########
@@ -16,4 +16,18 @@
# under the License.
from __future__ import annotations
+import os
+import sys
+
+if os.name == "nt":
+ from types import ModuleType
+
+ fcntl_mock = ModuleType("fcntl")
+ fcntl_mock.LOCK_SH = 1
+ fcntl_mock.LOCK_EX = 2
+ fcntl_mock.LOCK_NB = 4
+ fcntl_mock.LOCK_UN = 8
+ fcntl_mock.flock = lambda fd, operation: None
+ sys.modules["fcntl"] = fcntl_mock
Review Comment:
What does this do?
##########
shared/observability/src/airflow_shared/observability/metrics/stats.py:
##########
@@ -48,7 +48,8 @@ def _reset_backend_after_fork() -> None:
_backend = None
-os.register_at_fork(after_in_child=_reset_backend_after_fork)
+if hasattr(os, "register_at_fork"):
+ os.register_at_fork(after_in_child=_reset_backend_after_fork)
Review Comment:
Same here.
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "onlyarnav (via GitHub)" <gi...@apache.org>.
onlyarnav commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3615222498
##########
providers/databricks/tests/conftest.py:
##########
@@ -16,4 +16,18 @@
# under the License.
from __future__ import annotations
+import os
+import sys
+
+if os.name == "nt":
+ from types import ModuleType
+
+ fcntl_mock = ModuleType("fcntl")
+ fcntl_mock.LOCK_SH = 1
+ fcntl_mock.LOCK_EX = 2
+ fcntl_mock.LOCK_NB = 4
+ fcntl_mock.LOCK_UN = 8
+ fcntl_mock.flock = lambda fd, operation: None
+ sys.modules["fcntl"] = fcntl_mock
Review Comment:
Since fcntl is a POSIX-only module, airflow 3.0+ imports it during test execution setup, which crashes test collection on Windows. This mock was added so the test suite could be run and verified locally on a Windows machine. It was added by Antigravity, but I have reverted it.
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "onlyarnav (via GitHub)" <gi...@apache.org>.
onlyarnav commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3615212982
##########
shared/observability/src/airflow_shared/observability/metrics/stats.py:
##########
@@ -48,7 +48,8 @@ def _reset_backend_after_fork() -> None:
_backend = None
-os.register_at_fork(after_in_child=_reset_backend_after_fork)
+if hasattr(os, "register_at_fork"):
+ os.register_at_fork(after_in_child=_reset_backend_after_fork)
Review Comment:
Reverted
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "moomindani (via GitHub)" <gi...@apache.org>.
moomindani commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3654048500
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -1167,6 +1167,8 @@ class DatabricksRunNowOperator(ResumableJobMixin, BaseOperator):
before polling begins so that a worker crash and retry reconnects to the existing run
instead of triggering a duplicate run of the same job. Set to ``False`` to always trigger a
fresh run on retry. Requires Airflow 3.3+; on earlier versions it is silently ignored.
+ :param forward_dag_params: Whether to forward Dag-level params as ``job_parameters``
Review Comment:
The `.. note::` a few lines below still states forwarding happens unconditionally, so the two now contradict each other. Worth adding the opt-out to the note, e.g. append: "Set ``forward_dag_params=False`` to disable this."
Also worth noting there that `job_parameters` cannot be combined with `notebook_params` / `python_params` / `jar_params` / `spark_submit_params` / `python_named_params` / `dbt_commands` — the API rejects such a run, which is the main reason a user would reach for this flag.
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -1315,7 +1319,7 @@ def _build_run_now_payload(self) -> dict[str, Any]:
json["job_id"] = job_id
del json["job_name"]
- if not json.get("job_parameters") and self.params:
+ if self.forward_dag_params and not json.get("job_parameters") and self.params:
Review Comment:
Consider also skipping injection when a conflicting slot is already present.
Because `forward_dag_params` defaults to `True`, every Dag that is currently broken by #66613 stays broken until its author discovers this new flag — and the failure mode gives no hint that a new operator argument is the fix. The conflict is deterministic and fully knowable from the payload at this point, so it can be repaired automatically:
```python
_RUN_NOW_PARAM_SLOTS_CONFLICTING_WITH_JOB_PARAMETERS = (
"notebook_params",
"python_params",
"jar_params",
"spark_submit_params",
"python_named_params",
"dbt_commands",
)
if (
self.forward_dag_params
and not json.get("job_parameters")
and self.params
and not any(k in json for k in _RUN_NOW_PARAM_SLOTS_CONFLICTING_WITH_JOB_PARAMETERS)
):
json["job_parameters"] = dict(self.params)
```
That fixes the regression for existing users without requiring them to change anything, and `forward_dag_params=False` then covers the remaining case the issue describes — a job whose entry point rejects extra params even when there is no slot conflict.
I'm happy to be argued out of this if maintainers prefer to keep the change minimal and purely opt-in; the silent-failure-by-default aspect is what makes me raise it.
##########
providers/databricks/tests/unit/databricks/operators/test_databricks.py:
##########
@@ -2925,6 +2925,28 @@ def test_run_now_does_not_override_existing_job_parameters(self, db_mock_class):
actual = db_mock.run_now.call_args.args[0]
assert actual["job_parameters"] == {"explicit": "value"}
+ @mock.patch("airflow.providers.databricks.operators.databricks.DatabricksHook")
+ def test_run_now_does_not_inject_airflow_params_when_forward_dag_params_is_false(self, db_mock_class):
Review Comment:
The test correctly covers the flag's mechanics (I verified it fails without the guard). What isn't covered is the scenario that actually motivates the change: Dag-level `params` combined with one of the legacy param slots, which is what the API rejects. Nothing in the existing suite exercises that combination.
Worth adding a case asserting that with `notebook_params` set and `forward_dag_params=False`, the payload contains `notebook_params` and no `job_parameters` — that pins the regression scenario rather than just the flag. If you take the auto-skip suggestion on `_build_run_now_payload`, the same case parametrized over the six conflicting slots would cover it directly.
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "onlyarnav (via GitHub)" <gi...@apache.org>.
onlyarnav commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3615204535
##########
shared/observability/src/airflow_shared/observability/metrics/stats.py:
##########
Review Comment:
This change was originally added as a local environment workaround to prevent startup crashes when executing the tests on a Windows host (since os.register_at_fork is POSIX-only). I have reverted it.
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "jroachgolf84 (via GitHub)" <gi...@apache.org>.
jroachgolf84 commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3614847210
##########
providers/databricks/tests/conftest.py:
##########
Review Comment:
What are these changes for? Why are they needed?
##########
scripts/ci/prek/common_prek_utils.py:
##########
Review Comment:
Should this change be part of this PR?
##########
shared/observability/src/airflow_shared/observability/metrics/stats.py:
##########
Review Comment:
Should this change be part of 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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "onlyarnav (via GitHub)" <gi...@apache.org>.
onlyarnav commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3656974482
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -1315,7 +1319,7 @@ def _build_run_now_payload(self) -> dict[str, Any]:
json["job_id"] = job_id
del json["job_name"]
- if not json.get("job_parameters") and self.params:
+ if self.forward_dag_params and not json.get("job_parameters") and self.params:
Review Comment:
I've updated _build_run_now_payload to define _RUN_NOW_PARAM_SLOTS_CONFLICTING_WITH_JOB_PARAMETERS and automatically skip auto-injecting job_parameters whenever any of the 6 legacy parameter slots are present in json
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "onlyarnav (via GitHub)" <gi...@apache.org>.
onlyarnav commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3615224754
##########
providers/databricks/tests/conftest.py:
##########
Review Comment:
Since fcntl is a POSIX-only module, airflow 3.0+ imports it during test execution setup, which crashes test collection on Windows hosts. This mock was added so the test suite could be run and verified locally on a Windows machine. It was added by Antigravity, but I have reverted it.
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #70130:
URL: https://github.com/apache/airflow/pull/70130#issuecomment-5042678469
cc @moomindani for a review from Databricks team
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "onlyarnav (via GitHub)" <gi...@apache.org>.
onlyarnav commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3656962848
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -1167,6 +1167,8 @@ class DatabricksRunNowOperator(ResumableJobMixin, BaseOperator):
before polling begins so that a worker crash and retry reconnects to the existing run
instead of triggering a duplicate run of the same job. Set to ``False`` to always trigger a
fresh run on retry. Requires Airflow 3.3+; on earlier versions it is silently ignored.
+ :param forward_dag_params: Whether to forward Dag-level params as ``job_parameters``
Review Comment:
Updated! I've updated the .. note:: in the DatabricksRunNowOperator docstring as well as the user documentation in providers/databricks/docs/operators/run_now.rst to explain forward_dag_params=False and document the Databricks API conflict with the legacy parameter slots
--
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] Providers: Allow opting out of forwarding Dag-level parameters in DatabricksRunNowOperator [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #70130:
URL: https://github.com/apache/airflow/pull/70130
--
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