Add pyo3 integration

[pickfire]

Some parts taken from https://github.com/kangalioo/pyo3-chrono

Note, I will continue working on this in few more days, I have something I wanted to experiment in the meantime.

cc @kangalioo

Implemented

• PyDate <-> NaiveDate
• PyTime <-> NaiveTime
• PyDateTime <-> NaiveDateTime
• PyDelta <-> Duration (only implemented for oldtime feature)
• PyDate <-> Date (may confuse people since PyDate does not have timezone)
• PyTzInfo <-> Utc
• PyTzInfo <-> FixedOffset
• PyDateTime <-> DateTime

Thanks for contributing to chrono!

• Have you added yourself and the change to the changelog? (Don't worry
  about adding the PR number)
• If this pull request fixes a bug, does it add a test that verifies that
  we can't reintroduce it?

[pickfire]

Not sure how can we add pyo3 integration for Duration from time crate since we cannot impl for another crate, do we need to do a wrapper type or just remove support for pyo3 for time crate?

[Milo123459]

Not sure how can we add pyo3 integration for Duration from time crate since we cannot impl for another crate, do we need to do a wrapper type or just remove support for pyo3 for time crate?

You need a wrapper type by the looks of it

[pickfire]

@davidhewitt Do you know if there is any way that we are able to compare timezone? I tried comparing with Eq but even though both have the same value it didn't seemed to pass.

[pickfire]

Sorry for the long wait, I was waiting for 2018 edition move but it didn't seemed to happen (and probably not anytime soon). Currently I a couple of types that involves timezone (DateTime, Utc, FixedOffset) cannot be converted back to rust, one part is because pyo3 does not seemed to provide access to timezone information, another reason is I think it probably is not that useful as naive date/time, maybe that can come later.

Not sure where would be best to document these missing stuff.

[pickfire]

Rebased to latest master with 2018 edition and got it compiling, need to double check where I left off.

[djc]

What's the use case for this? Why would it be helpful/good for this to live inside chrono instead of in a separate crate? I like PyO3 and have worked with it before, but it will take a while before I'm comfortable adding a bunch of extra stuff to chrono given the lack of maintenance this crate has been seeing for a while.

[pickfire]

What's the use case for this? Why would it be helpful/good for this to live inside chrono instead of in a separate crate? I like PyO3 and have worked with it before, but it will take a while before I'm comfortable adding a bunch of extra stuff to chrono given the lack of maintenance this crate has been seeing for a while.

So next time when users use pyo3, chrono integration would be seamless, adding it in this crate allows using the internal representation of chrono and allows to convert directly to python. An alternative would be to add it to pyo3 but that have the same limitation as adding it as a separate crate, at least the integration is just like serde where you can just add a feature and use it rather than having to look up a separate crate, the pyo3 stuff is pretty much separated so it does not add that much of maintenance.

Adding it to a separate crate would require adding newtypes to implement the pyo3 traits due to orphan rules and may be quite inflexible in case users need the original types from chrono.

[djc]

Okay, that does make some sense.

In terms of how we implement this, I think I'd prefer to have a single top-level pyo3 module with a module hierarchy within that mirrors the internal chrono hierarchy (instead of scattering pyo3 code over the internal hierarchy).

[pickfire]

I moved most parts into pyo3 except oldtime since there are some internal functions within oldtime.

Also figured out how to convert from python utc.

I guess two more things left is to figure out conversion from python fixedoffset, I guess that we can just subtract the time from python to ours, I have some rough idea on this now. After that the last thing is datetime.

I guess I will convert it to draft for those two items but it should be good for review for most parts now.

[pickfire]

Weird, not sure why stuff used to passed and now starts failing due to associated functions visibility.

[davidhewitt]

Sorry for the long delay, finally took a quick look at this.

Regarding segfaults, looks like you've got incorrect ownership. The below:

        unsafe {
             PyDateTime_IMPORT();
             PyObject::from_owned_ptr(py, PyDateTime_TimeZone_UTC())
         }

should be

        unsafe {
             PyDateTime_IMPORT();
             PyObject::from_borrowed_ptr(py, PyDateTime_TimeZone_UTC())
         }

Because PyDateTime_TimeZone_UTC() returns a borrowed pointer (not owned).

@djc - I wonder if you have an opinion where this integration should live? Given that chrono is actively maintained again I'm reasonably comfortable with having chrono as an optional dependency in PyO3, although I see comments above imply there are internal chrono APIs used here for performance. Moving this to PyO3 may not be so good for performance in that case?

[davidhewitt]

(I resurrected PyO3/pyo3#1588 which I'll probably merge shortly for PyO3 0.17, which would remove the need for the unsafe code here.)

[djc]

@djc - I wonder if you have an opinion where this integration should live? Given that chrono is actively maintained again I'm reasonably comfortable with having chrono as an optional dependency in PyO3, although I see comments above imply there are internal chrono APIs used here for performance. Moving this to PyO3 may not be so good for performance in that case?

I think it would make more sense for the PyO3/chrono integration to live in the PyO3 repository than in the chrono repository: (1) the chrono side of this is relatively low-complexity compared to the PyO3 integration bits, (2) PyO3 is arguably a higher-level dependency than chrono and (3) I think a chrono integration provides more value to the average PyO3 user than a PyO3 integration would provide to the average chrono user (maybe this is a restatement of 2?).

If we identify extra API surface that we need to expose to make that work well, that's certainly a worthy trade-off.

[pickfire]

Because PyDateTime_TimeZone_UTC() returns a borrowed pointer (not owned).

Just curious, how did you find out about that? I did see two versions, one from_owned_ptr and one from_borrowed_ptr, but I don't know which one to use, so I just use one of it (both seemed to work for me most of the time besides segfault).

[davidhewitt]

"owned" form basically means it will decrease the python reference count later. TBH the only way to know is to look at the docs for the C API - most return owned pointers, in this case it's a bit murky because the C API is a macro which just expands to the static item and PyO3 has to make a function instead.

@pickfire, given the preference above to have this in PyO3, would you be interested in moving this to the PyO3 repo, perhaps after PyO3/pyo3#1588 merges?

[pickfire]

The thing I see is that Mdf needs to be exposed, so I am not sure if it is a good tradeoff since the internals are being exposed. As well as mdf function.

[pickfire]

And these to get internal values.

[pickfire]

@pickfire, given the preference above to have this in PyO3, would you be interested in moving this to the PyO3 repo, perhaps after PyO3/pyo3#1588 merges?

I am fine doing that, but I feel like that would lose the ability to be able to interface with underlying data directly. And we have to do extra code to transform it back to low level stuff.

[davidhewitt]

I see, that's a reasonable concern.

I agree with @djc that usually pyo3 provides integration with other crates as optional features.

I suspect that the Python ffi calls will dominate over slightly more efficient Rust code anyway. To help inform us, would you be willing to do some benchmarks to compare these implementations to some which don't use chrono internal APIs? (I'm happy to help you design them if needed.)

If the difference is not significant I think we should proceed to move to PyO3, maybe leaving some issues here in chrono requesting access to these more efficient APIs (or variants of them which the chrono team are happy to commit to).

If the difference matters, then I guess it's a choice for the chrono team whether to add APIs now to support the move, or to keep the PyO3 implementation here.

[djc]

I'd be happy to add API surface if that materially speeds up PyO3 access.

[pickfire]

I'd be happy to add API surface if that materially speeds up PyO3 access.

I can think of two ways, one is to add #[doc(hidden)], another way is to add internal feature but not sure how well that works across crates (have to compile multiple times?).

Do I now close this pull request and wait for both pull requests on pyo3 and chrono to be merged then I continue the port to pyo3?

[djc]

Do I now close this pull request and wait for both pull requests on pyo3 and chrono to be merged then I continue the port to pyo3?

I think it makes sense to close this pull request, and submit a PR to the pyo3 repo to add the chrono integration as well as a PR here for the necessary API expansions. For now, let's not do a feature flag or #[doc(hidden)] but just make them part of the public API.

[davidhewitt]

@pickfire I'll try to merge my PyO3 branch this weekend, it would then be possible for you to open both PyO3 and chrono PRs at the same time by using a git dependency on chrono.

[pickfire]

I have something that I wanted to do so I think I will only do this next month.

[davidhewitt]

No problem, this has been blocked on me for like a year already 😅

[Psykopear]

Hi everybody. I'm interested in an integration between pyo3 and chrono (and possibly chrono-tz too).

If I understand correctly, your current idea is to add the integration in PyO3 rather than here, doing something similar to what was done in this PR, and if needed add things to the public api here, is that right?

I'd like to help move things forward. I can start porting this PR to the PyO3 codebase (under a feature flag, in a chrono module inside src/convertions), see what's needed on the chrono side, and open other PRs/issues here to discuss changes to the public api.

@davidhewitt did I get this right? I read a lot of discussions/issues/PRs both here and on the PyO3 repo, I might have missed something
@pickfire are you currently working on this? If so, let me know if I can help

[pickfire]

I tried this again last week, but I noticed that this needs changes on both chrono and time crate, @djc do we have the luxury to be able to change time crate? We need pub const SECS_PER_DAY from time crate.

Thanks for the ping @Psykopear, but I only get to work on this once last week and haven't touch it yet, but I will continue on this for now, not sure what help I need yet, but I guess maybe help me follow up on time crate?

Now I am just slightly stuck at figuring out how to convert from FixedOffset to PyTimeZone because the API changed a bit, previously I only have to convert from FixedOffset to PyObject, but I believe I can figure this out in a moment.

[Psykopear]

good to hear that @pickfire .

I tried this again last week, but I noticed that this needs changes on both chrono and time crate, @djc do we have the luxury to be able to change time crate? We need pub const SECS_PER_DAY from time crate.

I'm sorry I'm not sure I understand the problem here, does chrono (or the chrono integration for PyO3) requires the time crate?
Are we talking about this one: https://github.com/time-rs/time ?

I know chrono used to be built upon time v0.1, but I don't see any reference of that in the current codebase (I ported this PR on top of the current main branch of PyO3).

Now I am just slightly stuck at figuring out how to convert from FixedOffset to PyTimeZone because the API changed a bit, previously I only have to convert from FixedOffset to PyObject, but I believe I can figure this out in a moment.

Yes I was working on that too while porting the PR, I ended up converting it where used (not sure it's the best way, but it works):

        let tz = self.offset().fix().into_py(py);
        let tz: &PyTzInfo = tz.extract(py).unwrap();

Anyway if you are already working on it, I can still help with review and tests

[djc]

@pickfire on main we no longer depend on time, on the 0.4.x branch we still do. I don't think there's a way to get new time 0.1 releases out, no. But for const SECS_PER_DAY, you can just define your own copy right?