#16692 show schedule_interval/timetable description in UI

[pateash]

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 ( cron or predefined presets ).
User can also choose to use Timetable API and this works as well.

Description implemented for Once and Null Timetable and we can easily extend it to support others going forward.

Predefined Presets are also evaluated

[potiuk]

LGTM. I know others have some opinions on which description is good so If you want to chime in - feel free before I merge it.

[github-actions]

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.

[ryanahamilton]

Since some of these can be lengthy, I'm wondering if the Runs: prefix is superfluous? Maybe the At too?

[pateash]

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.

[ryanahamilton]

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.

[pateash]

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.

[uranusjr]

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:

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:

'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.

[pateash]

@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.

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.

[uranusjr]

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.

[pateash]

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.

[uranusjr]

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.

[pateash]

I agree @uranusjr,
let's wait for your change.
Could you please mention your PR/Issue here, so we can track it.

[uranusjr]

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.

[pateash]

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 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.

[pateash]

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.

[uranusjr]

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.

[pateash]

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.

[uranusjr]

We should also add something to the documentation for this in docs/apache-airflow/howto/timetable.rst

[uranusjr]

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.

[pateash]

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.

[github-actions]

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.

[pateash]

@uranusjr there were some CI failures due to kerberos authentication.
I hope they are sorted already.

[uranusjr]

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.