migrate `PyDictMethods` trait bounds #4493

Icxolu · 2024-08-25T15:49:19Z

This migrates PyDictMethods to IntoPyObject and cleans up the map impls. I was not super sure how we want to deal with IntoPyDict, so I added a proposal as a second commit, but we can also drop that for now, if we need more discussion for that. In case we want to leave it like this, I will add changelog and migration entries.

LilyFoote

The first commit looks good to me.

davidhewitt

Thanks, agreed on the first commit looking great! Some suggestions on the second commit...

davidhewitt · 2024-09-30T20:24:12Z

src/types/dict.rs

+pub trait IntoPyDict<'py>: Sized {
    /// Converts self into a `PyDict` object pointer. Whether pointer owned or borrowed
    /// depends on implementation.
-    fn into_py_dict(self, py: Python<'_>) -> Bound<'_, PyDict>;
+    fn into_py_dict(self, py: Python<'py>) -> Bound<'_, PyDict>;


I think we can take the opportunity here to make the return type fallible? AFAIK one of the most likely panics on current conversion traits is unhashable keys.

(This method is new for 0.23 replacing the removed GIL Ref form)

That makes sense, I will adapt that 👍

davidhewitt · 2024-09-30T20:26:35Z

src/types/dict.rs

 /// Represents a tuple which can be used as a PyDict item.
-pub trait PyDictItem {
-    type K: ToPyObject;
-    type V: ToPyObject;
-    fn key(&self) -> &Self::K;
-    fn value(&self) -> &Self::V;
+pub trait PyDictItem<'py> {
+    type K: IntoPyObject<'py>;
+    type V: IntoPyObject<'py>;
+    fn unpack(self) -> (Self::K, Self::V);
 }


It's kind of breaking, but I can't really see a better way forward. Maybe we just mention this in the migration guide? Most users are not implementing / using this trait, I think.

Actually I think PyDictItem is completely internal to PyO3 and is invisible from the outside (it's only mentioned inside dict.rs which is a pub(crate) module). So maybe we just remove pub here to make that clear? Also we should update the example that's already in the migration guide in that regard. I will fix it up together with the fallibility mention.

Ah perfect, even better! We might need to have pub for the trait implementation, not sure.

I removed it and nothing complained, so it guess it's fine 😁

davidhewitt · 2024-09-30T20:27:30Z

src/types/dict.rs

@@ -557,76 +557,73 @@ pub(crate) use borrowed_iter::BorrowedDictIter;

 /// Conversion trait that allows a sequence of tuples to be converted into `PyDict`
 /// Primary use case for this trait is `call` and `call_method` methods as keywords argument.
-pub trait IntoPyDict: Sized {
+pub trait IntoPyDict<'py>: Sized {


I guess the alternative is keep the existing form around for a while and add a new trait IntoPyDictObject or similar which we can eventually rename back to just this. But I'm unsure how much value there really is in that, for this small corner of the API.

I have the feeling that this is mostly used to get access to the constructors we provide and is probably not often implemented on custom types. So I think it would likely cause more churn doing the renaming business.

Agreed, let's keep with this current pathway 👍

davidhewitt

Looks great to me, thanks! Basically just some closing thoughts / observations remaining. Please feel free to merge 👍

src/conversions/indexmap.rs

davidhewitt · 2024-10-01T19:42:28Z

src/types/dict.rs

@@ -557,76 +557,73 @@ pub(crate) use borrowed_iter::BorrowedDictIter;

 /// Conversion trait that allows a sequence of tuples to be converted into `PyDict`
 /// Primary use case for this trait is `call` and `call_method` methods as keywords argument.
-pub trait IntoPyDict: Sized {
+pub trait IntoPyDict<'py>: Sized {


Agreed, let's keep with this current pathway 👍

davidhewitt · 2024-10-01T19:46:56Z

src/types/dict.rs

        for item in self {
-            dict.set_item(item.key(), item.value())
-                .expect("Failed to set_item on dict");
+            let (key, value) = item.unpack();
+            dict.set_item(key, value)?;
        }


(Probably one for a future PR) I guess after I read https://medium.com/@veedrac/rust-is-slow-and-i-am-the-cure-32facc0fdcb we should eventually consider try_for_each or similar iterator algorithms for these loops. Potentially extra relevant after #4439.

Interesting read, thanks for linking. Might be interesting to do some benchmarks whether we can actually measure any difference. Sounds good for a future PR for me too.

davidhewitt · 2024-10-01T19:48:57Z

src/types/dict.rs

 /// Represents a tuple which can be used as a PyDict item.
-pub trait PyDictItem {
-    type K: ToPyObject;
-    type V: ToPyObject;
-    fn key(&self) -> &Self::K;
-    fn value(&self) -> &Self::V;
+pub trait PyDictItem<'py> {
+    type K: IntoPyObject<'py>;
+    type V: IntoPyObject<'py>;
+    fn unpack(self) -> (Self::K, Self::V);
 }


Ah perfect, even better! We might need to have pub for the trait implementation, not sure.

Co-authored-by: David Hewitt <[email protected]>

Icxolu added the CI-skip-changelog Skip checking changelog entry label Aug 25, 2024

LilyFoote reviewed Aug 25, 2024

View reviewed changes

Icxolu mentioned this pull request Sep 13, 2024

0.23 Release #4554

Closed

3 tasks

davidhewitt reviewed Sep 30, 2024

View reviewed changes

Icxolu added 2 commits October 1, 2024 18:59

migrate PyDictMethods trait bounds

96cb7d0

migrate IntoPyDict

50e4b38

Icxolu force-pushed the migrate-pydict branch from 3563ef4 to 5a05e27 Compare October 1, 2024 18:25

Icxolu removed the CI-skip-changelog Skip checking changelog entry label Oct 1, 2024

Icxolu force-pushed the migrate-pydict branch from 5a05e27 to 53c10b2 Compare October 1, 2024 18:54

make IntoPyDict::into_py_dict fallible

b40bb14

Icxolu force-pushed the migrate-pydict branch from 53c10b2 to c19c579 Compare October 1, 2024 19:06

update migration guide

e2e4f45

Icxolu force-pushed the migrate-pydict branch from c19c579 to e2e4f45 Compare October 1, 2024 19:18

davidhewitt approved these changes Oct 1, 2024

View reviewed changes

remove unnecessary clone

4a3ac02

Co-authored-by: David Hewitt <[email protected]>

Icxolu enabled auto-merge October 1, 2024 20:20

Icxolu added this pull request to the merge queue Oct 1, 2024

Merged via the queue into PyO3:main with commit faed5e2 Oct 1, 2024
40 of 42 checks passed

Icxolu deleted the migrate-pydict branch October 1, 2024 23:45

This was referenced Oct 2, 2024

ci: fix benchmarks build #4591

Merged

set_item consumes, but uses ToPyObject instead of IntoPyObject #480

Closed

Icxolu mentioned this pull request Oct 9, 2024

Contribution opportunity: improved conversion traits #4041

Closed

bschoenmaeckers mentioned this pull request Oct 25, 2024

Use try_for_each in into_py_dict #4647

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate `PyDictMethods` trait bounds #4493

migrate `PyDictMethods` trait bounds #4493

Icxolu commented Aug 25, 2024

LilyFoote left a comment

davidhewitt left a comment

davidhewitt Sep 30, 2024

Icxolu Oct 1, 2024

davidhewitt Sep 30, 2024

Icxolu Oct 1, 2024

davidhewitt Oct 1, 2024

Icxolu Oct 1, 2024

davidhewitt Sep 30, 2024

Icxolu Oct 1, 2024

davidhewitt Oct 1, 2024

davidhewitt left a comment

davidhewitt Oct 1, 2024

davidhewitt Oct 1, 2024

Icxolu Oct 1, 2024

davidhewitt Oct 1, 2024

migrate PyDictMethods trait bounds #4493

migrate PyDictMethods trait bounds #4493

Conversation

Icxolu commented Aug 25, 2024

LilyFoote left a comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

migrate `PyDictMethods` trait bounds #4493

migrate `PyDictMethods` trait bounds #4493