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

disable PyO3 "gil refs" feature #1085

Closed
wants to merge 5 commits into from
Closed

disable PyO3 "gil refs" feature #1085

wants to merge 5 commits into from

Conversation

davidhewitt
Copy link
Contributor

Change Summary

Attempt to port to the upcoming PyO3 API as a way to gain performance boosts here.

Still a long way to go, I'll force-push as I go and I suggest not reviewing until this is near the end, as there will be a lot of temporary churn 😂

Related issue number

N/A

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #1085 (d1b3565) into main (ab503cb) will increase coverage by 0.25%.
Report is 29 commits behind head on main.
The diff coverage is 94.20%.

❗ Current head d1b3565 differs from pull request most recent head f081d3e. Consider uploading reports for the commit f081d3e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1085      +/-   ##
==========================================
+ Coverage   90.21%   90.46%   +0.25%     
==========================================
  Files         106      106              
  Lines       16339    16716     +377     
  Branches       36       36              
==========================================
+ Hits        14740    15122     +382     
+ Misses       1592     1587       -5     
  Partials        7        7              
Files Coverage Δ
src/errors/location.rs 97.16% <100.00%> (ø)
src/errors/mod.rs 92.30% <100.00%> (ø)
src/errors/types.rs 99.68% <100.00%> (+0.23%) ⬆️
src/errors/value_exception.rs 91.07% <100.00%> (ø)
src/input/datetime.rs 98.76% <100.00%> (-0.01%) ⬇️
src/input/shared.rs 97.19% <100.00%> (+0.02%) ⬆️
src/serializers/computed_fields.rs 97.70% <100.00%> (+0.03%) ⬆️
src/serializers/config.rs 95.37% <100.00%> (+0.92%) ⬆️
src/serializers/extra.rs 99.53% <100.00%> (+<0.01%) ⬆️
src/serializers/filter.rs 96.53% <100.00%> (+0.08%) ⬆️
... and 87 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1083986...f081d3e. Read the comment docs.

Copy link

codspeed-hq bot commented Nov 21, 2023

CodSpeed Performance Report

Merging #1085 will improve performances by 30.63%

Comparing dh/py2 (f081d3e) with main (1083986)

Summary

⚡ 16 improvements
✅ 132 untouched benchmarks

Benchmarks breakdown

Benchmark main dh/py2 Change
test_core_future 55 µs 42.1 µs +30.63%
test_core_future 29.2 µs 25.7 µs +13.42%
test_date_from_datetime 38.4 µs 34 µs +12.87%
test_date_from_str 27.4 µs 23.3 µs +17.67%
test_core_root_model 95.3 µs 82.6 µs +15.35%
test_definition_in_tree 4.5 ms 4 ms +12.41%
test_list_of_nullable_core 491.4 µs 408.7 µs +20.24%
test_tuple_many_positional 33.8 µs 30 µs +12.42%
test_tuple_many_variable 33.6 µs 30 µs +12.05%
test_validate_literal[json-few_mixed] 29.8 µs 26.4 µs +12.58%
test_validate_literal[python-few_mixed] 27 µs 23.7 µs +13.55%
test_validate_literal[python-few_str_enum] 24.9 µs 21.2 µs +17.36%
test_core_model_json 50.2 µs 44.1 µs +13.81%
test_dataclass_serialization_json 44.8 µs 40 µs +12%
test_json_any_list_int 1,034.2 µs 918.8 µs +12.56%
test_to_string_direct 32.2 µs 28.5 µs +13%

@davidhewitt
Copy link
Contributor Author

@decathorpe, @mgorny,

(please forgive the direct pings here)

We're keen to move ahead with this here on pydantic-core to improve performance and segmentation faults, but are aware that depending on a git branch of PyO3 may cause headaches for distro packaging teams.

So, the question I wanted to ask you, is it good enough for Fedora and Gentoo packaging if we release a PyO3 0.21 beta and depend on that in jiter and pydantic-core? Or maybe a git branch on PyO3 is actually ok?

Hopefully this situation would be temporary while PyO3 0.21 development finishes, at most a month...

@decathorpe
Copy link

Dependencies on git branches / commits are very awkward and annoying to deal with for us in Fedora - though technically possible (basically, we would need to vendor pyo3, since our build environment has no internet access). Using a pre-release (like 0.21.0-beta.1) on the other hand would fit into our normal workflows - other than the fact that we normally don't package pre-releases unless really necessary.

@davidhewitt
Copy link
Contributor Author

Thanks, I will likely seek blessing from fellow PyO3 maintainers to do a prerelease in that case.

@mgorny
Copy link
Contributor

mgorny commented Jan 18, 2024

If by git repository, you mean the thing that has source = "git+https://github.com/..." in Cargo.lock, then we can handle that in Gentoo. However, real crates are ofc preferable.

@davidhewitt davidhewitt changed the title Update to use new PyO3 API update to PyO3 0.21.0-beta.0 Mar 10, 2024
@davidhewitt davidhewitt changed the title update to PyO3 0.21.0-beta.0 disable PyO3 "gil refs" feature Mar 12, 2024
@davidhewitt davidhewitt mentioned this pull request Mar 12, 2024
4 tasks
@davidhewitt
Copy link
Contributor Author

Merged in #1222

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.

3 participants