You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Inetov (via GitHub)" <gi...@apache.org> on 2023/11/01 12:08:40 UTC
[PR] Added support for multiple cron expressions [airflow]
Inetov opened a new pull request, #35337:
URL: https://github.com/apache/airflow/pull/35337
This PR adds the ability to use a list of CRON expressions for a DAG schedule.
About a year ago another developer tried to do this (#24733).
Here I completely reworked the code of his idea.
Usage example:
```python
@dag(
start_date=pendulum.yesterday(),
schedule=["*/20 3 5 * *", "0 12 * * MON,TUE"],
catchup=False,
)
def multicron_test_dag():
...
```
My implementation only supports `List[str]`.
Tests have been added and are working.
---
**^ 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
Re: [PR] Added support for multiple cron expressions [airflow]
Posted by "Inetov (via GitHub)" <gi...@apache.org>.
Inetov commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1792814059
@uranusjr @potiuk
Sorry for bothering you, this is my first X. should I just wait or write to someone?
--
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] Added support for multiple cron expressions [airflow]
Posted by "allebacco (via GitHub)" <gi...@apache.org>.
allebacco commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1801676220
I like the idea of scheduling with multiple CRON expressions, but I think that instead of supporting list of CRON expressions in the `schedule` argument, we should create a new `Timetable` implementation that should be used like:
```python
@dag(
start_date=pendulum.datetime(2023, 11, 1),
schedule=MultiCronExpressionTimetable(["*/20 3 5 * *", "0 12 * * MON,TUE"]),
catchup=False,
)
def multicron_test_dag():
...
```
With this approach, we aren't changing the current APIs. We could add the new Timetable as _experimental_ and then we could remove it, refactor it or promote it to stable in the future without affecting the APIs of the `DAG` class. I think that this is the reason for which Timetables have been created.
Moreover, creating a new Timetable means that only a very small amount of code will be affected: the testing and code review will be easier. A bug in the current implementation will almost all the timetables and, I think, all the Airflow 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] Added support for multiple cron expressions [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1792833125
I d not know those parts that well, but in a first glance it seems a bit ... to simple. That's at least the impression I have that the way timetables are used and implemented, there is some inherent complexity in handling edge cases - aligning the dates, switching DSTs etc. etc. . At least this was the impression I had looking at past problems and issues.
I tihnk @uranusjr knows those best and maybe he can comment on my worries at least.
If anything - I think my impression is that this one would reqiuire quite a bit more tests and maybe not even unit tests but some integration tests that are covering actual scheduling scenarios.
But again it coudl be just impression resulting from looking over the shoulders of those who implemented and reviewed timeteables previously, so I will leave the jugment of tha to them.
--
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] Added support for multiple cron expressions [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1792846827
> I found an error here in airflow webserver logs (not covered by tests):
>
> https://github.com/apache/airflow/blob/8c25304f047dd77dde6802155a894ce9a15481a8/airflow/api_connexion/schemas/common_schema.py#L130
>
>
> `Exception: Unknown object type: list`
> How to properly fix that?
No idea, but it calls for new API parameter - API needs to be stable and provide backwards compatibility to the clients that our users use, we cannot just CHANGE the API - the old API needs to stay as is and likely new parameters (I assume it is in returned value) should return more complex data structure). That's a "general" statement - not sure how it applies to this particular case, but one of the complexities of this task is to make sure the other parts of the system are just handling the new ways of specifying Cron expressions.
I guess (I have not looked in detail) but UI is also somehow recognising Cron timetables a bit differently and handling them a bit differently than regular timetables (for performance and visualisation) - so my guess is what you see there is part of the "special treatment" of the cron-like timetables that needs to be also updated (this is what I also think is part of my perceived "it looks too simple to handle all edge cases" comment above.
But again - that's my intelligent guess - I have not look at that code for quite a while and have not a lot of capacity to do so now, it's just my impression of the complexity of the task of adding multiple cron specifications (but again - I might be quite wrong here).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added support for multiple cron expressions [airflow]
Posted by "Inetov (via GitHub)" <gi...@apache.org>.
Inetov commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1788891652
I found an error here in airflow webserver logs (not covered by tests):
https://github.com/apache/airflow/blob/8c25304f047dd77dde6802155a894ce9a15481a8/airflow/api_connexion/schemas/common_schema.py#L130
`Exception: Unknown object type: list`
How to properly fix class `CronExpression`?
--
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] Added support for multiple cron expressions [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1935126473
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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] Added support for multiple cron expressions [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #35337: Added support for multiple cron expressions
URL: https://github.com/apache/airflow/pull/35337
--
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] Added support for multiple cron expressions [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1792851649
Maybe also @BasPH can add something here - I see he was the author of the previous attempt. and for some reason it was not completed, maybe I am just imagining the complexity of such task is quite a bit bigger than what I see here :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added support for multiple cron expressions [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1868395408
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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] Added support for multiple cron expressions [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1868472037
But - as usual - it's totally and entirely on the author to take "care" about their PR. rebase it when there are conflicts, fix tes cases, remind the reviewers that it still needs review (usually aftere all that - like passing green tests and.solving conflicts is done). Generally - we have 100s of PRs to take a look at regularly (sometimes daily) and we generally look at those PRs that we see authors are caring about.
--
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] Added support for multiple cron expressions [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1868472273
Also - ther are quite a few comments on that PR already and generailly author's task is to respond to them - either by confirming they agree with the comments and applying them or discussing with them and proposing alternative answers.
--
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] Added support for multiple cron expressions [airflow]
Posted by "BasPH (via GitHub)" <gi...@apache.org>.
BasPH commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1801907960
@allebacco Internally everything becomes a Timetable, so implementing a MultiCronExpressionTimetable is fine. However, I do suggest that we provide a "simpler" interface to users by allowing a list of strings. Airflow currently also does this for single strings, timedelta objects, etc.; it allows the user to provide a standard Python object and internally uses a corresponding Timetable to handle the schedule: https://github.com/apache/airflow/blob/8e0936fbd07de36b57c72750a685e7358c2bdd88/airflow/models/dag.py#L214-L228.
--
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] Added support for multiple cron expressions [airflow]
Posted by "Inetov (via GitHub)" <gi...@apache.org>.
Inetov commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1868438365
@potiuk @uranusjr as far as I understand, the Airflow team is not interested in this feature?
--
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] Added support for multiple cron expressions [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35337:
URL: https://github.com/apache/airflow/pull/35337#issuecomment-1868471282
> @potiuk @uranusjr as far as I understand, the Airflow team is not interested in this feature?
Well. If someone was to write a PR which will be good, sound, passess all the tests and gets a nice, extensible, fully tested PR that keeps on being rebased and has no conflicts, - I think there will be no objections in merging similar support. But it has to be solid, sound, and super-well tested including all the cases with timezones etc. This is rather complex subject as we learn - there are many potential problems with it it (time and especially time-zones are hard).
So we need to beliee that somoene doing it, knows what they are doing.
Other than that - I think the interest is the same as with any other proposal - it's not against our principles and assumptions, just needs someone with solid engineering approach to propose a good PR for 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org