You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "codewithsruthi (via GitHub)" <gi...@apache.org> on 2026/08/25 23:42:31 UTC
[PR] Move DataprocSubmitJobOperator start-trigger kwargs out of __init__ [airflow]
codewithsruthi opened a new pull request, #72083:
URL: https://github.com/apache/airflow/pull/72083
Template fields must not be read in an operator constructor. `DataprocSubmitJobOperator` still needs those values copied into `start_trigger_args` so `start_from_trigger` can skip the worker and let the triggerer render them.
This PR builds those kwargs in a helper, replaces the class-level `start_trigger_args` per task, and removes that one exemption.
related: #70296
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Cursor Grok 4.6
Generated-by: Cursor Grok 4.6 following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
Made with [Cursor](https://cursor.com)
--
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] Move DataprocSubmitJobOperator start-trigger kwargs out of __init__ [airflow]
Posted by "codewithsruthi (via GitHub)" <gi...@apache.org>.
codewithsruthi commented on code in PR #72083:
URL: https://github.com/apache/airflow/pull/72083#discussion_r3962931657
##########
providers/google/tests/unit/google/cloud/operators/test_dataproc.py:
##########
@@ -2521,6 +2521,21 @@ def test_start_from_trigger_without_deferrable_does_not_set_args(self):
assert op.start_from_trigger is True
assert op.start_trigger_args.trigger_kwargs == {}
+ def test_start_from_trigger_stores_templated_fields_verbatim(self):
Review Comment:
Agreed — that test also passed on `main`, so it did not lock in this change. I dropped it. The existing `start_from_trigger` cases still cover `trigger_kwargs`, and the exemption removal is what the constructor-logic hook actually checks.
##########
providers/google/src/airflow/providers/google/cloud/operators/dataproc.py:
##########
@@ -2005,23 +2005,26 @@ def __init__(
self.openlineage_inject_transport_info = openlineage_inject_transport_info
if self.deferrable and self.start_from_trigger:
- self.start_trigger_args = StartTriggerArgs(
- trigger_cls="airflow.providers.google.cloud.triggers.dataproc.DataprocSubmitJobDirectTrigger",
- trigger_kwargs={
- "job": self.job,
- "project_id": self.project_id,
- "region": self.region,
- "gcp_conn_id": self.gcp_conn_id,
- "impersonation_chain": self.impersonation_chain,
- "polling_interval_seconds": self.polling_interval_seconds,
- "cancel_on_kill": self.cancel_on_kill,
- "request_id": self.request_id,
- },
- next_method="execute_complete",
- next_kwargs=None,
- timeout=None,
+ # Replaced rather than mutated: ``start_trigger_args`` is a class attribute, so
+ # assigning through it would overwrite the arguments of every other task built
+ # from this operator.
+ self.start_trigger_args = replace(
+ self.start_trigger_args,
+ trigger_kwargs=self.build_direct_submit_trigger_kwargs(),
)
+ def build_direct_submit_trigger_kwargs(self) -> dict[str, Any]:
Review Comment:
Agreed. Renamed it to `_build_direct_submit_trigger_kwargs` and added a short comment that it stays outside `__init__` so the constructor-logic hook does not see the template-field reads, while `start_from_trigger` still needs the copy at construct time.
##########
providers/google/src/airflow/providers/google/cloud/operators/dataproc.py:
##########
@@ -2005,23 +2005,26 @@ def __init__(
self.openlineage_inject_transport_info = openlineage_inject_transport_info
if self.deferrable and self.start_from_trigger:
- self.start_trigger_args = StartTriggerArgs(
- trigger_cls="airflow.providers.google.cloud.triggers.dataproc.DataprocSubmitJobDirectTrigger",
- trigger_kwargs={
- "job": self.job,
- "project_id": self.project_id,
- "region": self.region,
- "gcp_conn_id": self.gcp_conn_id,
- "impersonation_chain": self.impersonation_chain,
- "polling_interval_seconds": self.polling_interval_seconds,
- "cancel_on_kill": self.cancel_on_kill,
- "request_id": self.request_id,
- },
- next_method="execute_complete",
- next_kwargs=None,
- timeout=None,
+ # Replaced rather than mutated: ``start_trigger_args`` is a class attribute, so
Review Comment:
Agreed — the previous code already allocated a fresh `StartTriggerArgs`, so there was no cross-task leak to fix. I reworded the PR description. Left the in-code comment so it stays consistent with the other `start_from_trigger` operators.
--
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] Move DataprocSubmitJobOperator start-trigger kwargs out of __init__ [airflow]
Posted by "rjgoyln (via GitHub)" <gi...@apache.org>.
rjgoyln commented on code in PR #72083:
URL: https://github.com/apache/airflow/pull/72083#discussion_r3955844616
##########
providers/google/src/airflow/providers/google/cloud/operators/dataproc.py:
##########
@@ -2005,23 +2005,26 @@ def __init__(
self.openlineage_inject_transport_info = openlineage_inject_transport_info
if self.deferrable and self.start_from_trigger:
- self.start_trigger_args = StartTriggerArgs(
- trigger_cls="airflow.providers.google.cloud.triggers.dataproc.DataprocSubmitJobDirectTrigger",
- trigger_kwargs={
- "job": self.job,
- "project_id": self.project_id,
- "region": self.region,
- "gcp_conn_id": self.gcp_conn_id,
- "impersonation_chain": self.impersonation_chain,
- "polling_interval_seconds": self.polling_interval_seconds,
- "cancel_on_kill": self.cancel_on_kill,
- "request_id": self.request_id,
- },
- next_method="execute_complete",
- next_kwargs=None,
- timeout=None,
+ # Replaced rather than mutated: ``start_trigger_args`` is a class attribute, so
+ # assigning through it would overwrite the arguments of every other task built
+ # from this operator.
+ self.start_trigger_args = replace(
+ self.start_trigger_args,
+ trigger_kwargs=self.build_direct_submit_trigger_kwargs(),
)
+ def build_direct_submit_trigger_kwargs(self) -> dict[str, Any]:
Review Comment:
Could we make the helper private (`_build_direct_submit_trigger_kwargs`) since it’s an implementation detail?
Also, a short comment explaining why it stays outside `__init__` would be helpful, since the reads still intentionally happen at construction time to satisfy `start_from_trigger`.
##########
providers/google/tests/unit/google/cloud/operators/test_dataproc.py:
##########
@@ -2521,6 +2521,21 @@ def test_start_from_trigger_without_deferrable_does_not_set_args(self):
assert op.start_from_trigger is True
assert op.start_trigger_args.trigger_kwargs == {}
+ def test_start_from_trigger_stores_templated_fields_verbatim(self):
Review Comment:
This test also passes on `main`, so it cannot be failed by reverting the change. The existing test above already covers `trigger_kwargs`, including `job` and `project_id`.
Since this is a pure refactor, could we either drop this test or repoint it to cover the `replace()` behaviour: preserving `next_method` / `next_kwargs` from the class attribute and leaving the class attribute unchanged?
##########
providers/google/src/airflow/providers/google/cloud/operators/dataproc.py:
##########
@@ -2005,23 +2005,26 @@ def __init__(
self.openlineage_inject_transport_info = openlineage_inject_transport_info
if self.deferrable and self.start_from_trigger:
- self.start_trigger_args = StartTriggerArgs(
- trigger_cls="airflow.providers.google.cloud.triggers.dataproc.DataprocSubmitJobDirectTrigger",
- trigger_kwargs={
- "job": self.job,
- "project_id": self.project_id,
- "region": self.region,
- "gcp_conn_id": self.gcp_conn_id,
- "impersonation_chain": self.impersonation_chain,
- "polling_interval_seconds": self.polling_interval_seconds,
- "cancel_on_kill": self.cancel_on_kill,
- "request_id": self.request_id,
- },
- next_method="execute_complete",
- next_kwargs=None,
- timeout=None,
+ # Replaced rather than mutated: ``start_trigger_args`` is a class attribute, so
Review Comment:
Keeping the comment for consistency is fine, but could we reword the PR description? The previous code already created a fresh `StartTriggerArgs`, so there was no cross-task leakage. The current wording may imply this PR fixed a bug that did not exist 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] Move DataprocSubmitJobOperator start-trigger kwargs out of __init__ [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 closed pull request #72083: Move DataprocSubmitJobOperator start-trigger kwargs out of __init__
URL: https://github.com/apache/airflow/pull/72083
--
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] Move DataprocSubmitJobOperator start-trigger kwargs out of __init__ [airflow]
Posted by "rjgoyln (via GitHub)" <gi...@apache.org>.
rjgoyln commented on PR #72083:
URL: https://github.com/apache/airflow/pull/72083#issuecomment-5582229712
LGTM overall! The refactor looks behaviour-preserving.
One concern: the new test would also pass on main, so it cannot fail when reverting the change. Could we either remove it or adjust it to specifically cover the replace() behaviour?
One minor point: please consider adding a short comment explaining why the helper is outside __init__.
Just my thoughts — feel free to resolve if you think otherwise.
--
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] Move DataprocSubmitJobOperator start-trigger kwargs out of __init__ [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #72083:
URL: https://github.com/apache/airflow/pull/72083#issuecomment-5648097981
After some investigation, this turned out to be a false positive in the pre-commit hook rather than something the operator can fix: with `start_from_trigger` the triggerer renders these fields itself, so the un-rendered copy in `__init__` is the intended flow. Moving the reads into a helper called from `__init__` only hides them from the hook; they still run at construct time.
The better approach, in my opinion, is to teach the hook about this case, which I did in #73040.
I'll close this one in favor of that, thanks for your contribution!
--
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