Skip to content

Update to PyO3 0.22 #66

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

Merged
merged 7 commits into from
Aug 3, 2024

Conversation

HenningHolmDE
Copy link
Contributor

This bumps the PyO3 dependency to the new 0.22 release.

As PyNativeType (used in src/de.rs) has now been moved behind the gil-refs feature, I added that feature to the list of PyO3 feature dependencies.
As Clone (used in tests/test_custom_types.rs) has now been moved behind the py-clone feature, I added that feature to the list of PyO3 feature dependencies for development.

However, if you would rather go down a different route with the upstream changes than just adding the required features (e.g. removing the corresponding deprecated functions instead), I'm happy to change the PR accordingly.

@HenningHolmDE HenningHolmDE marked this pull request as draft June 25, 2024 19:57
@HenningHolmDE
Copy link
Contributor Author

As it turns out, simply enabling gil-refs seems not the right way to go, as it introduces trait bounds to HasPyGilRef for downstream projects as well.

I will see, if I can come up with a better way.

Switched to draft until this is resolved.

This removes all code that relies on deprecated PyO3 functionality that
has now been moved behind the `gil-refs` feature and removes
intermittent use of the feature.
@HenningHolmDE
Copy link
Contributor Author

Second try.
I now reverted using the gil-refs feature and removed all code that relied on PyO3 functionality that has been moved behind the gil-refs feature gate in the new release.

I'm sorry for the seesaw and hope that the change makes more sense now.

@HenningHolmDE HenningHolmDE marked this pull request as ready for review June 25, 2024 20:45
@juntyr
Copy link
Contributor

juntyr commented Jun 26, 2024

I personally think that as long as Pyo3 still has the pre-bound methods, pythonize should probably have them as well.

What about adding a gil-refs feature to pythonize that gates the old methods and enables that feature inside Pyo3?

On the other hand I can also understand waning to reduce the maintenance burden earlier in this smaller helper crate.

@HenningHolmDE
Copy link
Contributor Author

I personally think that as long as Pyo3 still has the pre-bound methods, pythonize should probably have them as well.

What about adding a gil-refs feature to pythonize that gates the old methods and enables that feature inside Pyo3?

I would agree, adding a gil-refs feature seems like the better solution over removing the code as long as the required functions are still available in PyO3.

On the other hand I can also understand waning to reduce the maintenance burden earlier in this smaller helper crate.

Also, this would extend the CI effort a bit, as we would have to ensure building all feature "combinations" does work.

Anyone else thoughts on this?

@deedy5
Copy link

deedy5 commented Jul 9, 2024

up

Copy link

codecov bot commented Aug 3, 2024

Codecov Report

Attention: Patch coverage is 94.04762% with 5 lines in your changes missing coverage. Please review.

Project coverage is 83.83%. Comparing base (30ce873) to head (e7cf258).

Files Patch % Lines
src/ser.rs 93.97% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
+ Coverage   81.25%   83.83%   +2.58%     
==========================================
  Files           3        3              
  Lines        1152     1169      +17     
  Branches     1152     1169      +17     
==========================================
+ Hits          936      980      +44     
+ Misses        167      140      -27     
  Partials       49       49              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidhewitt
Copy link
Owner

Sorry for the long delay here. Thanks for the PR and getting this moving! I think it's ok to remove the gil-refs methods as we'll be removing them in the next PyO3 release anyway.

I'll get this merged and released ASAP. Have pushed some commits to finish off & fix conflicts.

@HenningHolmDE
Copy link
Contributor Author

Thanks for your reply and no worries about the delay. I guess, most of us are doing this in our spare time. ❤️

If you run into anything that needs further work, just let me now, I'm happy to support.

@davidhewitt davidhewitt merged commit 6c2c3f2 into davidhewitt:main Aug 3, 2024
22 checks passed
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.

4 participants