Skip to content
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

Make prev_logical_date variable offset-aware #29454

Merged
merged 3 commits into from
Feb 14, 2023
Merged

Conversation

akrava
Copy link
Contributor

@akrava akrava commented Feb 9, 2023

Closes: #29227


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Feb 9, 2023
@akrava akrava marked this pull request as ready for review February 10, 2023 00:04
@eladkal eladkal added this to the Airflow 2.5.2 milestone Feb 10, 2023
@jacobhurlburt
Copy link

Testing locally prev_logical_date = DateTime.min is also an option. Might be preferable since it is consistent with the move to use pendulum going forward.

@dimberman
Copy link
Contributor

Hi @akrava can you please add a failable test to this? IT would be great if we can have some way to ensure there isn't a regression in the future.

@bbovenzi bbovenzi mentioned this pull request Feb 13, 2023
@pierrejeambrun pierrejeambrun added the type:bug-fix Changelog: Bug Fixes label Feb 13, 2023
Copy link
Contributor

@josh-fell josh-fell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on adding a test to prevent future regression that @dimberman mentioned.

I don't have a preference on using Datetime instead. I'll defer to @pierrejeambrun and @bbovenzi there.

@akrava
Copy link
Contributor Author

akrava commented Feb 14, 2023

Thanks @jacobhurlburt and @dimberman for suggestions. I've changed to DateTime.min from pendulum library - and it works fine. Also I've added test for this case - this error is reproducing only when DAG has non-cron schedule (like time interval). Here is code:

airflow/airflow/www/views.py

Lines 2853 to 2869 in 47b67f1

if isinstance(dag.timetable, CronMixin):
# Optimized calendar generation for timetables based on a cron expression.
dates_iter: Iterator[datetime | None] = croniter(
dag.timetable._expression,
start_time=last_automated_data_interval.end,
ret_type=datetime,
)
for dt in dates_iter:
if dt is None:
break
if dt.year != year:
break
if dag.end_date and dt > dag.end_date:
break
dates[dt.date()] += 1
else:
prev_logical_date = datetime.min

From the code above we can see that non offset-aware datetime.min is running when isinstance(dag.timetable, CronMixin) is False. That's why I've added test case with latest_only DAG from example folder, because the schedule of this DAG is time interval, not cron:
with DAG(
dag_id="latest_only",
schedule=dt.timedelta(hours=4),

@akrava akrava requested review from pierrejeambrun and bbovenzi and removed request for pierrejeambrun February 14, 2023 16:17
@bbovenzi bbovenzi merged commit f837c01 into apache:main Feb 14, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 7, 2023
* Make `prev_logical_date` variable offset-aware

* Test Calendar page for DAG with non-cron like schedule

* Use pendulum DateTime.min which is offset-aware

(cherry picked from commit f837c01)
pierrejeambrun pushed a commit that referenced this pull request Mar 8, 2023
* Make `prev_logical_date` variable offset-aware

* Test Calendar page for DAG with non-cron like schedule

* Use pendulum DateTime.min which is offset-aware

(cherry picked from commit f837c01)
@rdzsp
Copy link

rdzsp commented Mar 14, 2023

Hello, I'm struggling with this issue. I'm installing Airflow by following this instruction https://airflow.apache.org/docs/apache-airflow/stable/installation/installing-from-pypi.html. But I see on this pull request seems fix the problem. Is your documentation not update yet?

@potiuk
Copy link
Member

potiuk commented Mar 14, 2023

Hello, I'm struggling with this issue. I'm installing Airflow by following this instruction https://airflow.apache.org/docs/apache-airflow/stable/installation/installing-from-pypi.html. But I see on this pull request seems fix the problem. Is your documentation not update yet?

If you want an official release, you have to wait for the new release to be published - now it is being voted - but you can help with testing it by installing 2.5.2rc2 version - it would be great if you test it and post information about it in #30028 - like others did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calendar page doesn't load when using a timedelta DAG schedule
9 participants