Support serialising to named mappings #62

juntyr · 2024-05-18T04:55:23Z

This PR includes four changes:

Implement PythonizeListType for PyTuple, a small convenience that cannot be achieved outside the crate
Add support for prebuilding mappings during serialisation, which delays the collection of mapping key-value pairs until all are known
Add support for named mappings during serialisation, which adds a tiny default-implemented helper trait method to pass forward the struct / tuple / enum name to the mapping type
Allow deserializing an enum from any mapping instead of just dicts

I have found them to be very useful to serialise to and from class-like Python types (e.g. namedtuple which keep more type information around).

juntyr · 2024-05-27T13:31:23Z

@davidhewitt what are your thoughts on this?

juntyr · 2024-06-13T07:41:23Z

@davidhewitt I apologise for pinging again - I'm hoping that this PR could move forward soon-ish as I'm working on publishing a crate of mine (which would need a feature similar to this) for a publication.

MarshalX · 2024-06-13T08:45:49Z

@juntyr Perhaps he unfollowed this repository or pull request. Maybe try pinging him on X, for example, as he has a few social links on his GitHub profile

davidhewitt · 2024-06-14T11:23:41Z

Hi @juntyr sorry for the delay! I have seen this PR but haven't had a chance to investigate yet. Please hold on and I'll do my best to get to this soon - I'm trying to wrap up PyO3 0.22 and then let's aim to ship this in pythonize 0.22 👍

juntyr · 2024-06-26T06:11:44Z

@davidhewitt Congrats on shipping pyo3 v0.22, I'm excited to start using it!

I'm looking forward to collaborating with you to push this PR forward :)

juntyr · 2024-08-03T19:23:28Z

The merge conflict should now be resolved again :)

juntyr · 2024-08-03T19:29:17Z

Once #66 is merged, I will very quickly rebase this PR again so that we can hopefully still get it into v0.22

davidhewitt

Thanks very much for this and sorry for the slow reply. Yes let's get this into 0.22. Can you please add CHANGELOG entries too?

I think overall I'm 👍 for this and understand the motivation, however want to just discuss a couple of the design choices especially with the intention to avoid performance overhead.

davidhewitt · 2024-08-03T21:18:46Z

src/ser.rs

 pub trait PythonizeDictType {
    /// Constructor
    fn create_mapping(py: Python) -> PyResult<Bound<PyMapping>>;
+
+    /// Constructor
+    fn create_mapping_with_items<
+        K: ToPyObject,
+        V: ToPyObject,
+        U: ExactSizeIterator<Item = (K, V)>,
+    >(
+        py: Python,
+        items: impl IntoIterator<Item = (K, V), IntoIter = U>,
+    ) -> PyResult<Bound<PyMapping>> {
+        let mapping = Self::create_mapping(py)?;
+
+        for (key, value) in items {
+            mapping.set_item(key, value)?;
+        }
+
+        Ok(mapping)
+    }
+
+    /// Constructor, allows the mappings to be named
+    fn create_mapping_with_items_name<
+        'py,
+        K: ToPyObject,
+        V: ToPyObject,
+        U: ExactSizeIterator<Item = (K, V)>,
+    >(
+        py: Python<'py>,
+        name: &str,
+        items: impl IntoIterator<Item = (K, V), IntoIter = U>,
+    ) -> PyResult<Bound<'py, PyMapping>> {
+        let _name = name;
+        Self::create_mapping_with_items(py, items)
+    }
 }


I have a couple of thoughts here:

I wonder, is the split between create_mapping_with_items and create_mapping_with_items_name a bit confusing? Should we potentially instead model this by having PythonizeTypes have a NamedMap type to handle the named cases?

I slightly worry about the performance regression here by adding an intermediate Vec. I think the way to solve this would be to replace create_mapping with create_builder(len), which returns an associated type implementing trait MappingBuilder { fn push_item(k, v); fn finish() -> Bound<'py, PyMapping> }. For the default dict implementation, the builder is then just a thin wrapper around the dict, in the named tuple case the builder can do the pre-collecting into a Vec.

I implemented both suggestions. I generally like both, even though they are more infective than the previous change. Perhaps you have some suggestions for how to further improve them?

codecov · 2024-08-04T08:34:13Z

Codecov Report

Attention: Patch coverage is 75.25773% with 24 lines in your changes missing coverage. Please review.

Project coverage is 83.41%. Comparing base (6c2c3f2) to head (7c18b9f).

Files	Patch %	Lines
src/ser.rs	77.77%	4 Missing and 16 partials ⚠️
src/de.rs	42.85%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
- Coverage   83.83%   83.41%   -0.42%     
==========================================
  Files           3        3              
  Lines        1169     1224      +55     
  Branches     1169     1224      +55     
==========================================
+ Hits          980     1021      +41     
- Misses        140      144       +4     
- Partials       49       59      +10

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

juntyr · 2024-08-04T09:32:58Z

GATs were only stabilised in 1.65 but pythonize uses 1.63 … so the builder can’t use a GAT. Perhaps I can inline the builder methods into the main trait itself (hopefully while keeping Map = PyDict)

juntyr · 2024-08-04T10:39:29Z

I also tried to add a small convenience wrapper type PythonizeUnnamedMappingWrapper so that

it is more explicit that type NamedMap = PythonizeUnnamedMappingWrapper<PyDict> discards the name
it is reusable for others so you don't need to copy-past two trait impls

davidhewitt

Looking great, thanks for the repeated rounds and sorry again for the delays!

Just a few finishing touches...

tests/test_custom_types.rs

src/ser.rs

davidhewitt

Looks perfect, thanks! One final small thing to check, and once we've agreed on that, let's merge 👍

src/ser.rs

The adapter is anyways constructed through the builder method Co-authored-by: David Hewitt <mail@davidhewitt.dev>

juntyr · 2024-08-07T07:09:51Z

Thanks for your review @davidhewitt!

juntyr · 2024-08-07T20:04:51Z

I’m unsure if the coverage can be easily improved as all new misses seem to be partial and short-circuiting related

davidhewitt · 2024-08-07T21:48:20Z

Agreed, I'm very happy to call this as good as it gets! Thanks again 🚀

juntyr force-pushed the named-mapping-items branch from 23565b6 to a95c5fe Compare August 3, 2024 19:21

davidhewitt reviewed Aug 3, 2024

View reviewed changes

juntyr and others added 3 commits August 4, 2024 03:46

Implement PythonizeListType for PyTuple

96ae2e2

Add support for prebuilding (named) mappings during serialisation

6cd708b

Allow deserializing an enum from any mapping

28f7bec

juntyr force-pushed the named-mapping-items branch from a95c5fe to 28f7bec Compare August 4, 2024 04:00

juntyr added 3 commits August 4, 2024 04:55

Implement suggested API changes

cc2015e

Add CHANGELOG entry

5211ccf

Use static names

40140ac

juntyr added 2 commits August 4, 2024 10:31

Don't use GATs

f3ff8a9

Amend the CHANGELOG

aa8f2ab

Add test for PythonizeListType impl for PyTuple

dade974

juntyr force-pushed the named-mapping-items branch from 4972787 to dade974 Compare August 4, 2024 10:44

davidhewitt reviewed Aug 6, 2024

View reviewed changes

tests/test_custom_types.rs Outdated Show resolved Hide resolved

src/ser.rs Outdated Show resolved Hide resolved

src/ser.rs Outdated Show resolved Hide resolved

src/ser.rs Outdated Show resolved Hide resolved

src/ser.rs Outdated Show resolved Hide resolved

juntyr added 3 commits August 6, 2024 20:39

Apply code review comments

893a1bc

Add short docs for the unnamed mapping adapter type

38d7a05

Add test for unnamed and named mappings

aa961a9

davidhewitt approved these changes Aug 6, 2024

View reviewed changes

src/ser.rs Outdated Show resolved Hide resolved

Remove constructor from type-level-only adapter

13df99f

The adapter is anyways constructed through the builder method Co-authored-by: David Hewitt <mail@davidhewitt.dev>

Fix fmt and clippy

7c18b9f

davidhewitt merged commit d7f54f1 into davidhewitt:main Aug 7, 2024
20 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support serialising to named mappings #62

Support serialising to named mappings #62

juntyr commented May 18, 2024

juntyr commented May 27, 2024

juntyr commented Jun 13, 2024

MarshalX commented Jun 13, 2024

davidhewitt commented Jun 14, 2024

juntyr commented Jun 26, 2024

juntyr commented Aug 3, 2024

juntyr commented Aug 3, 2024

davidhewitt left a comment

davidhewitt Aug 3, 2024

juntyr Aug 4, 2024

codecov bot commented Aug 4, 2024 •

edited

Loading

juntyr commented Aug 4, 2024

juntyr commented Aug 4, 2024

davidhewitt left a comment

davidhewitt left a comment

juntyr commented Aug 7, 2024

juntyr commented Aug 7, 2024

davidhewitt commented Aug 7, 2024

Support serialising to named mappings #62

Support serialising to named mappings #62

Conversation

juntyr commented May 18, 2024

juntyr commented May 27, 2024

juntyr commented Jun 13, 2024

MarshalX commented Jun 13, 2024

davidhewitt commented Jun 14, 2024

juntyr commented Jun 26, 2024

juntyr commented Aug 3, 2024

juntyr commented Aug 3, 2024

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt Aug 3, 2024

Choose a reason for hiding this comment

juntyr Aug 4, 2024

Choose a reason for hiding this comment

codecov bot commented Aug 4, 2024 • edited Loading

Codecov Report

juntyr commented Aug 4, 2024

juntyr commented Aug 4, 2024

davidhewitt left a comment

Choose a reason for hiding this comment

davidhewitt left a comment

Choose a reason for hiding this comment

juntyr commented Aug 7, 2024

juntyr commented Aug 7, 2024

davidhewitt commented Aug 7, 2024

codecov bot commented Aug 4, 2024 •

edited

Loading