You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/12/16 07:40:23 UTC
[GitHub] [airflow] Soonmok opened a new pull request, #28398: raise ValueError when logical date is earlier than start_date, add th…
Soonmok opened a new pull request, #28398:
URL: https://github.com/apache/airflow/pull/28398
…e test of it
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
related: #26865
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1050553683
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
I don’t recall specifying `logical_date` in trigger config being a valid usage? Why is this 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Soonmok commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051306708
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
Okay.
According to the [issue](https://github.com/apache/airflow/issues/26865), the issuer triggered the dag with config. so I assumed it was triggered with execution_date which is set by default_args.
but now I think there is another reason of this issue. I'll look it up more.
--
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
[GitHub] [airflow] Soonmok commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051279698
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
In this [tutorial](https://airflow.apache.org/docs/apache-airflow/1.10.1/tutorial.html), default_args doesn't provide logical_date, but the issuer was assigning execution_date to default_args.
I think we should check validation of using default_args.
--
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
[GitHub] [airflow] Soonmok commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051279698
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
In this [tutorial](https://airflow.apache.org/docs/apache-airflow/1.10.1/tutorial.html), default_args doesn't provide logical_date, but the issuer was assigning `execution_date` to default_args.
I think we should check validation of using default_args.
--
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
[GitHub] [airflow] Soonmok commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051282512
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
https://github.com/apache/airflow/blob/401fc57e8ba1dddb041e0d777bb0277a09f227db/airflow/models/dag.py#L295
The comment describe, default_arg values can be conflict with operator's arguments, it will follow to operator's argument.
Should `execution_date` argument follow this rule?
--
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
[GitHub] [airflow] Soonmok commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051289893
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
Thank you for the explanation.
Than, the issuer might have used `default_args` to parse `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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #28398: raise ValueError when logical date is earlier than start_date, add th…
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #28398:
URL: https://github.com/apache/airflow/pull/28398#issuecomment-1354335709
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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
[GitHub] [airflow] Taragolis commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051289256
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
`dag_run.conf` use in Templates, and `default_args` parameters propagate to tasks arguments if it not set explicitly.
--
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
[GitHub] [airflow] Soonmok commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051289893
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
Thank you for the explanation.
Than, the issuer might have used `default_args` to parse `logical_date` (execution_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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Soonmok commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051306708
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
Okay.
According to the issue, the issuer triggered the dag with config. so I assumed it was triggered with execution_date which is set by default_args.
but not I think there is another reason of this issue. I'll look it up more.
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
Okay.
According to the issue, the issuer triggered the dag with config. so I assumed it was triggered with execution_date which is set by default_args.
but now I think there is another reason of this issue. I'll look it up more.
--
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
[GitHub] [airflow] Taragolis commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051288043
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
`default_args` != `dag.conf`
1. https://airflow.apache.org/docs/apache-airflow/stable/dag-run.html#passing-parameters-when-triggering-dags
2. https://airflow.apache.org/docs/apache-airflow/stable/tutorial/fundamentals.html#tasks
--
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
[GitHub] [airflow] Soonmok commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051280102
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
Maybe we can set whitelist.
--
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
[GitHub] [airflow] Taragolis commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051297557
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
And this also incorrect assumption.
`default_args` designed for reuse same parameters across Tasks within the single DAG, and it user-defined on code level. This arguments do not changed by their nature in runtime, except selected arguments:
1. `params` - [Params](https://airflow.apache.org/docs/apache-airflow/stable/concepts/params.html#params) could defined on DAG and Task level, so it resolved during DAG initialisation
2. `start_date` and `end_date` - https://airflow.apache.org/docs/apache-airflow/stable/faq.html#what-s-the-deal-with-start-date
That is all exceptions.
`default_args` doesn't not contain `logical_date` / `execution_date` unless user define it, and this might happen if required propagate this arguments to [TriggerDagRunOperator](https://airflow.apache.org/docs/apache-airflow/stable/_api/airflow/operators/trigger_dagrun/index.html#airflow.operators.trigger_dagrun.TriggerDagRunOperator) or [ExternalTaskSensor](https://airflow.apache.org/docs/apache-airflow/stable/_api/airflow/sensors/external_task/index.html#airflow.sensors.external_task.ExternalTaskSensor) without explicit define it during initialisation. But even in this case most probably it would be something like:
```python
with DAG(
...,
default_args={
"execution_date": "{{ logical_date }}"
},
) as dag:
...
```
--
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
[GitHub] [airflow] Taragolis commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051288043
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
`default_args` != `dag_run.conf`
1. https://airflow.apache.org/docs/apache-airflow/stable/dag-run.html#passing-parameters-when-triggering-dags
2. https://airflow.apache.org/docs/apache-airflow/stable/tutorial/fundamentals.html#tasks
--
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
[GitHub] [airflow] Soonmok commented on a diff in pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok commented on code in PR #28398:
URL: https://github.com/apache/airflow/pull/28398#discussion_r1051291646
##########
airflow/api/common/trigger_dag.py:
##########
@@ -51,7 +51,11 @@ def _trigger_dag(
if dag is None or dag_id not in dag_bag.dags:
raise DagNotFound(f"Dag id {dag_id} not found")
- execution_date = execution_date if execution_date else timezone.utcnow()
+ if execution_date is None:
+ if conf and "logical_date" in conf:
+ execution_date = conf["logical_date"]
Review Comment:
I've change conf to default_args.
--
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
[GitHub] [airflow] Soonmok closed pull request #28398: raise ValueError when logical date is earlier than start_date
Posted by GitBox <gi...@apache.org>.
Soonmok closed pull request #28398: raise ValueError when logical date is earlier than start_date
URL: https://github.com/apache/airflow/pull/28398
--
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