Add support for mappingproxy objects by dignissimus · Pull Request #2435 · PyO3/pyo3

dignissimus · 2022-06-05T14:54:59Z

This pull request adds support for mappingproxy objects.

Resolves #2108

…call_method0

davidhewitt

Thanks very much for implementing this!

It's overall very good. To make sure we get this API right when we merge, I've placed quite a few comments around the place with some things that need changing and then also some places where we can discuss the design a little to ensure we make the right choices.

pyo3-ffi/src/descrobject.rs

pyo3-ffi/src/lib.rs

davidhewitt · 2022-06-07T06:53:24Z

src/types/dict.rs

+                let mappingproxy = <PyMappingProxy as PyTryFrom>::try_from(ob)?;
+                mappingproxy.copy()?.extract()


This .copy() call is sub-optimal as it will create a whole new copy as a Python dict first. Instead, we should just create the HashMap from iterating the mapping proxy.

Maybe to make the code reusable we could refactor into a helper something like this:

fn mappingproxy_to_items_sequence<'py, K: FromPyObject<'py>, V: FromPyObject<'py>>(mappingproxy: &'py PyMappingProxy) -> impl IntoIterator<Item = PyResult<(K, V)> { mappingproxy.iter() .map(|(key, value)| Ok((key.extract()?, value.extract()?))) }

and then creating the HashMap should just become

mappingproxy_to_items_sequence(mappingproxy).collect()?

davidhewitt · 2022-06-07T06:53:50Z

src/types/dict.rs

+                }
+                Ok(ret)
+            }
+            Err(_) => {


If the object is not a mappingproxy it would be nice to return this Err(_) from the dict conversion, to preserve existing UX.

Hopefully resolved in c5e434d

davidhewitt · 2022-06-07T07:09:56Z

src/types/mappingproxy.rs

+    /// Creates a new mappingproxy from the sequence given.
+    ///
+    /// The sequence must consist of `(PyObject, PyObject)`.
+    ///
+    /// Returns an error on invalid input. In the case of key collisions,
+    /// this keeps the last entry seen.
+    #[cfg(not(PyPy))]
+    pub fn from_sequence(py: Python<'_>, seq: PyObject) -> PyResult<&PyMappingProxy> {
+        unsafe {
+            let dict = py.from_owned_ptr::<PyDict>(PyDict::from_sequence(py, seq)?.as_ptr());
+            py.from_owned_ptr_or_err(ffi::PyDictProxy_New(dict.as_ptr()))
+        }
+    }


I guess this is just a direct copy of the PyDict equivalent? Depending on what we decide for PyMappingProxy::new, this may or may not be necessary.

I'm thinking that this isn't necessary, users will just be able to call PyMappingProxy::new(PyDict::from_sequence(...).as_mapping())

src/types/mappingproxy.rs

davidhewitt · 2022-06-07T07:15:12Z

src/types/mappingproxy.rs

+    /// Returns a new mappingproxy that contains the same key-value pairs as self.
+    ///
+    /// This is equivalent to the Python expression `self.copy()`.
+    pub fn copy(&self) -> PyResult<&PyDict> {


I don't think &PyDict is correct, looks like it calls copy on the inner mapping:

https://github.com/python/cpython/blob/66f5add1f0ac801cba5ece36e6cfd68985d82029/Objects/descrobject.c#L1135

So the return type should either be &PyAny or (maybe) &PyMapping.

Bump for this.

davidhewitt · 2022-06-07T07:18:37Z

src/types/mappingproxy.rs

+}
+
+#[cfg(test)]
+mod tests {


Very nice set of tests!

davidhewitt · 2022-06-07T07:19:58Z

tests/test_mappingproxy.rs

@@ -0,0 +1,86 @@
+use std::collections::HashMap;


I'm curious why these tests weren't just written as unit tests too?

This question still stands.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

Signed-off-by: Sam Ezeh <sam.z.ezeh@gmail.com>

davidhewitt · 2022-07-07T07:37:37Z

Hi @dignissimus, just wondering what the status of this is and whether you're waiting for any input from me? No rush at all, just browsing our backlog of PRs.

dignissimus · 2022-07-07T21:18:35Z

Hi @dignissimus, just wondering what the status of this is and whether you're waiting for any input from me? No rush at all, just browsing our backlog of PRs.

Hey! For the elements parameter, I'm looking for input on the type? When writing it, I saw the benefit of impl IntoPyDict as the fact that it covered types of data, so they don't have to be converted. to other datatypes. But if something else works better, I'd be willing to change it.

dignissimus · 2022-07-07T21:21:37Z

Also, some of the builds fail (emscripten) for reasons outside of testing, if I remember correctly it might be to do with imports? I'm not able to fix it on my own without help. I'd need guidance to amend it or have somebody else add a commit to the PR.

davidhewitt

Cool - added a bunch of suggestions which should help you continue moving!

davidhewitt · 2022-07-07T21:48:35Z

tests/test_mappingproxy.rs

@@ -0,0 +1,86 @@
+use std::collections::HashMap;


This question still stands.

davidhewitt · 2022-07-07T21:50:08Z

tests/test_mappingproxy.rs

+use pyo3::prelude::Python;
+use pyo3::types::{IntoPyMappingProxy, PyInt, PyString};
+
+const LEN: usize = 10_000_000;


I think reducing this size may fix emscripten; I think it's simply asking for an allocation larger than emscripten can support.

davidhewitt · 2022-07-07T21:50:41Z

src/types/mappingproxy.rs

+    /// Returns a new mappingproxy that contains the same key-value pairs as self.
+    ///
+    /// This is equivalent to the Python expression `self.copy()`.
+    pub fn copy(&self) -> PyResult<&PyDict> {


Bump for this.

davidhewitt · 2022-07-07T21:52:14Z

src/types/mappingproxy.rs

+
+impl PyMappingProxy {
+    /// Creates a mappingproxy from an object.
+    pub fn new(py: Python<'_>, elements: impl IntoPyDict) -> PyResult<&'_ PyMappingProxy> {


I think we should go with &PyMapping, we can backwards-compatibly widen to &PyAny if that turns out to make sense.

davidhewitt · 2022-07-07T21:53:40Z

src/types/dict.rs

+            }
+            Err(msg) => {
+                if let Ok(mappingproxy) = <PyMappingProxy as PyTryFrom>::try_from(ob) {
+                    mappingproxy.copy()?.extract()


I would still like to avoid this copy by iterating the mappingproxy.

davidhewitt · 2022-07-07T21:55:23Z

src/types/dict.rs

+                Ok(ret)
+            }
+            Err(_) => {
+                let mappingproxy = <PyMappingProxy as PyTryFrom>::try_from(ob)?;


Same comment r.e. copy here, and that hashbrown / indexmap implementations still needed

davidhewitt · 2022-07-07T21:57:41Z

src/types/mappingproxy.rs

+    /// Creates a new mappingproxy from the sequence given.
+    ///
+    /// The sequence must consist of `(PyObject, PyObject)`.
+    ///
+    /// Returns an error on invalid input. In the case of key collisions,
+    /// this keeps the last entry seen.
+    #[cfg(not(PyPy))]
+    pub fn from_sequence(py: Python<'_>, seq: PyObject) -> PyResult<&PyMappingProxy> {
+        unsafe {
+            let dict = py.from_owned_ptr::<PyDict>(PyDict::from_sequence(py, seq)?.as_ptr());
+            py.from_owned_ptr_or_err(ffi::PyDictProxy_New(dict.as_ptr()))
+        }
+    }


I'm thinking that this isn't necessary, users will just be able to call PyMappingProxy::new(PyDict::from_sequence(...).as_mapping())

dignissimus · 2022-07-08T00:10:10Z

Cool - added a bunch of suggestions which should help you continue moving!

Thanks! I'll make the changes this Saturday

DataTriny · 2023-04-16T09:21:53Z

Hello @dignissimus

Thank you for contributing to PyO3!

The project is undergoing changes on its licensing model to better align with the rest of the Rust crates ecosystem.

Before your work gets merged, please express your consent by leaving a comment on this pull request.

Thank you!

ofek · 2023-07-26T16:24:35Z

Has there been any update on this?

davidhewitt · 2023-07-26T20:39:28Z

No progress since what is seen here, it's also not a priority feature for me so unlikely to move any time soon without further external contributions.

davidhewitt · 2023-11-26T09:51:20Z

Superseded by #3521

dignissimus added 14 commits June 2, 2022 12:09

Write mappingproxy tests

f8fcfac

Amend tests to reflect proposed API

e8e3913

Update ffi

bd5b89d

Prevent integer underflow in test

4757e6d

Add support for mappingproxy objects

d992200

Merge branch 'main' into support-mappingproxy

171fe85

Formatting

2d4b883

Remove unused imports present during testing

9783ae5

Replace calls to ffi::PyObject_CallMethodNoArgs with calls to PyAny::…

11dfceb

…call_method0

Add imports to PyPy build

7bb1f09

Remove unused import on PyPy builds

f9b8d9e

Remove unused imports from PyPy build

4b9f02a

Add required imports to PyPy build

83fe47a

Move misplaced macro

136d8fa

davidhewitt requested changes Jun 7, 2022

View reviewed changes

dignissimus and others added 8 commits June 8, 2022 16:36

Remove outdated comments from merge

69baaf8

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

Remove #[inline] from private function

00bac8e

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

Don't export PyDictProxyObject

3bfb811

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

Don't rely on PyDictProxyObject

f4b5a05

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>

Don't export add_of_mut_shim macro

7f98dd9

Signed-off-by: Sam Ezeh <sam.z.ezeh@gmail.com>

Replace instance of &mut with macro call

9bd2f44

Preserve error message when extracting dicts

c5e434d

Add tests for PyDict::is_empty and PyMappingProxy::is_empty

651a007

davidhewitt reviewed Jul 7, 2022

View reviewed changes

davidhewitt mentioned this pull request Jul 26, 2023

Native handling of MappingProxyType pydantic/pydantic#6868

Open

michaelhly mentioned this pull request Oct 16, 2023

feat: Support for mappingproxy objects #3521

Closed

davidhewitt closed this Nov 26, 2023

KLMatlock mentioned this pull request Oct 24, 2024

feat: Adds the Bound<'_, PyMappingProxy> type #4644

Merged

		let mappingproxy = <PyMappingProxy as PyTryFrom>::try_from(ob)?;
		mappingproxy.copy()?.extract()

Conversation

dignissimus commented Jun 5, 2022

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhewitt commented Jul 7, 2022

Uh oh!

dignissimus commented Jul 7, 2022

Uh oh!

dignissimus commented Jul 7, 2022

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dignissimus commented Jul 8, 2022

Uh oh!

DataTriny commented Apr 16, 2023

Uh oh!

ofek commented Jul 26, 2023

Uh oh!

davidhewitt commented Jul 26, 2023

Uh oh!

davidhewitt commented Nov 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants