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

Use dynamic dispatch when converting chrono timezones into python #4923

Closed

Conversation

bschoenmaeckers
Copy link
Contributor

@bschoenmaeckers bschoenmaeckers commented Feb 19, 2025

This fixes #4909 by removing the constraint for Tz to be IntoPyObject and instead using dynamic dispatch, albeit at the cost of some performance.

I'm not sure if we want to go this route in future pyo3 releases and instead require users to implement IntoPyObject for custom TimeZone implementations or let them convert it into a standard chrono one. If so then this PR should be merged into the 0.23 branch only.

@bschoenmaeckers bschoenmaeckers force-pushed the chrono-dyn-tz branch 2 times, most recently from 94d7b3f to 9ff76a2 Compare February 19, 2025 12:48
@davidhewitt
Copy link
Member

Hmm, I think I prefer the static dispatch personally. But I think that this would be also potentially a (silent?) breaking change to backport because it would break any users that have implemented IntoPyObject for their custom Tz types?

So we might be stuck? 😟

cc @kylebarron

@bschoenmaeckers
Copy link
Contributor Author

I think I prefer the static dispatch personally.

My preference is definitely static dispatch as well.

But I think that this would be also potentially a (silent?) breaking change to backport because it would break any users that have implemented IntoPyObject for their custom Tz types?

IntoPyObject for custom Tz's did not used to work before pyo3 0.23.4 so we would be restoring the original behaviour for custom TimeZone types in 0.23. So it would not be a breaking change for 0.23 but only a fix of a breaking change right?

@bschoenmaeckers bschoenmaeckers changed the base branch from main to release-0.23 February 19, 2025 14:07
@davidhewitt
Copy link
Member

Yes, but at least the 0.23.3 -> 0.23.4 change was breaking at compile time, merging this would then break the new behaviour silently for any users who are using the 0.23.4 features.

@davidhewitt
Copy link
Member

Agreed that 0.23.3 -> 0.23.4 change was unfortunate, though I still think it's better to not thrash it further 😭

@bschoenmaeckers
Copy link
Contributor Author

Agreed that 0.23.3 -> 0.23.4 change was unfortunate, though I still think it's better to not thrash it further 😭

Sure closing this for now.

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.

Breaking change handling Arrow Tz (implementing Chrono Tz) in 0.23.4
2 participants