-
Notifications
You must be signed in to change notification settings - Fork 16.4k
fix: Persist EventsTimetable's description during serialization #51203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
32b6d88 to
cc230fb
Compare
|
Can we also backport it to 2.X as well if merged? |
|
This fix is awkward. I'd prefer the serialisation logic to just store the provided value, and supply it correctly on deserialisation. This shouldn't require an extra flag. |
|
It's implemented this way because when providing description to the EventsTimetable, both |
|
Why not fix the value during deserialisation? Like @classmethod
def deserialize(cls, data) -> Timetable:
timetable = cls(...)
timetable._summary = ...
return timetable |
cc230fb to
40152b3
Compare
|
Thanks for the advice, that make sense. Adjusted the code |
34d6af7 to
b0a8efe
Compare
b0a8efe to
430060f
Compare
|
Anything else that needs to be adjusted? |
|
Should we also backport it to 2.11 or fix it there? I think the difference is not visible on the UI but f.e. when listener is accessing dag timetable summary. |
|
I don’t have an opinion either way. This is not very meaningful from the user’s perspective, but also very easy to backport so why not. |
|
Okay, I think it will be useful, so if it's not a big effort then let's do it. Does this PR need a specific label only or there is something else that needs to be done? |
|
I think this should work, the rest will be automated after this is merged. |
|
Should we merge it? |
apache#51203) (cherry picked from commit a150e70) Co-authored-by: Kacper Muda <mudakacper@gmail.com>
The test misses timetable description after recent change in apache#51203
The test misses timetable description after recent change in #51203
The test misses timetable description after recent change in apache#51203
When using EventsTimetable, the schedule in Airflow UI is not shown correctly/. My guess is that the EventsTimetable description (and therefore summary) not being serialized is to blame.
In Airflow 3 we are relying on timetable.summary when displaying schedule in the UI. Currently, even if user provide a description Airflow UI will always display "X events" as timetable_summary, instead of description provided by the user.

In Airflow 2 we kept the timetable summary as DAG attribute and serialize it separately, so it always showed up in UI correctly.

This is also causing discrepancies in OpenLineage events, for DagRun events emitted from scheduler (for Airflow 2 and 3), we are receiving "X events", and for task events emitted from worker, the user description.
Example DAG to test it:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.