-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Make core compatible with pendulum 3
#34744
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
4b3d5f8 to
2fc1d4e
Compare
pendulum 3
uranusjr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally lgtm, just a couple of nits
bolkedebruin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions and small nits. Generally looks fine, thanks for putting in the work.
airflow/utils/timezone.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an issue moving to Pendulum 3 UTC? So moving from FixedTimezone to Timezone? FixedTimezone derives from Timezone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is interesting question. In general speaking right now we use two different version of UTC timezone in airflow codebase.
FixedTimezonecreated by pendulum.tz.timezone("UTC") in most part of codebaseTimezonecreated by pendulum.tz.timezone.Timezone("UTC") in timetables
This is a two different objects with different attributes however it should work exactly the same. So it might be a good idea to try to resolve different implementation of UTC timezone in airflow codebase, I just choose to keep the same as it now in most parts, but I happy to change it to pendulum.tz.timezone.Timezone("UTC")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bolkedebruin
Well as we could see on our CI Timezone("UTC") in pendulum 2.x might not work into some specific cases such
airflow/airflow/example_dags/plugins/workday.py
Lines 51 to 52 in 5623575
| else: | |
| holidays = holiday_calendar.holidays(start=next_start, end=next_start).to_pydatetime() |
This code works fine in pendulum 3, however it failed (or spawn a lot of warnings) in 2.1.2
from pendulum.tz.timezone import FixedTimezone, Timezone
from datetime import datetime
from pandas.tseries.holiday import USFederalHolidayCalendar
holiday_calendar = USFederalHolidayCalendar()
for utc in [FixedTimezone(offset=0, name="UTC"), Timezone("UTC")]:
print(f" {utc} {type(utc).__name__} ".center(72, "="))
next_start = datetime(2021, 9, 5, tzinfo=utc)
result = holiday_calendar.holidays(start=next_start, end=next_start)
print(f"Result: {next_start}")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And revert it back to FixedTimezone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and what about using
from datetime import timezone
utc = timezone.utcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check this one, some of the methods pendulum 2 have a problem with non-pendulum's timezones, e.g. ZoneInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then leave it for now, we can switch later
Python 3.12 has a few breaking changes comparing to earlier versions. While 3.7 - 3.11 were largely backwards compatible, Python 3.12 is the first one for a long time that started to break things more aggressively. For now we know that Airflow will not work with Python 3.12 mainly because of distutils removal (https://peps.python.org/pep-0632/) and not because of Airflow's usage of it but pendulum's before version 3. While we are working on getting Pendulum 3 support in apache#34744 and the apache#34746, there are likely other dependencies that have similar issue. Until we fix it and add official 3.12 support, we can limit airflow to not be installable on 3.12.
2fc1d4e to
e4232e9
Compare
Python 3.12 has a few breaking changes comparing to earlier versions. While 3.7 - 3.11 were largely backwards compatible, Python 3.12 is the first one for a long time that started to break things more aggressively. For now we know that Airflow will not work with Python 3.12 mainly because of distutils removal (https://peps.python.org/pep-0632/) and not because of Airflow's usage of it but pendulum's before version 3. While we are working on getting Pendulum 3 support in #34744 and the #34746, there are likely other dependencies that have similar issue. Until we fix it and add official 3.12 support, we can limit airflow to not be installable on 3.12.
Python 3.12 has a few breaking changes comparing to earlier versions. While 3.7 - 3.11 were largely backwards compatible, Python 3.12 is the first one for a long time that started to break things more aggressively. For now we know that Airflow will not work with Python 3.12 mainly because of distutils removal (https://peps.python.org/pep-0632/) and not because of Airflow's usage of it but pendulum's before version 3. While we are working on getting Pendulum 3 support in #34744 and the #34746, there are likely other dependencies that have similar issue. Until we fix it and add official 3.12 support, we can limit airflow to not be installable on 3.12. (cherry picked from commit 020691f)
|
Thank you for your PR, bro! |
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: bolkedebruin <bolkedebruin@users.noreply.github.com>
e4232e9 to
ee27310
Compare
|
@bolkedebruin Would you have a look again? Do you have any concern? |
potiuk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
LGTM with one small question see above. |
|
I've cherry picked changes into the #35798 and it fail in multiple new places, so I would suggest to close this one, and And continue working in that PR so we would sure that changes also would potentially work in pendulum 3 |
|
closed in favor of #35798 |
In pendulum 3 (beta) there is no access to
pendulum.tz.timezoneanymore otherwise we should callpendulum.timezonefor convert string/integer to pendulum timezones^ 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 newsfragments.