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 2021/07/11 18:36:42 UTC
[GitHub] [airflow] pateash opened a new pull request #16931: #16692 show cron schedule description in UI
pateash opened a new pull request #16931:
URL: https://github.com/apache/airflow/pull/16931
Closes #16692 ,
As the schedule_interval could be of type datetime.timedelta or
dateutil.relativedelta.relativedelta or str, only showing description whenever the type is 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r670949174
##########
File path: airflow/models/dag.py
##########
@@ -2404,6 +2404,20 @@ def __repr__(self):
def timezone(self):
return settings.TIMEZONE
+ @property
+ def timetable(self) -> Timetable:
+ interval = self.schedule_interval
+ if interval is None:
+ return NullTimetable()
+ if interval == "@once":
+ return OnceTimetable()
+ if isinstance(interval, (timedelta, relativedelta)):
+ return DeltaDataIntervalTimetable(interval)
+ if isinstance(interval, str):
+ return CronDataIntervalTimetable(interval, self.timezone)
+ type_name = type(interval).__name__
+ raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
Review comment:
Yeah, thought about the same,
but I am also not sure, how to do it,
we can create a base table like DagBase which will have the common properties
including, which are there in both DagModel and DAG class, but I think it's a overkill for now.
```python
class DagBase(Base):
"""A Base Class for Dag"""
@cached_property
def timetable(self) -> Timetable:
interval = self.schedule_interval
if interval is None:
return NullTimetable()
if interval == "@once":
return OnceTimetable()
if isinstance(interval, (timedelta, relativedelta)):
return DeltaDataIntervalTimetable(interval)
if isinstance(interval, str):
return CronDataIntervalTimetable(interval, self.timezone)
type_name = type(interval).__name__
raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
```
--
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] pateash closed pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash closed pull request #16931:
URL: https://github.com/apache/airflow/pull/16931
--
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 change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r729024231
##########
File path: airflow/timetables/interval.py
##########
@@ -221,6 +223,20 @@ def infer_manual_data_interval(self, *, run_after: DateTime) -> DataInterval:
end = self._get_prev(self._align(run_after))
return DataInterval(start=self._get_prev(end), end=end)
+ @cached_property
+ def interval_description(self) -> Optional[str]:
Review comment:
Since this value is for UI only, I feel it should simply return str. The None case will be coerced into a str eventually anyway, so it's better to just return `"None"` or an empty string instead.
Comments for the protocol definition about caching also apply 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
[GitHub] [airflow] pateash commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-940916118
> @pateash Would you have a chance to pick this back up?
@uranusjr I just returned from vacation,
I will work on it tomorrow.
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r703248777
##########
File path: airflow/models/dag.py
##########
@@ -2521,6 +2521,10 @@ def __init__(self, concurrency=None, **kwargs):
def __repr__(self):
return f"<DAG: {self.dag_id}>"
+ @cached_property
+ def timetable(self) -> Timetable:
+ return create_timetable(self.schedule_interval, self.timezone)
Review comment:
I am adding this in DagModel not in 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] pateash commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-992499309
@uranusjr there were some CI failures due to kerberos authentication.
I hope they are sorted already.
--
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] bbovenzi edited a comment on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-946829239
> 
The info icons next to the badges are unnecessary. I'd look at how the Next Run badge next to it works.
--
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] pateash edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-879111914
> Since some of these can be lengthy, I'm wondering if the `Runs: ` prefix is superfluous? Maybe the `At ` too?
`At` is part of library output, So I would not recommend stripping it manually.
From a Length perspective, I think most of the time the length should be manageable even with the `Runs:`,
let me know your thoughts.
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r670949174
##########
File path: airflow/models/dag.py
##########
@@ -2404,6 +2404,20 @@ def __repr__(self):
def timezone(self):
return settings.TIMEZONE
+ @property
+ def timetable(self) -> Timetable:
+ interval = self.schedule_interval
+ if interval is None:
+ return NullTimetable()
+ if interval == "@once":
+ return OnceTimetable()
+ if isinstance(interval, (timedelta, relativedelta)):
+ return DeltaDataIntervalTimetable(interval)
+ if isinstance(interval, str):
+ return CronDataIntervalTimetable(interval, self.timezone)
+ type_name = type(interval).__name__
+ raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
Review comment:
Yeah, thought about the same,
but I am also not sure, how to do it,
we can create a base class like DagBase which will have the common properties, but I think it's a overkill for now.
```python
class DagBase(Base):
"""A Base Class for Dag"""
@cached_property
def timetable(self) -> Timetable:
interval = self.schedule_interval
if interval is None:
return NullTimetable()
if interval == "@once":
return OnceTimetable()
if isinstance(interval, (timedelta, relativedelta)):
return DeltaDataIntervalTimetable(interval)
if isinstance(interval, str):
return CronDataIntervalTimetable(interval, self.timezone)
type_name = type(interval).__name__
raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
```
--
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] HaloKo4 commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
HaloKo4 commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-944498545
What will it show for 6th param in cron expression?
croniter doesn't follow the convention about it
see https://github.com/taichino/croniter/issues/176
--
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] pateash commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-888440466
I agree @uranusjr,
let's wait for your change.
Could you please mention your PR/Issue here, so we can track 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
[GitHub] [airflow] uranusjr edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883067237
--
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 change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r729024495
##########
File path: airflow/timetables/base.py
##########
@@ -94,6 +95,15 @@ def logical_date(self) -> DateTime:
class Timetable(Protocol):
"""Protocol that all Timetable classes are expected to implement."""
+ @cached_property
+ def interval_description(self) -> Optional[str]:
+ """Override to describe the interval.
+
+ For cron ``'30 21 * * 5'``, description could be like ``'At 09:30 PM, only on Friday'``.
+ This is used in the web UI.
+ """
+ return None
Review comment:
Also I feel this can simply be called `description`.
--
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] pateash edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883062473
--
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 pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-947623577
Again, I have no strong opinions here (as long as we don't provide incorrect description), so feel free to do what you think is best.
--
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] pateash edited a comment on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-947348126
> > 
>
> The info icons next to the badges are unnecessary. I'd look at how the Next Run badge next to it works.
the reason I have added <info> Icon, as we will not be able to provide the description in all the cases,
for example in case of deltatimetable we are not providing any description, in this case it will be easier for the user to know if there is any Description available or not.
--
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] pateash commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-951058516
> Generally looks good to me now! One thing I'm wondering is how `None` and `""` differ semantically. From the current implementation in HTML, they seem to be equal? How could a difference be introduced in the future? If there are no differences in semantics for our current use cases, IMO it'd be a good idea to _forbid_ None (NULL in database) for now to save it for potential use cases that may come up.
That's a really good point from future perspective,
We should really use empty string.
Doing the 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-992503893
Yeah those were just timing out, probably temporary networking error. This has nothing to do with them, and all other CI jobs pass without issues, so the possibility there is a genuine breakage is extremely low.
--
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 merged pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #16931:
URL: https://github.com/apache/airflow/pull/16931
--
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] pateash edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-879111914
> Since some of these can be lengthy, I'm wondering if the `Runs: ` prefix is superfluous? Maybe the `At ` too?
`At` is part of library output, So I would not recommend stripping it manually.
From a Length perspective, I think most of the time the length should be manageable even with the `Run:`,
let me know your thoughts.
--
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 edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883067237
--
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 pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883067237
The problem is we can’t store the timetable object in db (at least easily, technically doable but should be avoided). Maybe we can pre-render the description and store that (it’d be just a string) on `DagModel` instead? Call it `timetable_description` or something.
--
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] pateash edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883062473
> > we can create a base class like DagBase which will have the common properties, but I think it's a overkill but a better approach so applying this.
>
> The problem is, we are planning to make `timetable` directly customisable by the user like this:
>
> ```python
> class MyCustomTimetable(Timetable):
> ...
>
> dag = DAG(
> ...,
> timetable=MyCustomTimetable(),
> )
> ```
>
> and this can be any rich Python object. So while the `BaseDag` approach works right now, it will not work once the timetable instance is not calculated directly from `schedule_interval`. So we need to access the original `DAG.timetable` property instead.
>
> This is actually not difficult at all. All the DAG objects are in `current_app.dag_bag`, so you can add this to the template’s context:
>
> ```python
> 'dag_timetable': current_app.dag_bag.get_dag(dag.dag_id).timetable
> ```
>
> But it is pretty cumbersome to add this for _all_ views that render `dag.html` (because this is a base template inherited by many), and even more difficult to do for `dags.html`. So I’m thinking we probably need to expose `current_app.dag_bag` in the template instead, maybe by poplating `dag_bag` into the global Jinja environment (with e.g. `current_app.jinja_env.add_template_global()`), or something else. I’m unfortuantely not familiar with Flask enough to know the best practice.
@ashb @potiuk @kaxil @uranusjr
Please correct me If I am wrong here,
As far as I know we use DagModel object to render dag information in UI,
so for timeline even if we use something like an Enriched Python Object in the future, we will still need to store that information in Database/DagModel.
```python
class MyCustomTimetable(Timetable):
...
dag = DAG(
...,
timetable=MyCustomTimetable(),
)
```
So, while rendering in UI, we will need that information in DagModel and we can't use **DAG.timetable**.
--
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] pateash edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-940916118
> @pateash Would you have a chance to pick this back up?
@uranusjr I just returned from vacation,
I will work on it tomorrow.
--
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] mik-laj commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r702481903
##########
File path: airflow/timetables/base.py
##########
@@ -94,6 +95,15 @@ def logical_date(self) -> DateTime:
class Timetable(Protocol):
"""Protocol that all Timetable classes are expected to implement."""
+ @cached_property
+ def interval_description(self) -> Optional[str]:
+ """Override to describe the interval.
+
+ For cron ``'30 21 * * 5'``, description could be like ``'At 09:30 PM, only on Friday'``.
+ This is used in the web UI.
+ """
+ return None
Review comment:
```suggestion
interval_description: Optional[str] = None
"""Override to describe the interval.
For cron ``'30 21 * * 5'``, description could be like ``'At 09:30 PM, only on Friday'``.
This is used in the web UI.
"""
```
This is a protocol, so I don't reason see why we should use cached_propertty.
--
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] pateash edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883062473
--
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] pateash commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-947348126
> > 
>
> The info icons next to the badges are unnecessary. I'd look at how the Next Run badge next to it works.
the reason I have added <info> Icon, as we will not be able to provide the description always,
for example in case of deltatimetable we are not providing any description, in this case it will be easier for the user to know if there is any Description available or not.
--
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] HaloKo4 edited a comment on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
HaloKo4 edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-944498545
What will it show for 6th param in cron expression?
croniter doesn't follow the convention about it
see https://github.com/taichino/croniter/issues/176
It's important that the description string will strict to croniter otherwise it will show a misleading description
--
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 pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-940861842
@pateash Would you have a chance to pick this back up?
--
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] bbovenzi commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r729075558
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
There are a few parts in views.py that you can see we sometimes need to access both a DAG and DagModel.
https://github.com/apache/airflow/blob/main/airflow/www/views.py#L2409
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
There are a few parts in views.py that you can see we sometimes need to access both a DAG and DagModel.
https://github.com/apache/airflow/blob/main/airflow/www/views.py#L2357
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
Oh nooo. Link is correct now
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -190,6 +190,11 @@ <h2>{{ page_title }}</h2>
<a class="label label-default schedule" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}" data-dag-id="{{ dag.dag_id }}">
{{ dag.schedule_interval }}
</a>
+
+ {% if dag.timetable_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Schedule: {{ dag.timetable_description|string }}">info</span>
+ {% endif %}
+
Review comment:
Nitpick: Let's remove the empty lines but indent the content inside of the `if` statement
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -190,6 +190,11 @@ <h2>{{ page_title }}</h2>
<a class="label label-default schedule" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}" data-dag-id="{{ dag.dag_id }}">
{{ dag.schedule_interval }}
</a>
+
+ {% if dag.timetable_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Schedule: {{ dag.timetable_description|string }}">info</span>
+ {% endif %}
+
Review comment:
Also, what are we using the `id` for?
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag_model is defined and dag_model.timetable_description is not none %}
+ <span class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Schedule: {{ dag_model.timetable_description|string }}">info</span>
+ {% endif %}
{% if dag_model is defined and dag_model.next_dagrun is defined %}
<p class="label label-default js-tooltip" style="margin-left: 5px" id="next-run" data-html="true" data-placement="bottom">
Next Run: <time datetime="{{ dag_model.next_dagrun }}">{{ dag_model.next_dagrun }}</time>
</p>
{% endif %}
+
Review comment:
Let's remove this new line too.
--
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] pateash closed pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash closed pull request #16931:
URL: https://github.com/apache/airflow/pull/16931
--
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 change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r702607664
##########
File path: airflow/models/dag.py
##########
@@ -2521,6 +2521,10 @@ def __init__(self, concurrency=None, **kwargs):
def __repr__(self):
return f"<DAG: {self.dag_id}>"
+ @cached_property
+ def timetable(self) -> Timetable:
+ return create_timetable(self.schedule_interval, self.timezone)
Review comment:
This is wrong. After #17414, a DAG instance now has a timetable _attribute_ and this is not needed (in fact this property will break existing code).
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
`dag` here is not always a `DAG` object, but may be a `DagModel`. So this will not work. You need to introduce a common interface across `DAG` and `DagModel` instead of accessing `timetable` directly. See `DAG.bulk_write_to_db()` for how `DAG` attributes are written to corresponding `DagModel` attributes.
--
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 pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883067237
The problem is we can’t store the timetable object in db (at least easily, technically doable but should be avoided). Maybe we can pre-render the description and store that (it’d be just a string) on `DagModel` instead? Call it `timetable_description` or something.
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r671974786
##########
File path: airflow/models/dag.py
##########
@@ -340,7 +362,6 @@ def __init__(
self.template_undefined = template_undefined
self.parent_dag: Optional[DAG] = None # Gets set when DAGs are loaded
self.last_loaded = timezone.utcnow()
- self.safe_dag_id = dag_id.replace('.', '__dot__')
Review comment:
moved to property inside DagBase class
--
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] pateash commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-884180567
> The problem is we can’t store the timetable object in db (at least not easily; it’s technically doable but should be avoided). Maybe we can pre-render the description and store that (it’d be just a string) on `DagModel` instead? Call it `timetable_description` or something. Again, I’m not sure how best to do this.
Honestly, I am not sure what you are trying to suggest here.
@uranusjr, If you are trying to suggest some specific changes we should do, let me know.
currently, timetable.description has same logic for DAG and DagModel which might change in future as you pointed out.
We can modify that behaviour later whenever we do that 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
[GitHub] [airflow] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r670949174
##########
File path: airflow/models/dag.py
##########
@@ -2404,6 +2404,20 @@ def __repr__(self):
def timezone(self):
return settings.TIMEZONE
+ @property
+ def timetable(self) -> Timetable:
+ interval = self.schedule_interval
+ if interval is None:
+ return NullTimetable()
+ if interval == "@once":
+ return OnceTimetable()
+ if isinstance(interval, (timedelta, relativedelta)):
+ return DeltaDataIntervalTimetable(interval)
+ if isinstance(interval, str):
+ return CronDataIntervalTimetable(interval, self.timezone)
+ type_name = type(interval).__name__
+ raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
Review comment:
Yeah, thought about the same,
but I am also not sure, how to do it,
we can create a base table like DagBase which will have the common properties, but I think it's a overkill for now.
```python
class DagBase(Base):
"""A Base Class for Dag"""
@cached_property
def timetable(self) -> Timetable:
interval = self.schedule_interval
if interval is None:
return NullTimetable()
if interval == "@once":
return OnceTimetable()
if isinstance(interval, (timedelta, relativedelta)):
return DeltaDataIntervalTimetable(interval)
if isinstance(interval, str):
return CronDataIntervalTimetable(interval, self.timezone)
type_name = type(interval).__name__
raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
```
--
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] bbovenzi commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r729075558
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
There are a few parts in views.py that you can see we sometimes need to access both a DAG and DagModel.
https://www.airbnb.com/rooms/52733416?adults=3&check_in=2021-10-24&check_out=2021-10-30&translate_ugc=false&federated_search_id=13dcea61-24c4-4e9d-863b-dea85dd715fa&source_impression_id=p3_1634179624_IoBtfeVEwh%2FrxoQL
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r671974786
##########
File path: airflow/models/dag.py
##########
@@ -340,7 +362,6 @@ def __init__(
self.template_undefined = template_undefined
self.parent_dag: Optional[DAG] = None # Gets set when DAGs are loaded
self.last_loaded = timezone.utcnow()
- self.safe_dag_id = dag_id.replace('.', '__dot__')
Review comment:
moved to property inside DagBase class
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r729780395
##########
File path: airflow/models/dag.py
##########
@@ -2521,6 +2521,10 @@ def __init__(self, concurrency=None, **kwargs):
def __repr__(self):
return f"<DAG: {self.dag_id}>"
+ @cached_property
+ def timetable(self) -> Timetable:
+ return create_timetable(self.schedule_interval, self.timezone)
Review comment:
Ok got it,
So going forward **schedule_interval** will not be available and this is now just for backward compatibility.
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
> There are a few parts in views.py that you can see we sometimes need to access both a DAG and DagModel.
>
> https://www.airbnb.com/rooms/52733416?adults=3&check_in=2021-10-24&check_out=2021-10-30&translate_ugc=false&federated_search_id=13dcea61-24c4-4e9d-863b-dea85dd715fa&source_impression_id=p3_1634179624_IoBtfeVEwh%2FrxoQL
@bbovenzi, I do not any plans to visit Mexico but thanks for the invite. :rofl:
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
> There are a few parts in views.py that you can see we sometimes need to access both a DAG and DagModel.
>
> https://www.airbnb.com/rooms/52733416?adults=3&check_in=2021-10-24&check_out=2021-10-30&translate_ugc=false&federated_search_id=13dcea61-24c4-4e9d-863b-dea85dd715fa&source_impression_id=p3_1634179624_IoBtfeVEwh%2FrxoQL
@bbovenzi, I do not any plans to visit Mexico but thanks for the invite. :rofl:
##########
File path: airflow/timetables/base.py
##########
@@ -94,6 +95,15 @@ def logical_date(self) -> DateTime:
class Timetable(Protocol):
"""Protocol that all Timetable classes are expected to implement."""
+ @cached_property
+ def interval_description(self) -> Optional[str]:
+ """Override to describe the interval.
+
+ For cron ``'30 21 * * 5'``, description could be like ``'At 09:30 PM, only on Friday'``.
+ This is used in the web UI.
+ """
+ return None
Review comment:
implemented
##########
File path: airflow/timetables/interval.py
##########
@@ -221,6 +223,20 @@ def infer_manual_data_interval(self, *, run_after: DateTime) -> DataInterval:
end = self._get_prev(self._align(run_after))
return DataInterval(start=self._get_prev(end), end=end)
+ @cached_property
+ def interval_description(self) -> Optional[str]:
Review comment:
Caching disabled,
I am checking for none in jinja, it there is no description I will not show info at all.
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
Thanks, I can use dag_model object here as well similar to what @bbovenzi used for **next_dagrun**,
let me know your thoughts.
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
Thanks, I am using dag_model object here as well similar to what @bbovenzi used for **next_dagrun**,
let me know your thoughts.
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -190,6 +190,11 @@ <h2>{{ page_title }}</h2>
<a class="label label-default schedule" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}" data-dag-id="{{ dag.dag_id }}">
{{ dag.schedule_interval }}
</a>
+
+ {% if dag.timetable_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Schedule: {{ dag.timetable_description|string }}">info</span>
+ {% endif %}
+
Review comment:
removed.
--
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] pateash commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-947349360
> > I think it's better to show a message that says "descritpion can not be evaluated for non standard cron" or something of a sort
>
> That works for me. My main concern is only to not show the wrong explanation.
Agree, but I think we should not provide any description if not available ( as we are doing in other cases )
let me now your thoughts.
--
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] pateash commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-947644821
> Again, I have no strong opinions here (as long as we don't provide incorrect description), so feel free to do what you think is best.
Sure, I am pretty much done with the changes from my end.
please let me know if you have any other 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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-946829239
> 
The info icons next to the badges are unnecessary. I'd look at how the Next Run badge on the DAG page works.
--
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] pateash edited a comment on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-947349360
> > I think it's better to show a message that says "descritpion can not be evaluated for non standard cron" or something of a sort
>
> That works for me. My main concern is only to not show the wrong explanation.
Agree, but I think we should not provide any description if not available ( as we are doing in other cases like datetimedeltaTimeTable and other types)
let me know your thoughts.
--
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] pateash commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883062473
> > we can create a base class like DagBase which will have the common properties, but I think it's a overkill but a better approach so applying this.
>
> The problem is, we are planning to make `timetable` directly customisable by the user like this:
>
> ```python
> class MyCustomTimetable(Timetable):
> ...
>
> dag = DAG(
> ...,
> timetable=MyCustomTimetable(),
> )
> ```
>
> and this can be any rich Python object. So while the `BaseDag` approach works right now, it will not work once the timetable instance is not calculated directly from `schedule_interval`. So we need to access the original `DAG.timetable` property instead.
>
> This is actually not difficult at all. All the DAG objects are in `current_app.dag_bag`, so you can add this to the template’s context:
>
> ```python
> 'dag_timetable': current_app.dag_bag.get_dag(dag.dag_id).timetable
> ```
>
> But it is pretty cumbersome to add this for _all_ views that render `dag.html` (because this is a base template inherited by many), and even more difficult to do for `dags.html`. So I’m thinking we probably need to expose `current_app.dag_bag` in the template instead, maybe by poplating `dag_bag` into the global Jinja environment (with e.g. `current_app.jinja_env.add_template_global()`), or something else. I’m unfortuantely not familiar with Flask enough to know the best practice.
@ashb @potiuk @kaxil @uranusjr
Please correct me If I am wrong here,
As far as I know we use DagModel object to render dag information in UI,
so for timeline even if we use something like an Enriched Python Object in the future, we will still need to store that information in Database/DagModel right?
```python
class MyCustomTimetable(Timetable):
...
dag = DAG(
...,
timetable=MyCustomTimetable(),
)
```
So, while rendering in UI, we will need that information in DagModel and we can't use **DAG.timetable**.
--
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] pateash edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883062473
@ashb @potiuk @kaxil @uranusjr @ryanahamilton
Please correct me If I am wrong here,
As far as I know we use DagModel object to render dag information in UI,
so for timeline even if we use something like an Enriched Python Object in the future, we will still need to store that information in Database/DagModel.
```python
class MyCustomTimetable(Timetable):
...
dag = DAG(
...,
timetable=MyCustomTimetable(),
)
```
So, while rendering in UI, we will need that information in DagModel and we can't use **DAG.timetable**.
I think, whenever we do a change in DAG Api for allowing **timetable** as an argument, we can modify our model to accommodate that change and show the description according to the requirements needed at that point in 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
[GitHub] [airflow] pateash commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883062473
> > we can create a base class like DagBase which will have the common properties, but I think it's a overkill but a better approach so applying this.
>
> The problem is, we are planning to make `timetable` directly customisable by the user like this:
>
> ```python
> class MyCustomTimetable(Timetable):
> ...
>
> dag = DAG(
> ...,
> timetable=MyCustomTimetable(),
> )
> ```
>
> and this can be any rich Python object. So while the `BaseDag` approach works right now, it will not work once the timetable instance is not calculated directly from `schedule_interval`. So we need to access the original `DAG.timetable` property instead.
>
> This is actually not difficult at all. All the DAG objects are in `current_app.dag_bag`, so you can add this to the template’s context:
>
> ```python
> 'dag_timetable': current_app.dag_bag.get_dag(dag.dag_id).timetable
> ```
>
> But it is pretty cumbersome to add this for _all_ views that render `dag.html` (because this is a base template inherited by many), and even more difficult to do for `dags.html`. So I’m thinking we probably need to expose `current_app.dag_bag` in the template instead, maybe by poplating `dag_bag` into the global Jinja environment (with e.g. `current_app.jinja_env.add_template_global()`), or something else. I’m unfortuantely not familiar with Flask enough to know the best practice.
@ashb @potiuk @kaxil @uranusjr
Please correct me If I am wrong here,
As far as I know we use DagModel object to render dag information in UI,
so for timeline even if we use something like an Enriched Python Object in the future, we will still need to store that information in Database/DagModel right?
```python
class MyCustomTimetable(Timetable):
...
dag = DAG(
...,
timetable=MyCustomTimetable(),
)
```
So, while rendering in UI, we will need that information in DagModel and we can't use **DAG.timetable**.
--
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 edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883067237
The problem is we can’t store the timetable object in db (at least not easily; it’s technically doable but should be avoided). Maybe we can pre-render the description and store that (it’d be just a string) on `DagModel` instead? Call it `timetable_description` or something. Again, I’m not sure how best to do this.
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r670949174
##########
File path: airflow/models/dag.py
##########
@@ -2404,6 +2404,20 @@ def __repr__(self):
def timezone(self):
return settings.TIMEZONE
+ @property
+ def timetable(self) -> Timetable:
+ interval = self.schedule_interval
+ if interval is None:
+ return NullTimetable()
+ if interval == "@once":
+ return OnceTimetable()
+ if isinstance(interval, (timedelta, relativedelta)):
+ return DeltaDataIntervalTimetable(interval)
+ if isinstance(interval, str):
+ return CronDataIntervalTimetable(interval, self.timezone)
+ type_name = type(interval).__name__
+ raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
Review comment:
Yeah, thought about the same,
but I am also not sure, how to do it,
we should create a base table like DagBase which will have the common properties
including, which are there in both DagModel and DAG class
```python
@property
def safe_dag_id(self):
return self.dag_id.replace('.', '__dot__')
@cached_property
def timetable(self) -> Timetable:
interval = self.schedule_interval
if interval is None:
return NullTimetable()
if interval == "@once":
return OnceTimetable()
if isinstance(interval, (timedelta, relativedelta)):
return DeltaDataIntervalTimetable(interval)
if isinstance(interval, str):
return CronDataIntervalTimetable(interval, self.timezone)
type_name = type(interval).__name__
raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
```
--
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] pateash closed pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash closed pull request #16931:
URL: https://github.com/apache/airflow/pull/16931
--
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] pateash commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883062473
> > we can create a base class like DagBase which will have the common properties, but I think it's a overkill but a better approach so applying this.
>
> The problem is, we are planning to make `timetable` directly customisable by the user like this:
>
> ```python
> class MyCustomTimetable(Timetable):
> ...
>
> dag = DAG(
> ...,
> timetable=MyCustomTimetable(),
> )
> ```
>
> and this can be any rich Python object. So while the `BaseDag` approach works right now, it will not work once the timetable instance is not calculated directly from `schedule_interval`. So we need to access the original `DAG.timetable` property instead.
>
> This is actually not difficult at all. All the DAG objects are in `current_app.dag_bag`, so you can add this to the template’s context:
>
> ```python
> 'dag_timetable': current_app.dag_bag.get_dag(dag.dag_id).timetable
> ```
>
> But it is pretty cumbersome to add this for _all_ views that render `dag.html` (because this is a base template inherited by many), and even more difficult to do for `dags.html`. So I’m thinking we probably need to expose `current_app.dag_bag` in the template instead, maybe by poplating `dag_bag` into the global Jinja environment (with e.g. `current_app.jinja_env.add_template_global()`), or something else. I’m unfortuantely not familiar with Flask enough to know the best practice.
@ashb @potiuk @kaxil @uranusjr
Please correct me If I am wrong here,
As far as I know we use DagModel object to render dag information in UI,
so for timeline even if we use something like an Enriched Python Object in the future, we will still need to store that information in Database/DagModel right?
```python
class MyCustomTimetable(Timetable):
...
dag = DAG(
...,
timetable=MyCustomTimetable(),
)
```
So, while rendering in UI, we will need that information in DagModel and we can't use **DAG.timetable**.
--
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] pateash commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-913190061
Hi @potiuk, @uranusjr
I have done the changes as per new changes in timetable framework.
--
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] pateash commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-879129282
> I'd be in favor of at least removing the "Runs: ". Additionally, the adjacent column heading is "Runs", so using the same term in two different columns with slightly different meanings is probably best to avoid.
Sure, make sense.
--
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 change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r729020028
##########
File path: airflow/timetables/base.py
##########
@@ -94,6 +95,15 @@ def logical_date(self) -> DateTime:
class Timetable(Protocol):
"""Protocol that all Timetable classes are expected to implement."""
+ @cached_property
+ def interval_description(self) -> Optional[str]:
+ """Override to describe the interval.
+
+ For cron ``'30 21 * * 5'``, description could be like ``'At 09:30 PM, only on Friday'``.
+ This is used in the web UI.
+ """
+ return None
Review comment:
And there's really no point to cache this even in subclasses either. The property is easy to calculate, and generally only accessed once in web UI to render the value.
--
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] pateash edited a comment on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-946730836
Changes done, we are not evaluating description for Cron having more than 5 params
--
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] github-actions[bot] commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-991384406
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
[GitHub] [airflow] github-actions[bot] commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-879023481
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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] pateash edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883062473
--
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 change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r669088674
##########
File path: airflow/www/views.py
##########
@@ -619,6 +619,9 @@ def index(self):
dag.can_edit = (permissions.ACTION_CAN_EDIT, dag_resource_name) in user_permissions
dag.can_trigger = dag.can_edit and can_create_dag_run
dag.can_delete = can_delete_dag
+ dag.schedule_interval_description = wwwutils.get_schedule_interval_description(
+ dag.schedule_interval
+ )
Review comment:
It’s better to incorporate this into the timetable model, something like `dag.timetable.description` (perhaps as a `@property`). This avoids the `if type(schedule_interval)` check, which is generally a code smell (should use polymorphism instead), and allows future custom timetables (proposed in [AIP-39](https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-39+Richer+scheduler_interval)) to also provide a nice description to show in UI.
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r703248151
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
sure, let me check,
but as far as I know we always get DagModel object in dag.html,
what are the cases where we will have Dag object.
--
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 pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-882368742
> we can create a base class like DagBase which will have the common properties, but I think it's a overkill but a better approach so applying this.
The problem is, we are planning to make `timetable` directly customisable by the user like this:
```python
class MyCustomTimetable(Timetable):
...
dag = DAG(
...,
timetable=MyCustomTimetable(),
)
```
and this can be any rich Python object. So while the `BaseDag` approach works right now, it will not work once the timetable instance is not calculated directly from `schedule_interval`. So we need to access the original `DAG.timetable` property instead.
This is actually not difficult at all. All the DAG objects are in `current_app.dag_bag`, so you can add this to the template’s context:
```python
'dag_timetable': current_app.dag_bag.get_dag(dag.dag_id).timetable
```
But it is pretty cumbersome to add this for *all* views that render `dag.html` (because this is a base template inherited by many), and even more difficult to do for `dags.html`. So I’m thinking we probably need to expose `current_app.dag_bag` in the template instead, maybe by poplating `dag_bag` into the global Jinja environment (with e.g. `current_app.jinja_env.add_template_global()`), or something else. I’m unfortuantely not familiar with Flask enough to know the best practice.
--
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] ashb commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r669419936
##########
File path: airflow/www/views.py
##########
@@ -619,6 +619,9 @@ def index(self):
dag.can_edit = (permissions.ACTION_CAN_EDIT, dag_resource_name) in user_permissions
dag.can_trigger = dag.can_edit and can_create_dag_run
dag.can_delete = can_delete_dag
+ dag.schedule_interval_description = wwwutils.get_schedule_interval_description(
+ dag.schedule_interval
+ )
Review comment:
Agreed, especially since the basic timetable framework is already merged in to main branch.
--
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] pateash commented on a change in pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r735147907
##########
File path: airflow/timetables/simple.py
##########
@@ -54,6 +54,9 @@ class NullTimetable(_TrivialTimetable):
This corresponds to ``schedule_interval=None``.
"""
+ def __init__(self) -> None:
+ self.description = "Never, external triggers only"
Review comment:
@uranusjr thanks,
changes done.
--
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] bbovenzi edited a comment on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
bbovenzi edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-946829239
> 
The info icons next to the badges are unnecessary. I'd look at how the Next Run badge works.
--
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] pateash edited a comment on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-944879588
--
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 edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-883067237
The problem is we can’t store the timetable object in db (at least easily, technically doable but should be avoided). Maybe we can pre-render the description and store that (it’d be just a string) on `DagModel` instead? Call it `timetable_description` or something. Again, I’m not sure how best to do this.
--
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] alexInhert commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
alexInhert commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-946843727
> For now, though, I think we should avoid dealing with the problem entirely, and put an artificial limit so _description is not shown for 6- and 7-segment cron expressions_.
Not showing anything might feel more a bug rather than a feature.
I think it's better to show a message that says "descritpion can not be evaluated for non standard cron" or something of a sort but that is just my opinion.
--
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] pateash commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-946730836
Changes added, we are not evaluating description for Cron having more than 5 params
--
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 pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-947099358
> I think it's better to show a message that says "descritpion can not be evaluated for non standard cron" or something of a sort
That works for me. My main concern is only to not show the wrong explaination.
--
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] pateash commented on a change in pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r735139208
##########
File path: airflow/timetables/interval.py
##########
@@ -124,6 +125,19 @@ def __init__(self, cron: str, timezone: Timezone) -> None:
self._expression = cron_presets.get(cron, cron)
self._timezone = timezone
+ descriptor = ExpressionDescriptor(
+ expression=self._expression, casing_type=CasingTypeEnum.Sentence, use_24hour_time_format=True
+ )
+ try:
+ # checking for more than 5 parameters in Cron and avoiding evaluation for now,
+ # as Croniter has inconsistent evaluation with other libraries
+ if self._expression.count(" ") > 4:
+ raise FormatException()
Review comment:
I didn't know about croniter.expanded, this is much better.
--
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] pateash edited a comment on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-951058516
> Generally looks good to me now! One thing I'm wondering is how `None` and `""` differ semantically. From the current implementation in HTML, they seem to be equal? How could a difference be introduced in the future? If there are no differences in semantics for our current use cases, IMO it'd be a good idea to _forbid_ None (NULL in database) for now to save it for potential use cases that may come up.
That's a really good point from future perspective,
We should really use empty string for keeping options open.
Doing the 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-950539145
Generally looks good to me now! One thing I'm wondering is how `None` and `""` differ semantically. From the current implementation in HTML, they seem to be equal? How could a difference be introduced in the future? If there are no differences in semantics for our current use cases, IMO it'd be a good idea to *forbid* None (NULL in database) for now to save it for potential use cases that may come up.
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r670949174
##########
File path: airflow/models/dag.py
##########
@@ -2404,6 +2404,20 @@ def __repr__(self):
def timezone(self):
return settings.TIMEZONE
+ @property
+ def timetable(self) -> Timetable:
+ interval = self.schedule_interval
+ if interval is None:
+ return NullTimetable()
+ if interval == "@once":
+ return OnceTimetable()
+ if isinstance(interval, (timedelta, relativedelta)):
+ return DeltaDataIntervalTimetable(interval)
+ if isinstance(interval, str):
+ return CronDataIntervalTimetable(interval, self.timezone)
+ type_name = type(interval).__name__
+ raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
Review comment:
Yeah, thought about the same,
but I am also not sure, how to do it,
we can create a base class like DagBase which will have the common properties, but I think it's a overkill but a better approach so applying this.
```python
class DagBase(Base):
"""A Base Class for Dag"""
@cached_property
def timetable(self) -> Timetable:
interval = self.schedule_interval
if interval is None:
return NullTimetable()
if interval == "@once":
return OnceTimetable()
if isinstance(interval, (timedelta, relativedelta)):
return DeltaDataIntervalTimetable(interval)
if isinstance(interval, str):
return CronDataIntervalTimetable(interval, self.timezone)
type_name = type(interval).__name__
raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
```
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r670768419
##########
File path: airflow/www/views.py
##########
@@ -619,6 +619,9 @@ def index(self):
dag.can_edit = (permissions.ACTION_CAN_EDIT, dag_resource_name) in user_permissions
dag.can_trigger = dag.can_edit and can_create_dag_run
dag.can_delete = can_delete_dag
+ dag.schedule_interval_description = wwwutils.get_schedule_interval_description(
+ dag.schedule_interval
+ )
Review comment:
thanks @uranusjr @ashb,
makes sense.
--
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] pateash commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-879111914
> Since some of these can be lengthy, I'm wondering if the `Runs: ` prefix is superfluous? Maybe the `At ` too?
`At` is part of library output, So I would not recommend stripping it manually.
From a Length perspective, I think most of the time the length should be manageable even with the `Run:`,
let me know your thoughts.
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r703248151
##########
File path: airflow/www/templates/airflow/dag.html
##########
@@ -91,11 +91,15 @@ <h4 class="pull-right" style="user-select: none;-moz-user-select: auto;">
<a class="label label-default" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}">
Schedule: {{ dag.schedule_interval }}
</a>
+ {% if dag.timetable.interval_description is not none %}
+ <span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="Runs: {{ dag.timetable.interval_description|string }}">info</span>
+ {% endif %}
Review comment:
sure, let me check,
but as far as I know we always get DagModel object in dag.html,
@uranusjr what are the cases where we will have Dag object.
--
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 change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r729024231
##########
File path: airflow/timetables/interval.py
##########
@@ -221,6 +223,20 @@ def infer_manual_data_interval(self, *, run_after: DateTime) -> DataInterval:
end = self._get_prev(self._align(run_after))
return DataInterval(start=self._get_prev(end), end=end)
+ @cached_property
+ def interval_description(self) -> Optional[str]:
Review comment:
Since this value is for UI only, I feel it's better to just return an empty string instead.
Comments for the protocol definition about caching also apply 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
[GitHub] [airflow] uranusjr commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r729022127
##########
File path: airflow/models/dag.py
##########
@@ -2521,6 +2521,10 @@ def __init__(self, concurrency=None, **kwargs):
def __repr__(self):
return f"<DAG: {self.dag_id}>"
+ @cached_property
+ def timetable(self) -> Timetable:
+ return create_timetable(self.schedule_interval, self.timezone)
Review comment:
Starting with Airflow 2.2, the user can specify a custom timetable for the DAG that's not backed by a `schedule_interval` value, and this won't work for those DAGs. So instead of adding a dynamic property, `DagModel` should gain a `timetable_description` column that's populated when the `DagModel` is updated from the source `DAG` instance.
--
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] pateash commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r670954242
##########
File path: airflow/timetables/base.py
##########
@@ -83,6 +83,13 @@ def interval(cls, start: DateTime, end: DateTime) -> "DagRunInfo":
class Timetable(Protocol):
"""Protocol that all Timetable classes are expected to implement."""
+ @property
+ def interval_description(self) -> [str, None]:
Review comment:
Agree, but I think Optional[] is more descriptive, as it means that there is no description available rather.
--
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] ryanahamilton commented on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-879107733
Since some of these can be lengthy, I'm wondering if the `Runs: ` prefix is superfluous? Maybe the `At ` too?
--
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 edited a comment on pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-888179851
I think we need to delay this a bit until we find a way to properly serialise `DAG.timetable` to `DagModel` (and `SerializedDagModel`, but that’s another matter). I will work on that in the coming weeks, and once that’s figured out, this should be much more straightforward to implement. The timetable thing is scheduled to be released on 2.2, so this should also make it into that release as well.
--
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 pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-882368742
--
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] pateash commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
pateash commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-944879588
> What will it show for 6th param in cron expression? croniter doesn't follow the convention about it see [taichino/croniter#176](https://github.com/taichino/croniter/issues/176) It's important that the description string will strict to croniter otherwise it will show a misleading description
It will consider the first one as the second in the case of 6 parameters and show something like this.
@uranusjr @ashb How do we schedule in case of 6 or 7 Params in Cron Expression?
--
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 change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r670821620
##########
File path: airflow/timetables/base.py
##########
@@ -83,6 +83,13 @@ def interval(cls, start: DateTime, end: DateTime) -> "DagRunInfo":
class Timetable(Protocol):
"""Protocol that all Timetable classes are expected to implement."""
+ @property
+ def interval_description(self) -> [str, None]:
+ """Defines the interval description,
+ eg. for cron '30 21 * * 5', description could be like 'At 09:30 PM, only on Friday'
+ """
Review comment:
```suggestion
"""Override to describe the interval.
For cron ``'30 21 * * 5'``, description could be like ``'At 09:30 PM, only on Friday'``.
This is used in the web UI.
"""
```
##########
File path: airflow/timetables/interval.py
##########
@@ -79,6 +80,20 @@ class CronDataIntervalTimetable(_DataIntervalTimetable):
def __init__(self, cron: str, timezone: datetime.tzinfo) -> None:
self._schedule = CronSchedule(cron, timezone)
+ @Timetable.interval_description.getter
Review comment:
I think just `@property` would do the same (well technically a bit different, but arguably better).
##########
File path: airflow/timetables/base.py
##########
@@ -83,6 +83,13 @@ def interval(cls, start: DateTime, end: DateTime) -> "DagRunInfo":
class Timetable(Protocol):
"""Protocol that all Timetable classes are expected to implement."""
+ @property
+ def interval_description(self) -> [str, None]:
Review comment:
```suggestion
def interval_description(self) -> Optional[str]:
```
Or we can make this `str` and return an empty string when a description is not available, since an empty description does not make sense in the UI anyway. Instead of `if dag.timetable.interval_description is not None` we can just do `if not dag.timetable.interval_description`.
##########
File path: airflow/models/dag.py
##########
@@ -2404,6 +2404,20 @@ def __repr__(self):
def timezone(self):
return settings.TIMEZONE
+ @property
+ def timetable(self) -> Timetable:
+ interval = self.schedule_interval
+ if interval is None:
+ return NullTimetable()
+ if interval == "@once":
+ return OnceTimetable()
+ if isinstance(interval, (timedelta, relativedelta)):
+ return DeltaDataIntervalTimetable(interval)
+ if isinstance(interval, str):
+ return CronDataIntervalTimetable(interval, self.timezone)
+ type_name = type(interval).__name__
+ raise TypeError(f"{type_name} is not a valid DAG.schedule_interval.")
Review comment:
Instead of duplicating the logic on `DagModel`, we should reuse `DAG.timetable` instead. But I’m not sure how this should be done.
--
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] ryanahamilton commented on a change in pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r668817504
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -163,6 +163,11 @@ <h2>{{ page_title }}</h2>
<a class="label label-default schedule" href="{{ url_for('DagRunModelView.list') }}?_flt_3_dag_id={{ dag.dag_id }}" data-dag-id="{{ dag.dag_id }}">
{{ dag.schedule_interval }}
</a>
+
+ {% if dag.schedule_interval_description is not none %}
+ <span id='schedule-description-{{ dag.safe_dag_id }}' class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="{{ dag.schedule_interval_description|string }}">info</span>
Review comment:
One last nit:
```suggestion
<span id="schedule-description-{{ dag.safe_dag_id }}" class="material-icons text-muted js-tooltip" aria-hidden="true" data-original-title="{{ dag.schedule_interval_description|string }}">info</span>
```
--
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 pull request #16931: #16692 show cron description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-888179851
I think we need to delay this a bit until we find a way to properly serialise `DAG.timetable` to `DagModel` (and `SerializedDagModel`, but that’s another matter). I will work on that in the coming weeks, and once that’s figured out, this should be much more straightforward to implement. The serialisation thing is scheduled to be released on 2.2, so this should also make it into that release as well.
--
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] github-actions[bot] commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-991384406
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
[GitHub] [airflow] uranusjr commented on pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#issuecomment-945378085
I don't think we support the 7th segment. The 6th has always been a problem since croniter handles it incorrectly (as mentioned above), so we just leave it undefined for the moment. There's actually an open issue to explicitly detect the 6th segment and show a warning so we can more easily migrate to the "correct" behaviour when possible. #16731
For now, though, I think we should avoid dealing with the problem entirely, and put an artificial limit so _description is not shown for 6- and 7-segment cron expressions_.
--
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 change in pull request #16931: #16692 show schedule_interval/timetable description in UI
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16931:
URL: https://github.com/apache/airflow/pull/16931#discussion_r733641121
##########
File path: airflow/models/dag.py
##########
@@ -2706,6 +2707,7 @@ class DagModel(Base):
# Earliest time at which this ``next_dagrun`` can be created.
next_dagrun_create_after = Column(UtcDateTime)
+ timetable_description = Column(String, nullable=True)
Review comment:
Maybe this can be better groupped under `schedule_interval` instead? That column is actually `timetable.summary` now but we just didn't bother to rename it.
##########
File path: airflow/timetables/base.py
##########
@@ -103,6 +103,12 @@ def logical_date(self) -> DateTime:
class Timetable(Protocol):
"""Protocol that all Timetable classes are expected to implement."""
+ description: Optional[str] = None
+ """Describes the timetable interval,
+ eg. for CronDataIntervalTimetable ``'30 21 * * 5'``,
+ description could be like ``'At 21:30, only on Friday'``, This is used in the webserver UI.
+ """
Review comment:
```suggestion
"""Human-readable description of the timetable.
For example, this can produce something like ``'At 21:30, only on Friday'``
from the cron expression ``'30 21 * * 5'``. This is used in the webserver UI.
"""
```
Format this to be more in line with other docstrings on this class.
##########
File path: airflow/timetables/simple.py
##########
@@ -54,6 +54,9 @@ class NullTimetable(_TrivialTimetable):
This corresponds to ``schedule_interval=None``.
"""
+ def __init__(self) -> None:
+ self.description = "Never, external triggers only"
Review comment:
This can just be a class variable:
```python
class NullTimetable(_TrivialTimetable):
description = "Never, external triggers only"
```
(Same goes for `OnceTimetable` below)
##########
File path: airflow/timetables/interval.py
##########
@@ -124,6 +125,19 @@ def __init__(self, cron: str, timezone: Timezone) -> None:
self._expression = cron_presets.get(cron, cron)
self._timezone = timezone
+ descriptor = ExpressionDescriptor(
+ expression=self._expression, casing_type=CasingTypeEnum.Sentence, use_24hour_time_format=True
+ )
+ try:
+ # checking for more than 5 parameters in Cron and avoiding evaluation for now,
+ # as Croniter has inconsistent evaluation with other libraries
+ if self._expression.count(" ") > 4:
+ raise FormatException()
Review comment:
This could be fragile for inputs like `*/5 * * * *` (notice the extra space between the second and third segment). It may be better to use `split()` here instead, or rely on `croniter`'s parser:
```pycon
>>> from croniter import croniter
>>> len(croniter('*/5 * * * *').expanded)
5
>>> len(croniter('*/5 * * * * 1').expanded)
6
```
--
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