PyO3 / pyo3

Rust bindings for the Python interpreter
https://pyo3.rs
Apache License 2.0
12.39k stars 765 forks source link

Add more `IntoPyObject` impls #4446

Closed Icxolu closed 3 months ago

Icxolu commented 3 months ago

This adds more IntoPyObject impls, mostly for reference types, plus a few that I missed in the first round. I generally tried to provide one where ever we also have a ToPyObject impl, but there could be still some that I missed somewhere. They will hopefully show up once I start migrating trait bounds.

Icxolu commented 3 months ago

How should we go about testing these? Probably some level of coverage will be reached once we start migrating off the older traits, though it would be nice to drive that coverage closer to 100% if possible.

I definitely agree here, we should strive to keep coverage high. Maybe we can try to implement the IntoPy<PyObject>/ToPyObject impls in terms of IntoPyObject impls, that would make sure that they're in sync and also cover them with the existing tests. What do you think of that.

davidhewitt commented 3 months ago

Maybe we can try to implement the IntoPy/ToPyObject impls in terms of IntoPyObject impls, that would make sure that they're in sync and also cover them with the existing tests.

👍 I think that'd be a good way to reduce the amount of duplication, and then as we rotate out the old traits all that will be left behind is thin wrappers which won't be a huge concern if deprecated & not covered.

There might be a few cases where the old traits can't use these new ones (e.g. different bytes specialization behaviour maybe?), where I guess we just live with the duplication.

Icxolu commented 3 months ago

There might be a few cases where the old traits can't use these new ones (e.g. different bytes specialization behaviour maybe?), where I guess we just live with the duplication.

True, I guess all generic types can't do this, because of the bounds... Anyway doing it for "leaf" types will be better than not doing at all.

codspeed-hq[bot] commented 3 months ago

CodSpeed Performance Report

Merging #4446 will improve performances by 17.36%

Comparing Icxolu:intopyobject-ref (2ab5a01) with main (144b199)

Summary

⚡ 1 improvements ✅ 76 untouched benchmarks

Benchmarks breakdown

Benchmark main Icxolu:intopyobject-ref Change
bytes_new_small 394.4 ns 336.1 ns +17.36%
Icxolu commented 3 months ago

I looks like coverage has improved significantly from that. Now its pretty much just the generic collections (and for a lot of them just the reference) impls that are not covered. (I also found a few impls that I've missed previously, so I added those as well)