Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
uranusjr merged PR #46390: URL: https://github.com/apache/airflow/pull/46390 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
Lee-W commented on PR #46390: URL: https://github.com/apache/airflow/pull/46390#issuecomment-2648247169 > > @uranusjr I was testing this and found that the DAG run is being inserted into the `dagrun` table with a None logical date, but it remains in the queued state. > > This would be fixed bt @wei in #46460 ah... my ID is `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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
Lee-W commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1949219811
##
tests/api_fastapi/core_api/routes/public/test_dag_run.py:
##
@@ -1165,6 +1160,7 @@ def test_should_respond_200(
f"/public/dags/{DAG1_ID}/dagRuns",
json=request_json,
)
+print(f"body is {response.json()}")
Review Comment:
```suggestion
```
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1949198397
##
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##
@@ -354,7 +356,8 @@ def trigger_dag_run(
f"DAG with dag_id: '{dag_id}' has import errors and cannot be
triggered",
)
-logical_date = pendulum.instance(body.logical_date)
+logical_date = timezone.coerce_datetime(body.logical_date)
Review Comment:
`coerce_datetime ` will handle none
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 commented on PR #46390: URL: https://github.com/apache/airflow/pull/46390#issuecomment-2648220375 > @uranusjr I was testing this and found that the DAG run is being inserted into the `dagrun` table with a None logical date, but it remains in the queued state. This would be fixed bt @wei in https://github.com/apache/airflow/pull/46460 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 commented on PR #46390: URL: https://github.com/apache/airflow/pull/46390#issuecomment-2647740814 @uranusjr I was testing this and found that the DAG run is being inserted into the `dagrun` table with a None logical date, but it remains in the queued state. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
Lee-W commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1948867735
##
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##
@@ -354,7 +356,8 @@ def trigger_dag_run(
f"DAG with dag_id: '{dag_id}' has import errors and cannot be
triggered",
)
-logical_date = pendulum.instance(body.logical_date)
+logical_date = timezone.coerce_datetime(body.logical_date)
Review Comment:
I thought `body.logical_date` could be None now? What would happen if it's
None
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 commented on PR #46390: URL: https://github.com/apache/airflow/pull/46390#issuecomment-2647422326 > Almost good, just one question on removed run_id logic. Replied [here](https://github.com/apache/airflow/pull/46390#discussion_r1948694375) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1948704995
##
airflow/api_fastapi/core_api/datamodels/dag_run.py:
##
@@ -96,18 +96,16 @@ def check_data_intervals(cls, values):
)
return values
+## when logical date is null, the run id should be generated from
run_after + random string.
+# TODO we need to modify this validator after
https://github.com/apache/airflow/pull/46398 is merged
Review Comment:
Implemented
##
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##
@@ -354,7 +356,8 @@ def trigger_dag_run(
f"DAG with dag_id: '{dag_id}' has import errors and cannot be
triggered",
)
-logical_date = pendulum.instance(body.logical_date)
+logical_date = pendulum.instance(body.logical_date) if body.logical_date
is not None else None
Review Comment:
Implemented
##
airflow/models/dag.py:
##
@@ -1781,7 +1781,8 @@ def create_dagrun(
:meta private:
"""
-logical_date = timezone.coerce_datetime(logical_date)
+if logical_date is not None:
+logical_date = timezone.coerce_datetime(logical_date)
Review Comment:
Implemented
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 commented on code in PR #46390: URL: https://github.com/apache/airflow/pull/46390#discussion_r1948694375 ## airflow/api_fastapi/core_api/routes/public/dag_run.py: ## @@ -365,20 +368,11 @@ def trigger_dag_run( end=pendulum.instance(body.data_interval_end), ) else: -data_interval = dag.timetable.infer_manual_data_interval(run_after=logical_date) - -if body.dag_run_id: -run_id = body.dag_run_id -else: -run_id = dag.timetable.generate_run_id( -run_type=DagRunType.MANUAL, -logical_date=logical_date, -data_interval=data_interval, -) Review Comment: @uranusjr run_id will always be populated by `model_validator`. As mentioned by @pierrejeambrun [here](https://github.com/apache/airflow/pull/46390#discussion_r1943189358) -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
uranusjr commented on code in PR #46390: URL: https://github.com/apache/airflow/pull/46390#discussion_r1948532783 ## airflow/models/dag.py: ## @@ -1781,7 +1781,8 @@ def create_dagrun( :meta private: """ -logical_date = timezone.coerce_datetime(logical_date) +if logical_date is not None: +logical_date = timezone.coerce_datetime(logical_date) Review Comment: ```suggestion logical_date = timezone.coerce_datetime(logical_date) ``` Does not need to change this; `coerce_datetime` can handle None correctly. -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
uranusjr commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1948531482
##
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##
@@ -354,7 +356,8 @@ def trigger_dag_run(
f"DAG with dag_id: '{dag_id}' has import errors and cannot be
triggered",
)
-logical_date = pendulum.instance(body.logical_date)
+logical_date = pendulum.instance(body.logical_date) if body.logical_date
is not None else None
Review Comment:
```suggestion
logical_date = timezone.coerce_datetime(body.logical_date)
```
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
uranusjr commented on code in PR #46390: URL: https://github.com/apache/airflow/pull/46390#discussion_r1948532227 ## airflow/api_fastapi/core_api/routes/public/dag_run.py: ## @@ -365,20 +368,11 @@ def trigger_dag_run( end=pendulum.instance(body.data_interval_end), ) else: -data_interval = dag.timetable.infer_manual_data_interval(run_after=logical_date) - -if body.dag_run_id: -run_id = body.dag_run_id -else: -run_id = dag.timetable.generate_run_id( -run_type=DagRunType.MANUAL, -logical_date=logical_date, -data_interval=data_interval, -) Review Comment: I think this logic still needs to exist? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
uranusjr commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1948530388
##
airflow/api_fastapi/core_api/datamodels/dag_run.py:
##
@@ -96,18 +96,16 @@ def check_data_intervals(cls, values):
)
return values
+## when logical date is null, the run id should be generated from
run_after + random string.
+# TODO we need to modify this validator after
https://github.com/apache/airflow/pull/46398 is merged
Review Comment:
```suggestion
## when logical date is null, the run id should be generated from
run_after + random string.
# TODO: AIP83: we need to modify this validator after
https://github.com/apache/airflow/pull/46398 is merged
```
Just so we can find this easier later.
##
airflow/api_fastapi/core_api/datamodels/dag_run.py:
##
@@ -96,18 +96,16 @@ def check_data_intervals(cls, values):
)
return values
+## when logical date is null, the run id should be generated from
run_after + random string.
+# TODO we need to modify this validator after
https://github.com/apache/airflow/pull/46398 is merged
@model_validator(mode="after")
def validate_dag_run_id(self):
if not self.dag_run_id:
-self.dag_run_id = DagRun.generate_run_id(DagRunType.MANUAL,
self.logical_date)
+self.dag_run_id = DagRun.generate_run_id(
+DagRunType.MANUAL, self.logical_date if self.logical_date is
not None else pendulum.now("UTC")
Review Comment:
```suggestion
DagRunType.MANUAL, self.logical_date or pendulum.now("UTC")
```
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1948454040
##
airflow/api_fastapi/core_api/datamodels/dag_run.py:
##
@@ -99,15 +100,11 @@ def check_data_intervals(cls, values):
@model_validator(mode="after")
def validate_dag_run_id(self):
if not self.dag_run_id:
-self.dag_run_id = DagRun.generate_run_id(DagRunType.MANUAL,
self.logical_date)
+self.dag_run_id = DagRun.generate_run_id(
+DagRunType.MANUAL, self.logical_date if self.logical_date is
not None else pendulum.now("UTC")
+)
Review Comment:
@sunank200 TODO is already
[here](https://github.com/apache/airflow/pull/46390/files#diff-2046c053c5d62417377a58df31e9432f143bcbed09ad1656382b3b5e09b9263bR100)
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
sunank200 commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1948421890
##
airflow/api_fastapi/core_api/datamodels/dag_run.py:
##
@@ -99,15 +100,11 @@ def check_data_intervals(cls, values):
@model_validator(mode="after")
def validate_dag_run_id(self):
if not self.dag_run_id:
-self.dag_run_id = DagRun.generate_run_id(DagRunType.MANUAL,
self.logical_date)
+self.dag_run_id = DagRun.generate_run_id(
+DagRunType.MANUAL, self.logical_date if self.logical_date is
not None else pendulum.now("UTC")
+)
Review Comment:
We should add this TODO as an issue so that we don't miss 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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
pierrejeambrun commented on code in PR #46390: URL: https://github.com/apache/airflow/pull/46390#discussion_r1946901639 ## airflow/api_fastapi/core_api/datamodels/dag_run.py: ## @@ -96,18 +96,16 @@ def check_data_intervals(cls, values): ) return values +## when logical date is null, the run id should be generated from run_after + random string. Review Comment: Do we need to do this part now or wait for #46398 ? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
pierrejeambrun commented on code in PR #46390: URL: https://github.com/apache/airflow/pull/46390#discussion_r1946901639 ## airflow/api_fastapi/core_api/datamodels/dag_run.py: ## @@ -96,18 +96,16 @@ def check_data_intervals(cls, values): ) return values +## when logical date is null, the run id should be generated from run_after + random string. Review Comment: Do we need to do this part now or wait for #46398 ? -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1946017233
##
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##
@@ -362,16 +362,19 @@ def trigger_dag_run(
end=pendulum.instance(body.data_interval_end),
)
else:
-data_interval =
dag.timetable.infer_manual_data_interval(run_after=logical_date)
+data_interval = dag.timetable.infer_manual_data_interval(
+run_after=logical_date or pendulum.now("UTC")
Review Comment:
fixed
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1945997520
##
airflow/api_fastapi/core_api/datamodels/dag_run.py:
##
@@ -99,15 +100,11 @@ def check_data_intervals(cls, values):
@model_validator(mode="after")
def validate_dag_run_id(self):
if not self.dag_run_id:
-self.dag_run_id = DagRun.generate_run_id(DagRunType.MANUAL,
self.logical_date)
+self.dag_run_id = DagRun.generate_run_id(
+DagRunType.MANUAL, self.logical_date if self.logical_date is
not None else pendulum.now("UTC")
+)
Review Comment:
Run_id logic is being implemented in
[PR](https://github.com/apache/airflow/pull/46398). I will update this after
that change is 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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
pierrejeambrun commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1943189358
##
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##
@@ -362,16 +362,19 @@ def trigger_dag_run(
end=pendulum.instance(body.data_interval_end),
)
else:
-data_interval =
dag.timetable.infer_manual_data_interval(run_after=logical_date)
+data_interval = dag.timetable.infer_manual_data_interval(
+run_after=logical_date or pendulum.now("UTC")
+)
-if body.dag_run_id:
-run_id = body.dag_run_id
-else:
-run_id = dag.timetable.generate_run_id(
+run_id = (
+body.dag_run_id
+if body.dag_run_id
+else dag.timetable.generate_run_id(
Review Comment:
I don't think `body.dag_run_id` can ever be none. It's automatically
validated and provided a value if `if not self.dag_run_id:` via
`DagRun.generate_run_id(` in the datamodel.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
pierrejeambrun commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1943187402
##
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##
@@ -362,16 +362,19 @@ def trigger_dag_run(
end=pendulum.instance(body.data_interval_end),
)
else:
-data_interval =
dag.timetable.infer_manual_data_interval(run_after=logical_date)
+data_interval = dag.timetable.infer_manual_data_interval(
+run_after=logical_date or pendulum.now("UTC")
Review Comment:
I think we should have only one `pendulum.now("UTC")` so it's easy to
identify that all of those were automatically filled with `now`. Here we will
have multiple different `now` values.
##
airflow/api_fastapi/core_api/routes/public/dag_run.py:
##
@@ -362,16 +362,19 @@ def trigger_dag_run(
end=pendulum.instance(body.data_interval_end),
)
else:
-data_interval =
dag.timetable.infer_manual_data_interval(run_after=logical_date)
+data_interval = dag.timetable.infer_manual_data_interval(
+run_after=logical_date or pendulum.now("UTC")
+)
-if body.dag_run_id:
-run_id = body.dag_run_id
-else:
-run_id = dag.timetable.generate_run_id(
+run_id = (
+body.dag_run_id
+if body.dag_run_id
+else dag.timetable.generate_run_id(
Review Comment:
I don't think `body.dag_run_id` can ever be none. It's automatically
validated and provided a value if `if not self.dag_run_id:` via
`DagRun.generate_run_id(`
##
airflow/models/backfill.py:
##
@@ -141,6 +140,7 @@ class BackfillDagRunExceptionReason(str, Enum):
IN_FLIGHT = "in flight"
ALREADY_EXISTS = "already exists"
UNKNOWN = "unknown"
+CLEARED_RUN = "cleared existing run"
Review Comment:
Not sure I understand this addition as part of the PR.
##
airflow/migrations/versions/0032_3_0_0_rename_execution_date_to_logical_date_and_nullable.py:
##
@@ -17,9 +17,9 @@
# under the License.
"""
-Drop ``execution_date`` unique constraint on DagRun.
+Make logical_date nullable.
Review Comment:
Editing a migration will not re-run anything for people using breeze. (They
might run into silent errors until they clean their db and re-run migration
from scratch).
That's not a big deal but we might want to drop a message in the contributor
channel.
##
airflow/models/backfill.py:
##
@@ -262,7 +266,23 @@ def _create_backfill_dag_run(
dag_run_conf,
backfill_sort_ordinal,
session,
+from_date,
+to_date,
):
+from airflow.models import DAG
Review Comment:
I assume non local import creates a cyclic import ?
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
dstandish commented on code in PR #46390:
URL: https://github.com/apache/airflow/pull/46390#discussion_r1941670101
##
airflow/api_fastapi/core_api/datamodels/dag_run.py:
##
@@ -99,15 +100,11 @@ def check_data_intervals(cls, values):
@model_validator(mode="after")
def validate_dag_run_id(self):
if not self.dag_run_id:
-self.dag_run_id = DagRun.generate_run_id(DagRunType.MANUAL,
self.logical_date)
+self.dag_run_id = DagRun.generate_run_id(
+DagRunType.MANUAL, self.logical_date if self.logical_date is
not None else pendulum.now("UTC")
+)
Review Comment:
on the [decision
doc](https://cwiki.apache.org/confluence/display/AIRFLOW/Option+2+clarification+doc+WIP),
question 6 states that when logical date is null, the run id should be
generated from run_after + random string.
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] AIP-83 Logical date should be required field when triggering run via API [airflow]
vatsrahul1001 closed pull request #46389: AIP-83 Logical date should be required field when triggering run via API URL: https://github.com/apache/airflow/pull/46389 -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
