Add chrono 0.4 integration

[Psykopear]

This PR adds the integration for chrono 0.4, and it supersedes #2604
Most of the work here was done by @pickfire , I just adapted it to work with the published version of chrono.

Here the list of PR that brought us here:
chronotope/chrono#542
#2604
pickfire#1
chronotope/chrono#812

Thanks to @pickfire for doing the hard work, and to @djc and @davidhewitt for the reviews and help during the whole process.

Closes #2604

[mejrs]

Can you add an entry to https://docs.rs/pyo3/latest/pyo3/#optional-feature-flags as well? :)

[davidhewitt]

Thanks, the code is looking pretty good, added some comments with bits which I think we can tweak.

Have you got experience with proptest? I think it could be worth adding some tests which run Rust -> Python -> Rust round-trips using proptest, as I'm a bit worried there are edge cases in the conversions and it would be good to identify them (even if we choose to live with them).

Also, please add newsfragments/2612.packaging.md with contents

Added optional `chrono` feature to convert `chrono` types into types in the `datetime` module.

[Psykopear]

Great, thanks everybody for the review, I'll work on the suggestions tomorrow

[pickfire]

I think the proptests here make some of the test we wrote previously reduntant. Do you want me to remove them, or is it ok to keep both?

The existing tests test out some hard-coded known bound values, I think would be nice to keep them, I think proptest does test it but it's more like a blackbox.

[Psykopear]

Looks like CI is failing due to throtlling from github, I think I triggered way too many runs today, sorry.

[davidhewitt]

Nice, use of proptests looks good!

The existing tests test out some hard-coded known bound values, I think would be nice to keep them, I think proptest does test it but it's more like a blackbox.

Agreed, proptest is great for sniffing out weird edge cases and then it can be nice to make them actual tests so that anyone working on a new implementation sees all the known edge cases.

I'm not entirely sure about the PyDelta normalization either, if it's the same as the done documented for datetime.timedelta() I think we probably do want this. Is there a FixedOffset::west for negative offsets?

I also am a little murky on whether we're doing the right thing when we convert datetimes into DateTime<FixedOffset> and DateTime<Utc>. Do we accept the right varieties of timezones and how do these conversions interact with IANA timezones?

If we need to look at supporting chrono-tz here, I would be ok with us exploring it.

[pickfire]

I also am a little murky on whether we're doing the right thing when we convert datetimes into DateTime and DateTime. Do we accept the right varieties of timezones and how do these conversions interact with IANA timezones?

I didn't look at this previously because it does not come from datetime and instead could come from zoneinfo or pytz. Was also thinking of keeping the scope small and just include stuff from datetime so only utc and fixed offset is added.

[Psykopear]

Talking about converting datetimes, I'm still making my mind about it, but I'm starting to have a clearer vision now.

I think PyO3 should just represent datetimes as they are represented in python:

• Python naive -> Chrono naive
• Python fixed offset -> Chrono fixed offset
• Python UTC -> Chrono UTC
• Python with proper timezone info (pytz or zoneinfo) -> chrono_tz

Here I assume chrono/chrono_tz because that's what we are working on right now, but this is true for any other date time library.

Ideally naive datetimes should never be exposed to users or sent around the internet, but that's not the case in the real world, so we have to deal with them.

Users of this library will have to decide what to do in case of naive datetimes.
In python naive datetimes are assumed to be in the same (local) timezone of the interpreter that instanced the datetime (see also the red warning here).
If a user of PyO3 can reasonably assume that the datetime was generated in the same timezone their code is ran, they can decide to convert it to either UTC/FixedOffset or to chrono_tz, or even just decide to only use naive datetimes, otherwise they can just panic or apply whatever strategy they want to.

But I feel like this is not something PyO3 should decide, since here we can't assume that datetimes were generated on the same timezone where the conversion takes place (think about parsing a naive datetime coming from any source on the internet).

Given these considerations, I think we should:

• Expose a conversion for Python naive -> Chrono naive
• Add (in a different PR, using a different feature since requirements would be different) a chrono_tz integration for Python with IANA tzinfo

About the ToPyObject implementation for DateTime<Tz>.
Right now we convert it to a fixed offset representation.
This is not wrong, since we are doing it for a specific date, so there should be no ambiguity (apart from future changes in timezones definition, which is not exactly rare, but we can't do better without proper IANA timezones use), and it would work for any python that doesn't have IANA timezones info (so python<3.9 without pytz or backports.zoneinfo packages).
But if a PyO3 user wants to properly support IANA timezones, they could require python>=3.9 or the presence of a library, and then use a separate chrono_tz integration.
The ToPyObject for DateTime<Tz> would have to be removed from the chrono feature if the chrono_tz feature is enabled too, since a ToPyObject for DateTime<chrono_tz::Tz> would be a conflicting implementation (at least I think I ran into this poroblem when I tried), but that's doable.
With chrono_tz we should also be able to add the FromPyObject given the requirements mentioned before, I already tried that in another library and it seems to work.

[davidhewitt]

Yep, I agree with all of the above. So I think I'm comfortable that the design we've got here is now correct.

Just a few comments to finish off on the FixedOffset conversions, and then I'm tempted to include this in PyO3 0.17.2 (coming soon).

[Psykopear]

Ok, I should have addressed the last comments:

• Added a comment on why we pass None to utcoffset
• Added a test confirming that ZoneInfo is not converted to FixedOffset
• Improved the error message, tested that too
• Fixed the FixedOffset conversion, normalized the timedelta
• I also added the NaiveDateTime translation since it was missing, given the considerations on my last comment.

I didn't find a way to properly exclude the ZoneInfo test for python < 3.9 using cfg features, so I'm just testing it if the import zoneinfo works on python, see the comment on the test.

[davidhewitt]

So close now! Just some final comments and then hopefully good to merge. Thanks for all the work on the many iterations of this 🙂

[Psykopear]

The CI fails on windows with this error:

ZoneInfoNotFoundError('No time zone found with key Europe/London'),

The reason is that windows does not offer a system IANA database:
https://docs.python.org/3/library/zoneinfo.html#data-sources

So we'd need to install the tzdata package in CI, or skip this test on windows.

edit: I'm looking at other PRs on how to do this, is it ok to add something like:

    if platform.system() == "Windows":
        session.install("tzdata>=2022.2")

in the test function in the noxfile?

edit2: Mh, I think that's only related to python tests though, here we'd need a python dependency for rust tests, so maybe just a python -m pip install tzdata somewhere in the workflow if the platform is windows?

[davidhewitt]

I'd prefer the rust tests to not have an external Python dependency, so I think easiest for now to just skip that test on Windows.

[davidhewitt]

This looks complete to me! Test coverage looks excellent and I'm feeling confident the implementation behaves as users would expect.

Thank you very much @Psykopear, @pickfire, @mejrs and @djc for all the hard work here - it's been quite the epic journey for this feature. Will release it into 0.17.2 in the near future.

[Psykopear]

Great! Thanks everybody, this was really a team effort

[messense]

Some proptests failed in https://github.com/PyO3/pyo3/actions/runs/3104312627/jobs/5028714517

[Psykopear]

I'm going to check what went wrong, where should I commit the fix? A separate branch and PR?

[messense]

where should I commit the fix? A separate branch and PR?

Yes.

[Psykopear]

The test correctly crashes because I'm trying to initialize a FixedOffset with an out of bound value, but somehow running the proptest locally did not fail, so I missed this. I need to learn more about proptests I think, sorry for the inconvenience.

PR opened: #2636