Add chrono 0.4 integration

[Psykopear]

This PR adds the integration for chrono 0.4, and it supersedes https://github.com/PyO3/pyo3/pull/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: https://github.com/chronotope/chrono/pull/542 https://github.com/PyO3/pyo3/pull/2604 https://github.com/pickfire/pyo3/pull/1 https://github.com/chronotope/chrono/pull/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

[Psykopear]

@davidhewitt I think the PR is ready to be reviewed. I still get CI failures on clippy, because it's trying to compile the integration with the limited python api, but I think in this case it doesn't make much sense since this integration needs it, right? How would you like to proceed here?

[Psykopear]

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

[Psykopear]

Ok, I applied the suggested changes, moved to using the C API for offsets, added a couple of tests for bounds checking, added the newsfragment for the changelog and rebased everything on the main branch.

The CI is now finally passing!

@davidhewitt never used proptest, but the readme is quite clear, and I see it's already used in the types/num module, so I'll try using it for datetimes too and see if I catch any other edge case.

[Psykopear]

Wow, I really like proptests. I hope I used it properly, please review the tests and let me know if you want me to change anything. I added tests for the roundtrip of all the involved types with all the allowed values. There are values that are valid in Rust but will panic when converting to python, so I only proptested for values that are allowed in Python. Some considerations

FixedOffset

The roundtrip conversion of FixedOffset didn't work as it is. During the trip, FixedOffset::east for negative values turned into a positive one: -00:00:01 turns into 23:59:59. I made it work by not normalizing the PyDelta when converting the FixedOffset to Python, but I'm not sure this is the right approach. Can we have problems using the delta in python without the normalization? I added a comment marked XXX where this happens.

DateTime<Tz>

This can't be tested this way with proptest, because we don't have a FromPyObject for DateTime<Tz>. We convert DateTime<Tz> to fixed offset datetimes in python, so if we get anything back it's turned into a DateTime<FixedOffset>.

I think to be able to convert DateTime<Tz> (other than FixedOffset and Utc) back and forth from Python, we'd need to integrate chrono_tz and require the zoneinfo module in python (so python >=3.9), so this is probably out of scope for this PR. Would it make sense to add a separate integration to properly handle IANA timezones?

Other tests

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?

[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.

[pickfire]

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?

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.

[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.

[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.

[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