You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "uranusjr (via GitHub)" <gi...@apache.org> on 2024/08/07 21:59:15 UTC
[PR] Deprecate implicit DAG schedule [airflow]
uranusjr opened a new pull request, #41321:
URL: https://github.com/apache/airflow/pull/41321
Currently, if a DAG is created with the 'schedule' argument, Airflow sets the DAG to run daily. This is almost never what people want, and every best practice tells you to always specify the argument explicitly, but we couldn't change the default due to compatibility considerations.
We can finally change this in Airflow 3.0, but first we need to tell users about it.
<!--
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.
-->
<!--
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:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.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
Re: [PR] Deprecate implicit DAG schedule [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #41321:
URL: https://github.com/apache/airflow/pull/41321#discussion_r1710078438
##########
airflow/models/dag.py:
##########
@@ -678,6 +678,12 @@ def __init__(
self.timetable = DatasetTriggeredTimetable(DatasetAll(*schedule))
self.schedule_interval = self.timetable.summary
elif isinstance(schedule, ArgNotSet):
+ warnings.warn(
Review Comment:
Yeah... no better way... only chance would be to drop the attribute (which we did in the past once :-D) and add a new one with a new default... yeah. sad.
--
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] Deprecate implicit DAG schedule [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #41321:
URL: https://github.com/apache/airflow/pull/41321#issuecomment-2282852128
@uranusjr @ephraimbuddy - Do we need to get this for 2.10.0rc1?
--
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] Deprecate implicit DAG schedule [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #41321:
URL: https://github.com/apache/airflow/pull/41321#discussion_r1708556536
##########
airflow/models/dag.py:
##########
@@ -678,6 +678,12 @@ def __init__(
self.timetable = DatasetTriggeredTimetable(DatasetAll(*schedule))
self.schedule_interval = self.timetable.summary
elif isinstance(schedule, ArgNotSet):
+ warnings.warn(
Review Comment:
We've definitely considered it and wanted to do it for a long time now. But yeah, breaking change...
--
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] Change default DAG schedule to None [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #41321:
URL: https://github.com/apache/airflow/pull/41321#issuecomment-2287141021
https://github.com/astronomer/airflow/pull/1512/ -should fix those warnings.
--
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] Deprecate implicit default DAG schedule [airflow]
Posted by "malthe (via GitHub)" <gi...@apache.org>.
malthe commented on PR #41321:
URL: https://github.com/apache/airflow/pull/41321#issuecomment-2290585497
s/with/without a ‘schedule’ argument.
--
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] Deprecate implicit default DAG schedule [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr merged PR #41321:
URL: https://github.com/apache/airflow/pull/41321
--
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] Deprecate implicit DAG schedule [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #41321:
URL: https://github.com/apache/airflow/pull/41321#discussion_r1708914131
##########
airflow/models/dag.py:
##########
@@ -678,6 +678,12 @@ def __init__(
self.timetable = DatasetTriggeredTimetable(DatasetAll(*schedule))
self.schedule_interval = self.timetable.summary
elif isinstance(schedule, ArgNotSet):
+ warnings.warn(
Review Comment:
Yeah there is already a default, and we can’t really just change that default unless we warn everyone not use that default first. I do wonder if there’s a better way.
--
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] Deprecate implicit DAG schedule [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #41321:
URL: https://github.com/apache/airflow/pull/41321#discussion_r1708088533
##########
airflow/models/dag.py:
##########
@@ -678,6 +678,12 @@ def __init__(
self.timetable = DatasetTriggeredTimetable(DatasetAll(*schedule))
self.schedule_interval = self.timetable.summary
elif isinstance(schedule, ArgNotSet):
+ warnings.warn(
Review Comment:
Just curious - maybe lagging history: Why was it never considered setting `schedule=None` per default?
If I take a look to ALL your changes in all tests I see this seems the be actually a common and reasonable default (whereas I acknowledge thatchanging the default in a breaking change might confuse and irritate a LOT of users)
--
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] Deprecate implicit default DAG schedule [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #41321:
URL: https://github.com/apache/airflow/pull/41321#issuecomment-2287957990
> > A backport to the 2.10 branch is needed.
>
> According to the new rules it's on the committer who merges it to backport it and create a new PR if I read them right.
I raised https://github.com/apache/airflow/pull/41457 to document the protocols
--
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] Deprecate implicit default DAG schedule [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #41321:
URL: https://github.com/apache/airflow/pull/41321#issuecomment-2288433134
Finally… now merge and work on bacporting.
--
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] Change default DAG schedule to None [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #41321:
URL: https://github.com/apache/airflow/pull/41321#issuecomment-2287382099
This should be ready if CI passes.
**A backport to the 2.10 branch is needed.**
--
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] Change default DAG schedule to None [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #41321:
URL: https://github.com/apache/airflow/pull/41321#issuecomment-2287385800
> A backport to the 2.10 branch is needed.
According to the new rule it's on the committer who merges it to backport it and create a new PR if I read them right.
--
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] Change default DAG schedule to None [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #41321:
URL: https://github.com/apache/airflow/pull/41321#issuecomment-2283227079
Alternative PR posted https://github.com/apache/airflow/pull/41397
--
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] Deprecate implicit DAG schedule [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #41321:
URL: https://github.com/apache/airflow/pull/41321#issuecomment-2283178774
I’m going to do this another way to not break the tests for now. We still need to eventually fix all the tests when we migrate to 3.0, but it’ll buy us some time.
--
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