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

Refs #32365 -- [PoC] Remove dependency on pytz #13073

Closed
wants to merge 1 commit into from

Conversation

pganssle
Copy link
Contributor

@pganssle pganssle commented Jun 16, 2020

Update: see ticket-32365

Now that PEP 495 and PEP 615 have been accepted, there's not much reason to continue using pytz, and its non-standard interface is a major source of bugs.

This PR is an attempt to see how difficult it would be to remove the dependency on pytz and replace it with a PEP 495-compatible time zone library.

Because I expect you'll have a bunch of end users who will be relying on pytz's specific interface, instead of going straight for backports.zoneinfo, I've replaced it with pytz_deprecation_shim, which is a library I wrote for the purpose of allowing libraries to gracefully deprecate pytz wherever it has leaked into their public interface. There are some semantic differences between the shims and pytz (as evidenced by some of the changes I've had to make here), but I've kept the semantics of all pytz-specific functions as close as possible (without being bug-for-bug compatible, mind you). End users using the .localize and .normalize functions or accessing the .zone attribute will get a warning.

This PR is still a draft while I work out some of the kinks. Since it's a proof-of-concept, I haven't tried re-writing the documentation.

I am also not really a Django user, so I'm not super sure whether users are allowed to supply their own time zones for some of this stuff. If so, it may be necessary to make some adjustments to account for the possibility that they would set a pytz time zone.

@felixxm
Copy link
Member

felixxm commented Jun 17, 2020

Thanks for this patch, however we'll not decide for an extra requirement. We can reconsider this patch when Python 3.9 becomes the minimal Python supported by Django.

@felixxm felixxm closed this Jun 17, 2020
@adamchainz
Copy link
Sponsor Member

@felixxm I disagree, I think it's safe to depend on these packages. @pganssle is the author of the PEP and Python 3.9 feature, and I encouraged him to look into moving Django. Doing this earlier will help users move to the Python standard library way of doing things before they move to Python 3.9, and become aware of the issues with pytz.

We should maybe have a mailing list discussion.

@pganssle
Copy link
Contributor Author

pganssle commented Jun 17, 2020

Yes, I guess after it's closed it loses "Draft" status, but as I mentioned in the original PR text I opened this PR as a testing ground to see how hard it would be to get the CI passing after the migration. Looks like the required changes are pretty minimal (though it did bring up some minor backwards compatibility issues I wasn't familiar with).

I had planned to bring a proper proposal to the mailing list when I have some more time and energy to do so.

I can't say that suddenly closing my PoC PR while still in draft status is the friendliest reception I've gotten (particularly since it appears that non-committers don't have the option to re-open), but I understand if you don't want random contributors wasting their time, so I can understand why you wouldn't want to delay.

@carltongibson
Copy link
Member

We should maybe have a mailing list discussion.

Yes... 🙂

Thanks for your input @pganssle — please don't take the response the wrong way.

I saw this fly past on Twitter one-time. Then I saw this PR. I really don't see that Django needs to be in the early adopters crowd here. There are a lot of projects with a lot smaller install base, and a lot quicker release cycle that can adopt it quickly and surface the issues before we need to jump in.

I'm all for progress, but not really for experimenting with Django. (The bottom line is we introduce too many regressions for my liking already; we try hard not to but...)

@ngnpope
Copy link
Member

ngnpope commented Jun 17, 2020

FWIW I also disagree. Timezone handling is a mess and now that there is a clear way forward, and a migration strategy, it would make sense for this to be made easier for users of Django.

I certainly don't understand why a draft pull request with clear stated aims of being a proof-of-concept to ascertain the feasibility of this improvement should be closed in such an abrupt, unfriendly way.

@carltongibson
Copy link
Member

carltongibson commented Jun 17, 2020

Gentlefolks, I also think you need to remember that we're all on the same side here, and that what you as native speakers interpret as "unfriendly" is in fact no more than an matter of fluency of expression — nuances of tone are not easy or automatic. Ta.

Smiley face: 🙂

@pganssle
Copy link
Contributor Author

I'll also note that you'll need something like pytz-deprecation-shim anyway, even after Python 3.9 is the minimal version. Either that or it will be a very hard break for end users. A minimal version of the package can be vendored into Django (one without 2.7 support, for example), but of course my time is limited and support for the package as vendored would lag behind the upstream package.

@felixxm
Copy link
Member

felixxm commented Jun 17, 2020

... such an abrupt, unfriendly way.

That wasn't my intention. I don't see any issue in reopening it when we reach a consensus on the mailing list.

Please note that the discussion about making pytz a required dependency took us 4 months.

@pganssle
Copy link
Contributor Author

That wasn't my intention. I don't see any issue in reopening it when we reach a consensus on the mailing list.

Well the issue is that it's not done (hence "draft" status) and now further commits in this branch are not going to trigger CI. In fact, the existing (passing) CI hooks are now fairly hidden, which will be annoying when I show this off as a proof of concept. I'll also have to open a new PR if I'm going to keep this up to date with rebases against master.

I don't really think we need to get bogged down in what is and is not friendly, I only mentioned this because it was very discouraging to have my draft PR closed with no option to re-open and no discussion.

There are a lot of projects with a lot smaller install base, and a lot quicker release cycle that can adopt it quickly and surface the issues before we need to jump in.

Well, I do know one project with a strictly larger install base and a quite long release cycle that has already adopted zoneinfo 😉. Regardless, I was not intending for this PR to become the center of discussion, since I didn't even take it out of draft status or open a TRAC ticket, but since it's attracting a lot of discussion already, I suppose it's time for me to start a thread on the mailing list.

@felixxm felixxm changed the title Remove dependency on pytz [PoC] Remove dependency on pytz Jun 17, 2020
@felixxm felixxm reopened this Jun 17, 2020
@ngnpope
Copy link
Member

ngnpope commented Jun 17, 2020

I'll also note that you'll need something like pytz-deprecation-shim anyway, even after Python 3.9 is the minimal version.

This for me is a major point. It seems to make sense to approach this now rather than wait for Python 3.9 to be the minimum version supported.

If we wait for 3.9 to be the minimum version we're looking at October 2024 for Python 3.8 end-of-life. At which point we'll have Django 4.2 LTS running to April 2026 and Django 5.0 to April 2025.

And then we think about deprecating pytz over another couple of releases minimum? So that is maybe six years out, assuming pytz lasts that long...

It looks as though pytz-deprecation-shim could be used earlier to discourage the use of the pytz-specific API while keeping backward compatibility until Python 3.9 is the minimal version supported.

It seems prudent to warn early with this stuff to give ample time to migrate.

@carltongibson
Copy link
Member

Yes... but there's middle ground between waiting until the last minute and being first in the line. I'd be inclined to something like, see how the 3.9 rollout goes, watch a few other project adopt, and then, if smooth, think about it, for say 4.0, which gives plenty of time before the 4.2 LTS.

Anyhow... mailing list.

@pganssle
Copy link
Contributor Author

I've sent a detailed rationale with some suggested first steps to the mailing list here, for people who find this issue but aren't sure where to go for further discussion.

@carltongibson
Copy link
Member

Hi @pganssle. Thanks for that. Super email! Very clear.

@carltongibson
Copy link
Member

For folks following it here, I posted on the mailing list topic: I think we should move to using the shim for Django 4.0, to be removed in Django 5.0. Please though do input, because it's tricky (I'm not sure there is a perfect answer.)

@carltongibson
Copy link
Member

Noting here from the discussion that we'll likely proceed with the shim approach here for Django 4.0 (after the next LTS), dropping at Django 5.0.

@pganssle has an invite to help him also add an early opt-in for Django 3.2:

... If anyone wants to jump on either of these ahead of me I don't mind at all and feel free to ping me for review. ...

@pope1ni — you're on my suspects list for someone who might be up for that. 😀

@carltongibson
Copy link
Member

Hi @pganssle — Hope you're doing OK!

Starting to look towards our feature freeze for the 3.2 LTS in January… You said you may have bandwidth to come back to this to add a initial feature flag that will opt-in to zone info from 3.2. Do you have that capacity? If so super! 🦸

If not I think we'll need to implement that anyhow. @pope1ni — you're on my suspects list for someone who might take that on. Is that something you'd have capacity to look at?

Just organising a bit of hustle. Thanks 😀

@ngnpope
Copy link
Member

ngnpope commented Jan 6, 2021

Hi @carltongibson.

Sorry - I didn't have any capacity for looking into this. I replied on the mailing list and see that Aymeric has suggested an opt-out, rather than opt-in, approach. I quite like this idea as I feel that fewer people are using pytz correctly anyway and it encourages them to think about making these changes sooner rather than later. The opt-out allows more time for those with more complex usage.

@carltongibson
Copy link
Member

Hey @pope1ni — Happy New Year!

No stress, yes — let's look at Aymeric's straight-to-the-goal for 4.0.

Thanks! 👍

@carltongibson carltongibson changed the title [PoC] Remove dependency on pytz Refs #32365 -- [PoC] Remove dependency on pytz Jan 19, 2021
@carltongibson
Copy link
Member

Tracking this now on ticket-32365. I'll close this here. Thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants