Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
Lee-W merged PR #45961: URL: https://github.com/apache/airflow/pull/45961 -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1969697018 ## airflow/api_fastapi/execution_api/datamodels/taskinstance.py: ## @@ -230,7 +230,6 @@ class DagRun(StrictBaseModel): clear_number: int run_type: DagRunType conf: Annotated[dict[str, Any], Field(default_factory=dict)] -external_trigger: bool = False Review Comment: I searched for `external_trigger` in `task_sdk/src/airflow/sdk/api/datamodels/_generated.py` and it looks like I have already removed it. https://github.com/apache/airflow/pull/45961/files#diff-faa54b815ea1397c8bf23ba8017a4d39e01ae0d2134ac36d40cfebb6e9bdfb0fL302 -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1969697018 ## airflow/api_fastapi/execution_api/datamodels/taskinstance.py: ## @@ -230,7 +230,6 @@ class DagRun(StrictBaseModel): clear_number: int run_type: DagRunType conf: Annotated[dict[str, Any], Field(default_factory=dict)] -external_trigger: bool = False Review Comment: I searched for `external_trigger` in `task_sdk/src/airflow/sdk/api/datamodels/_generated.py` and it looks like I have already removed 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1969541461 ## airflow/api_fastapi/execution_api/datamodels/taskinstance.py: ## @@ -230,7 +230,6 @@ class DagRun(StrictBaseModel): clear_number: int run_type: DagRunType conf: Annotated[dict[str, Any], Field(default_factory=dict)] -external_trigger: bool = False Review Comment: Sure, I will be available to fix about in a hour. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
ashb commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1969486439 ## airflow/api_fastapi/execution_api/datamodels/taskinstance.py: ## @@ -230,7 +230,6 @@ class DagRun(StrictBaseModel): clear_number: int run_type: DagRunType conf: Annotated[dict[str, Any], Field(default_factory=dict)] -external_trigger: bool = False Review Comment: This will need a comparable change made into `task_sdk/src/airflow/sdk/api/datamodels/_generated.py` (It's not automated yet, that'll be done in #47026 but for now please just manually edit that file. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1969305642 ## newsfragments/45961.significant.rst: ## @@ -0,0 +1,13 @@ +Replace the ``external_trigger`` check with ``DagRunType``, and update any logic that relies on ``external_trigger`` to use ``run_type`` instead. + + +* Types of change + + * [ ] Dag changes + * [ ] Config changes + * [ ] API changes + * [ ] CLI changes + * [ ] Behaviour changes + * [ ] Plugin changes + * [ ] Dependency changes + * [x] Code interface changes Review Comment: Thanks! I just took a look at the commit. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
Lee-W commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1969277848 ## newsfragments/45961.significant.rst: ## @@ -0,0 +1,13 @@ +Replace the ``external_trigger`` check with ``DagRunType``, and update any logic that relies on ``external_trigger`` to use ``run_type`` instead. + + +* Types of change + + * [ ] Dag changes + * [ ] Config changes + * [ ] API changes + * [ ] CLI changes + * [ ] Behaviour changes + * [ ] Plugin changes + * [ ] Dependency changes + * [x] Code interface changes Review Comment: just updated it. you can take a look at what I have added -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1969264193 ## newsfragments/45961.significant.rst: ## @@ -0,0 +1,13 @@ +Replace the ``external_trigger`` check with ``DagRunType``, and update any logic that relies on ``external_trigger`` to use ``run_type`` instead. + + +* Types of change + + * [ ] Dag changes + * [ ] Config changes + * [ ] API changes + * [ ] CLI changes + * [ ] Behaviour changes + * [ ] Plugin changes + * [ ] Dependency changes + * [x] Code interface changes Review Comment: > Oops, missing migration rules here. Hi @Lee-W, do you mean we should add example for migrating `external_trigger` with `DagRunType` ? I have run `check-significant-newsfragments-are-valid` pre-commit successfully. ## newsfragments/45961.significant.rst: ## @@ -0,0 +1,13 @@ +Replace the ``external_trigger`` check with ``DagRunType``, and update any logic that relies on ``external_trigger`` to use ``run_type`` instead. + + +* Types of change + + * [ ] Dag changes + * [ ] Config changes + * [ ] API changes + * [ ] CLI changes + * [ ] Behaviour changes + * [ ] Plugin changes + * [ ] Dependency changes + * [x] Code interface changes Review Comment: > Oops, missing migration rules here. Hi @Lee-W, do you mean we should add example for migrating `external_trigger` with `DagRunType` ? I have run `check-significant-newsfragments-are-valid` pre-commit successfully. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
Lee-W commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1969261401 ## newsfragments/45961.significant.rst: ## @@ -0,0 +1,13 @@ +Replace the ``external_trigger`` check with ``DagRunType``, and update any logic that relies on ``external_trigger`` to use ``run_type`` instead. + + +* Types of change + + * [ ] Dag changes + * [ ] Config changes + * [ ] API changes Review Comment: It also contains API and CLI changes -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
Lee-W commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1969255191 ## newsfragments/45961.significant.rst: ## @@ -0,0 +1,13 @@ +Replace the ``external_trigger`` check with ``DagRunType``, and update any logic that relies on ``external_trigger`` to use ``run_type`` instead. + + +* Types of change + + * [ ] Dag changes + * [ ] Config changes + * [ ] API changes + * [ ] CLI changes + * [ ] Behaviour changes + * [ ] Plugin changes + * [ ] Dependency changes + * [x] Code interface changes Review Comment: Oops, missing migration rules 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on PR #45961: URL: https://github.com/apache/airflow/pull/45961#issuecomment-268436 I added a commit that should fix the static checks. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on PR #45961: URL: https://github.com/apache/airflow/pull/45961#issuecomment-2681096023 Not unrelated, those lines reference the removed `external_trigger` field. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on PR #45961: URL: https://github.com/apache/airflow/pull/45961#issuecomment-2676166777 Only non-related `ts-compile-format-lint-ui` static check fail 🎉 -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1964628811 ## providers/openlineage/tests/unit/openlineage/plugins/test_adapter.py: ## @@ -605,9 +606,8 @@ def test_emit_dag_started_event(mock_stats_incr, mock_stats_timer, generate_stat "dag_id": "dag_id", "data_interval_start": event_time.isoformat(), "data_interval_end": event_time.isoformat(), -"external_trigger": False if AIRFLOW_V_3_0_PLUS else None, "run_id": run_id, -"run_type": None, +"run_type": DagRunType.MANUAL if AIRFLOW_V_3_0_PLUS else None, Review Comment: I'm not really sure about _will run_type will be when external_trigger is False_ , but based on https://github.com/apache/airflow/pull/45961#discussion_r1928395914 we don't need to check airflow version when asserting run_type. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961:
URL: https://github.com/apache/airflow/pull/45961#discussion_r1964626098
##
providers/openlineage/tests/unit/openlineage/plugins/test_adapter.py:
##
@@ -605,9 +606,8 @@ def test_emit_dag_started_event(mock_stats_incr,
mock_stats_timer, generate_stat
"dag_id": "dag_id",
"data_interval_start": event_time.isoformat(),
"data_interval_end": event_time.isoformat(),
-"external_trigger": False if AIRFLOW_V_3_0_PLUS else None,
"run_id": run_id,
-"run_type": None,
+"run_type": DagRunType.MANUAL if AIRFLOW_V_3_0_PLUS else None,
Review Comment:
Thanks for the reminder, just resolved.
##
providers/standard/src/airflow/providers/standard/operators/latest_only.py:
##
@@ -53,8 +54,8 @@ def choose_branch(self, context: Context) -> str |
Iterable[str]:
# If the DAG Run is externally triggered, then return without
# skipping downstream tasks
dag_run: DagRun = context["dag_run"] # type: ignore[assignment]
-if dag_run.external_trigger:
-self.log.info("Externally triggered DAG_Run: allowing execution to
proceed.")
+if dag_run.run_type != DagRunType.SCHEDULED:
Review Comment:
Thanks for the reminder, just resolved.
--
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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1964626098 ## providers/openlineage/tests/unit/openlineage/plugins/test_adapter.py: ## @@ -605,9 +606,8 @@ def test_emit_dag_started_event(mock_stats_incr, mock_stats_timer, generate_stat "dag_id": "dag_id", "data_interval_start": event_time.isoformat(), "data_interval_end": event_time.isoformat(), -"external_trigger": False if AIRFLOW_V_3_0_PLUS else None, "run_id": run_id, -"run_type": None, +"run_type": DagRunType.MANUAL if AIRFLOW_V_3_0_PLUS else None, Review Comment: Thanks for the reminder, just resolved. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1964598864 ## airflow/utils/types.py: ## @@ -57,6 +57,11 @@ def from_run_id(run_id: str) -> DagRunType: return DagRunType.MANUAL +externally_triggered_type: frozenset[DagRunType] = frozenset( +[DagRunType.BACKFILL_JOB, DagRunType.MANUAL, DagRunType.ASSET_TRIGGERED] +) Review Comment: This is not currently used, I will remove it. It came up during the discussion above, but instead of introducing `externally_triggered_type`, simply comparing `run_type != DagRunType.SCHEDULED` is a much cleaner approach. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
Lee-W commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1962955761 ## newsfragments/45961.significant.rst: ## Review Comment: Yep, both should be easy to add. We decided we don't want to handle REST API as there are way too many ways to interact with 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1962923926 ## newsfragments/45961.significant.rst: ## Review Comment: I think the only user-facing interface Ruff can cover are 1. DAG.create_dagrun. You used to be able to pass in `external_trigger=True` (or False) and that’s set on the created DagRun row. This is now removed. 2. When accessing a DagRun (in context; I don’t think there are other officially supported ways?) there used to be an attribute `external_trigger` that’s now gone. `external_trigger=True` basically means `run_type="manual"`. There are also REST API changes but I don’t think we have a way to detect those. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1962923926 ## newsfragments/45961.significant.rst: ## Review Comment: I think the only user-facing interface Ruff can cover are 1. DAG.create_dagrun. You used to be able to pass in `external_trigger=True` (or False) and that’s set on the created DagRun row. This is now removed. 2. When accessing a DagRun (likely only in context?) there used to be an attribute `external_trigger` that’s now gone. `external_trigger=True` basically means `run_type="manual"`. There are also REST API changes but I don’t think we have a way to detect those. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
Lee-W commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1962903985 ## newsfragments/45961.significant.rst: ## Review Comment: Yep, I think so. It's not something that has been moved to providers or missing args. AIR302 should be able to be covered. Would be even better if @jason810496 can provide us with an example so that I can have a better idea on how this should be migrated -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1962870548 ## providers/openlineage/tests/unit/openlineage/plugins/test_adapter.py: ## @@ -605,9 +606,8 @@ def test_emit_dag_started_event(mock_stats_incr, mock_stats_timer, generate_stat "dag_id": "dag_id", "data_interval_start": event_time.isoformat(), "data_interval_end": event_time.isoformat(), -"external_trigger": False if AIRFLOW_V_3_0_PLUS else None, "run_id": run_id, -"run_type": None, +"run_type": DagRunType.MANUAL if AIRFLOW_V_3_0_PLUS else None, Review Comment: Does this run_type need to be different from different versions? Especially since external_trigger is False, should this always be SCHEDULED instead? -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1962869448 ## newsfragments/45961.significant.rst: ## Review Comment: I think this change should be covered by AIR302? cc @Lee-W -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on code in PR #45961:
URL: https://github.com/apache/airflow/pull/45961#discussion_r1962578093
##
providers/standard/src/airflow/providers/standard/operators/latest_only.py:
##
@@ -53,8 +54,8 @@ def choose_branch(self, context: Context) -> str |
Iterable[str]:
# If the DAG Run is externally triggered, then return without
# skipping downstream tasks
dag_run: DagRun = context["dag_run"] # type: ignore[assignment]
-if dag_run.external_trigger:
-self.log.info("Externally triggered DAG_Run: allowing execution to
proceed.")
+if dag_run.run_type != DagRunType.SCHEDULED:
Review Comment:
Should this check against MANUAL instead?
--
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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1961984309 ## airflow/utils/types.py: ## @@ -57,6 +57,11 @@ def from_run_id(run_id: str) -> DagRunType: return DagRunType.MANUAL +externally_triggered_type: frozenset[DagRunType] = frozenset( +[DagRunType.BACKFILL_JOB, DagRunType.MANUAL, DagRunType.ASSET_TRIGGERED] +) Review Comment: Where is this used? (Also the name should be plural.) -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on code in PR #45961:
URL: https://github.com/apache/airflow/pull/45961#discussion_r1961980426
##
airflow/migrations/versions/0059_3_0_0_remove_external_trigger_field.py:
##
@@ -0,0 +1,49 @@
+#
+# 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.
+
+"""
+remove external_trigger field.
+
+Revision ID: e00344393f31
+Revises: 6a9e7a527a88
+Create Date: 2025-02-04 16:45:49.068781
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+
+revision = "e00344393f31"
+down_revision = "6a9e7a527a88"
+branch_labels = None
+depends_on = None
+airflow_version = "3.0.0"
+
+
+def upgrade():
+"""Apply remove external_trigger field."""
+with op.batch_alter_table("dag_run", schema=None) as batch_op:
+batch_op.drop_column("external_trigger")
+
+
+def downgrade():
+"""Unapply remove external_trigger field."""
+with op.batch_alter_table("dag_run", schema=None) as batch_op:
+batch_op.add_column(sa.Column("external_trigger", sa.BOOLEAN(),
autoincrement=False, nullable=True))
Review Comment:
This should try to recreate the original external_trigger value using
run_type.
##
airflow/migrations/versions/0059_3_0_0_remove_external_trigger_field.py:
##
@@ -0,0 +1,49 @@
+#
+# 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.
+
+"""
+remove external_trigger field.
+
+Revision ID: e00344393f31
+Revises: 6a9e7a527a88
+Create Date: 2025-02-04 16:45:49.068781
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+
+revision = "e00344393f31"
+down_revision = "6a9e7a527a88"
+branch_labels = None
+depends_on = None
+airflow_version = "3.0.0"
+
+
+def upgrade():
+"""Apply remove external_trigger field."""
+with op.batch_alter_table("dag_run", schema=None) as batch_op:
+batch_op.drop_column("external_trigger")
+
+
+def downgrade():
+"""Unapply remove external_trigger field."""
+with op.batch_alter_table("dag_run", schema=None) as batch_op:
+batch_op.add_column(sa.Column("external_trigger", sa.BOOLEAN(),
autoincrement=False, nullable=True))
Review Comment:
Downgrade should try to recreate the original external_trigger value using
run_type.
--
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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1957683871 ## newsfragments/45961.significant.rst: ## @@ -0,0 +1,12 @@ +Replace ``external_trigger`` check with ``DagRunType`` Review Comment: We should mention if the user currently relies on `external_trigger`, they can replace it by checking `run_type`. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on PR #45961: URL: https://github.com/apache/airflow/pull/45961#issuecomment-2646110394 The only failures are caused by: - UI static check - Compatibility tests for `2.9.3:P3.9` and `2.10.4:P3.10` providers (likely broken due to the new provider structure?) - Flaky test: `TestWorkflowTrigger.test_task_workflow_trigger_skipped` Do we need to fix the compatibility tests, or is this PR ready to be merged? -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
uranusjr commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1939015080 ## airflow/models/dagrun.py: ## @@ -1227,11 +1217,11 @@ def _emit_true_scheduling_delay_stats_for_finished_state(self, finished_tis: lis rid of the outliers on the stats side through dashboards tooling. Note that the stat will only be emitted for scheduler-triggered DAG runs -(i.e. when ``external_trigger`` is *False* and ``clear_number`` is equal to 0). +(i.e. when ``run_type`` is *MANUAL* and ``clear_number`` is equal to 0). Review Comment: I dug into the history and yes it seems that `external_trigger` is *False* when `run_type` is set to `BACKFILL_JOB`. Also, `external_trigger` predates both `run_type` and `triggered_by` by a lot, its true definition is quite ambiguous and lost to time, so we can probably take some liberty changing its definition as long as the new implementation makes sense and does not cause bugs. I think it’d be reasonable enough to define `external_trigger == True` as equivalent to `run_type == "manual"` for refactoring purposes. If this causes bugs, they should be fixed under this definition. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1932460386 ## airflow/models/dagrun.py: ## @@ -1227,11 +1217,11 @@ def _emit_true_scheduling_delay_stats_for_finished_state(self, finished_tis: lis rid of the outliers on the stats side through dashboards tooling. Note that the stat will only be emitted for scheduler-triggered DAG runs -(i.e. when ``external_trigger`` is *False* and ``clear_number`` is equal to 0). +(i.e. when ``run_type`` is *MANUAL* and ``clear_number`` is equal to 0). Review Comment: After looking deeper, I think `external_trigger is False` is equivalent to `run_type is SCHEDULED` and should fit this case. -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
dstandish commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1931000403 ## providers/src/airflow/providers/standard/operators/latest_only.py: ## @@ -53,7 +54,7 @@ def choose_branch(self, context: Context) -> str | Iterable[str]: # If the DAG Run is externally triggered, then return without # skipping downstream tasks dag_run: DagRun = context["dag_run"] # type: ignore[assignment] -if dag_run.external_trigger: +if dag_run.run_type == DagRunType.MANUAL: Review Comment: so this is about short circuiting the "latest only check" when dag is externally-triggered. I don't think it makes a lot of sense to short circuit this when backfilling since, isn't that the point of the latest only operator? -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
ashb commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1929088059 ## airflow/models/dagrun.py: ## @@ -1227,11 +1217,11 @@ def _emit_true_scheduling_delay_stats_for_finished_state(self, finished_tis: lis rid of the outliers on the stats side through dashboards tooling. Note that the stat will only be emitted for scheduler-triggered DAG runs -(i.e. when ``external_trigger`` is *False* and ``clear_number`` is equal to 0). +(i.e. when ``run_type`` is *MANUAL* and ``clear_number`` is equal to 0). Review Comment: I think we need to look at what the intent here was, and how that maps on to the new dag run types. My guess is that when "external_trigger" was added, there was either scheduled, or external. So Manual is clearly not scheduled, but what abouty Backfill runs? How should we treat those. And for that I delegate to @uranusjr and @dstandish *boing flip* 😄 -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1928447187 ## airflow/models/dagrun.py: ## @@ -1227,11 +1217,11 @@ def _emit_true_scheduling_delay_stats_for_finished_state(self, finished_tis: lis rid of the outliers on the stats side through dashboards tooling. Note that the stat will only be emitted for scheduler-triggered DAG runs -(i.e. when ``external_trigger`` is *False* and ``clear_number`` is equal to 0). +(i.e. when ``run_type`` is *MANUAL* and ``clear_number`` is equal to 0). Review Comment: How about adding `externally_triggered_type` in `airflow.utils.types` ? ```python externally_triggered_type: frozenset[DagRunType] = frozenset([DagRunType.BACKFILL_JOB, DagRunType.MANUAL, DagRunType.ASSET_TRIGGERED]) ``` -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
jason810496 commented on code in PR #45961: URL: https://github.com/apache/airflow/pull/45961#discussion_r1928447187 ## airflow/models/dagrun.py: ## @@ -1227,11 +1217,11 @@ def _emit_true_scheduling_delay_stats_for_finished_state(self, finished_tis: lis rid of the outliers on the stats side through dashboards tooling. Note that the stat will only be emitted for scheduler-triggered DAG runs -(i.e. when ``external_trigger`` is *False* and ``clear_number`` is equal to 0). +(i.e. when ``run_type`` is *MANUAL* and ``clear_number`` is equal to 0). Review Comment: How about adding an externally_triggered_type in airflow.utils.types? Instead of checking `run_type == DagRunType.MANUAL`, we could use `run_type in externally_triggered_type`. WDYT? ```python externally_triggered_type: frozenset[DagRunType] = frozenset([DagRunType.BACKFILL_JOB, DagRunType.MANUAL, DagRunType.ASSET_TRIGGERED]) ``` -- 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]
Re: [PR] Replace `external_trigger` check with DagRunType [airflow]
ashb commented on code in PR #45961:
URL: https://github.com/apache/airflow/pull/45961#discussion_r1928387183
##
airflow/jobs/scheduler_job_runner.py:
##
@@ -1476,14 +1476,18 @@ def _update_state(dag: DAG, dag_run: DagRun):
dag_run.state = DagRunState.RUNNING
dag_run.start_date = timezone.utcnow()
-if dag.timetable.periodic and not dag_run.external_trigger and
dag_run.clear_number < 1:
+if (
+dag.timetable.periodic
+and not dag_run.run_type == DagRunType.MANUAL
Review Comment:
```suggestion
and dag_run.run_type != DagRunType.MANUAL
```
##
airflow/utils/db_cleanup.py:
##
@@ -107,9 +107,9 @@ def readable_config(self):
_TableConfig(
table_name="dag_run",
recency_column_name="start_date",
-extra_columns=["dag_id", "external_trigger"],
+extra_columns=["dag_id", "run_type"],
keep_last=True,
-keep_last_filters=[column("external_trigger") == false()],
+keep_last_filters=[column("run_type") != "manual"],
Review Comment:
We shouldn't use a string literal here but the enum constant please.
##
airflow/example_dags/plugins/event_listener.py:
##
@@ -147,9 +147,8 @@ def on_dag_run_failed(dag_run: DagRun, msg: str):
print("Dag run in failure state")
dag_id = dag_run.dag_id
run_id = dag_run.run_id
-external_trigger = dag_run.external_trigger
-print(f"Dag information:{dag_id} Run id: {run_id} external trigger:
{external_trigger}")
+print(f"Dag information:{dag_id} Run id: {run_id}")
Review Comment:
Not that it _matters_, but lets include the run_type here in its place.
##
airflow/migrations/versions/0056_3_0_0_remove_external_trigger_field.py:
##
@@ -0,0 +1,56 @@
+#
+# 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.
+
+"""
+remove external_trigger field.
+
+Revision ID: fbe8516980a3
+Revises: e39a26ac59f6
+Create Date: 2025-01-23 09:53:31.283015
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "fbe8516980a3"
+down_revision = "e39a26ac59f6"
+branch_labels = None
+depends_on = None
+airflow_version = "3.0.0"
+
+
+def upgrade():
+"""Apply remove external_trigger field."""
+# ### commands auto generated by Alembic - please adjust! ###
+with op.batch_alter_table("dag_run", schema=None) as batch_op:
+batch_op.drop_column("external_trigger")
+
+# ### end Alembic commands ###
+
+
+def downgrade():
+"""Unapply remove external_trigger field."""
+# ### commands auto generated by Alembic - please adjust! ###
+with op.batch_alter_table("dag_run", schema=None) as batch_op:
+batch_op.add_column(sa.Column("external_trigger", sa.BOOLEAN(),
autoincrement=False, nullable=True))
+
+# ### end Alembic commands ###
Review Comment:
```suggestion
```
##
airflow/migrations/versions/0056_3_0_0_remove_external_trigger_field.py:
##
@@ -0,0 +1,56 @@
+#
+# 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.
+
+"""
+remove external_trigger field.
+
+Revision ID: fbe8516980a3
+Revises: e39a26ac59f6
+Create Date: 2025-01-23 09:53:31.283015
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "fbe8516980a3"
+down_revision = "e39a26ac5
