Remove PyObject, rename PyAny -> PyObject #863

davidhewitt · 2020-04-10T09:24:59Z

Fixes #356

This is an experiment to solve the well-known confusion around PyObject and PyAny by removing PyObject and then renaming PyAny to PyObject, roughly as suggested in #679

All the tests pass except for doc tests. I think there's a lot of documentation that would need fixing up.

I wanted to open this PR as an experiment; there will be plenty of things we can improve in it and I'm prepared to keep rebasing this and fixing it up against master as we go.

Most notable changes that I think might want discussion:

I had to remove the implementation FromPy<T> for T to get conversions Py<T> -> Py<PyObject> to compile. We might want to rethink this. Maybe min_specialization feature would allow these conflicting impls.
ToPyObject::to_object() trait now returns &PyObject. Not sure if maybe Py<PyObject> is better.

~~Bikeshed:~~

I've been wondering what happens if we renamed PyObject to just Object - I don't think the Py is particularly interesting and typing Py<Object> is heck of a lot less annoying thanPy<PyObject>. This would be a good moment to do such a rename if we're breaking everything anyway. Moved bikeshed to #873

TODO:

birkenfeld · 2020-04-10T10:26:11Z

In general, I'm delighted with this.

I've been wondering what happens if we renamed PyObject to just Object - I don't
think the Py is particularly interesting and typing Py<Object> is heck of a lot
less annoying thanPy<PyObject>. This would be a good moment to do such a rename
if we're breaking everything anyway.

Although Py<Object> looks good, I guess that would make all the concrete types candidates for renaming as well. PyList -> List etc? In the reference form, this now loses the information that they're Python objects.

While we are bold, let me throw renaming Py in the ring: it's easily confused with the Python type, instances of which are usually called py. Also the name doesn't really give a hint as to the smart pointer's purpose.

Unfortunately I don't have a good name in mind, let's call it Own here (which is bad, I know). So we'd have Own<PyObject>, Own<PyList> and Own<MyStruct> for Rust-based types...

kngwyu · 2020-04-10T10:53:00Z

Thank you for the hard work!

While we are bold, let me throw renaming Py in the ring: it's easily confused with the Python type, instances of which are usually called py. Also the name doesn't really give a hint as to the smart pointer's purpose.

It sounds better to rename Py if we can find a good name, though I don't have a good name too.

davidhewitt · 2020-04-10T10:53:06Z

I'm open to renaming Py as an alternative.

Given that Py is kinda like Arc, Rc, one name could be Prc aka "Python Reference Counted". (I'm reading that out loud as "Parc")

Prc does look weird but so did Arc and Rc when I started learning Rust.

Another named for Py could perhaps be Unscoped (because it's detaching GIL-scoped references from the GIL lifetime), but that's much longer...

davidhewitt · 2020-04-10T10:55:00Z

Also, I'm kinda hoping in the long run that we don't use Py much because most things will work with GIL-scoped references. So Py only comes into use when things need to escape the GIL lifetime.

kngwyu · 2020-04-10T10:56:06Z

Added help-wanted label since I want to hear more opinions about this naming issue.

althonos · 2020-04-10T12:11:57Z

I've been wondering what happens if we renamed PyObject to just Object - I don't think the Py is particularly interesting and typing Py<Object>.

I'm kinda for keeping PyObject against Object for consistency with all the other types (PyDict, PyList, etc.), even with the Py<PyObject> situation. An interesting option is what rlua does; types are not Lua prefixed in the API, but they are in rlua::prelude to avoid confusion.

Given that Py is kinda like Arc, Rc, one name could be Prc aka "Python Reference Counted". (I'm reading that out loud as "Parc")

Sounds good to make that more explicit.

programmerjake · 2020-04-10T18:44:56Z

how about PyRc instead of Py? it's short and seems totally obvious.

davidhewitt · 2020-04-11T10:18:29Z

An interesting option is what rlua does; types are not Lua prefixed in the API, but they are in rlua::prelude to avoid confusion.

I find this option quite interesting too. In code where I'm interacting with Python a lot I feel that typing Py (in any form) a lot isn't particularly helpful, as I already know the code is in a Python context.

I think there's a lot of ideas here around naming of PyObject vs Object, Py vs PyRc etc. To keep this PR more focussed specifically on simplifying the current PyAny / PyObject situation, perhaps we can move the more general discussion into its own issue?

davidhewitt · 2020-04-17T20:57:51Z

I did some cleanups and rebased. CI might even be green this time! I moved the question of names to #873.

davidhewitt · 2020-04-17T21:26:57Z

ToPyObject::to_object() trait now returns &PyObject. Not sure if maybe Py is better.

I have been thinking further about this and I think it is the correct decision. Especially in the future when we have PyObject<'a>, I think this offers the best ergonomics. (Also I think we want most code to use &PyObject / PyObject<'a> and only to use the Py<PyObject> form when necessary.)

davidhewitt · 2020-04-26T19:03:21Z

src/instance.rs

+    // /// Extracts some type from the Python object.
+    // ///
+    // /// This is a wrapper function around `FromPyObject::extract()`.
+    // pub fn extract<'p, D>(&'p self, py: Python<'p>) -> PyResult<D>
+    // where
+    //     D: FromPyObject<'p>,
+    // {
+    //     FromPyObject::extract(self.as_ref(py))
+    // }
+
+    // /// Retrieves an attribute value.
+    // ///
+    // /// This is equivalent to the Python expression `self.attr_name`.
+    // pub fn getattr<N>(&self, py: Python, attr_name: N) -> PyResult<PyObject>
+    // where
+    //     N: ToPyObject,
+    // {
+    //     attr_name.with_borrowed_ptr(py, |attr_name| unsafe {
+    //         PyObject::from_owned_ptr_or_err(py, ffi::PyObject_GetAttr(self.as_ptr(), attr_name))
+    //     })
+    // }
+
+    // /// Calls the object.
+    // ///
+    // /// This is equivalent to the Python expression `self(*args, **kwargs)`.
+    // pub fn call(
+    //     &self,
+    //     py: Python,
+    //     args: impl IntoPy<Py<PyTuple>>,
+    //     kwargs: Option<&PyDict>,
+    // ) -> PyResult<PyObject> {
+    //     let args = args.into_py(py).into_ptr();
+    //     let kwargs = kwargs.into_ptr();
+    //     let result = unsafe {
+    //         PyObject::from_owned_ptr_or_err(py, ffi::PyObject_Call(self.as_ptr(), args, kwargs))
+    //     };
+    //     unsafe {
+    //         ffi::Py_XDECREF(args);
+    //         ffi::Py_XDECREF(kwargs);
+    //     }
+    //     result
+    // }
+
+    // /// Calls the object with only positional arguments.
+    // ///
+    // /// This is equivalent to the Python expression `self(*args)`.
+    // pub fn call1(&self, py: Python, args: impl IntoPy<Py<PyTuple>>) -> PyResult<PyObject> {
+    //     self.call(py, args, None)
+    // }
+
+    // /// Calls the object without arguments.
+    // ///
+    // /// This is equivalent to the Python expression `self()`.
+    // pub fn call0(&self, py: Python) -> PyResult<PyObject> {
+    //     self.call(py, (), None)
+    // }
+
+    // /// Calls a method on the object.
+    // ///
+    // /// This is equivalent to the Python expression `self.name(*args, **kwargs)`.
+    // pub fn call_method(
+    //     &self,
+    //     py: Python,
+    //     name: &str,
+    //     args: impl IntoPy<Py<PyTuple>>,
+    //     kwargs: Option<&PyDict>,
+    // ) -> PyResult<PyObject> {
+    //     name.with_borrowed_ptr(py, |name| unsafe {
+    //         let args = args.into_py(py).into_ptr();
+    //         let kwargs = kwargs.into_ptr();
+    //         let ptr = ffi::PyObject_GetAttr(self.as_ptr(), name);
+    //         if ptr.is_null() {
+    //             return Err(PyErr::fetch(py));
+    //         }
+    //         let result = PyObject::from_owned_ptr_or_err(py, ffi::PyObject_Call(ptr, args, kwargs));
+    //         ffi::Py_DECREF(ptr);
+    //         ffi::Py_XDECREF(args);
+    //         ffi::Py_XDECREF(kwargs);
+    //         result
+    //     })
+    // }
+
+    // /// Calls a method on the object with only positional arguments.
+    // ///
+    // /// This is equivalent to the Python expression `self.name(*args)`.
+    // pub fn call_method1(
+    //     &self,
+    //     py: Python,
+    //     name: &str,
+    //     args: impl IntoPy<Py<PyTuple>>,
+    // ) -> PyResult<PyObject> {
+    //     self.call_method(py, name, args, None)
+    // }
+
+    // /// Calls a method on the object with no arguments.
+    // ///
+    // /// This is equivalent to the Python expression `self.name()`.
+    // pub fn call_method0(&self, py: Python, name: &str) -> PyResult<PyObject> {
+    //     self.call_method(py, name, (), None)
+    // }


These methods used to exist on PyObject. I'm trying to decide if we should move them to Py<T>, or delete them.

davidhewitt · 2020-04-26T19:05:40Z

src/instance.rs

 /// };
 /// let gil = Python::acquire_gil();
 /// let py = gil.python();
-/// assert_eq!(obj.as_ref(py).len().unwrap(), 0);  // PyAny implements ObjectProtocol
+/// assert_eq!(obj.as_ref(py).len().unwrap(), 0);  // PyObject implements ObjectProtocol
 /// ```
 pub trait AsPyRef: Sized {


Now that the old PyObject doesn't exist, I'm wondering if AsPyRef trait should be moved to be just a method on Py<T>. I don't think there is value in it being a trait any more.

davidhewitt · 2020-04-26T21:18:38Z

@kngwyu this is now looking good to me; would be curious to hear what you have to say about it! 😄

birkenfeld · 2020-04-27T07:22:24Z

I tried this with my (smallish) extension module, and there was no unexpected fallout.

I changed existing PyObject to &PyObject (in function args) and Py<PyObject> (elsewhere, e.g. in trait impls of IntoPy). Then all PyAny to PyObject.

Finally, I just had remove a bunch of py arguments to methods that now didn't need them anymore.

👍

However, this needs a good step-by-step guide in the release notes, since the errors I got trying to compile the original code were mostly not particularly helpful.

davidhewitt · 2020-04-27T08:23:04Z

I changed existing PyObject to &PyObject (in function args) and Py (elsewhere, e.g. in trait impls of IntoPy). Then all PyAny to PyObject.

Where you changed PyObject to &PyObject did this increase the number of lifetime annotations you had to add to these functions? That is possibly my main worry of ergonomic downsides of this change.

However, this needs a good step-by-step guide in the release notes, since the errors I got trying to compile the original code were mostly not particularly helpful.

I agree. It would be interesting to see if I could write rerast rules to automate migration. @birkenfeld is your extension module open source? I'd be interested to try writing rerast rules for it.

I also wondered about adding to migration.md in the guide.

birkenfeld · 2020-04-27T08:43:54Z

@davidhewitt no, it's not unfortunately. I have a few other projects waiting for PyO3 bindings but can only start them once it compiles on stable...

kngwyu · 2020-04-27T09:28:10Z

@birkenfeld

However, this needs a good step-by-step guide in the release notes, since the errors I got trying to compile the original code were mostly not particularly helpful.

Yeah, thank you for trying it out.

@davidhewitt
I'm sorry but now I don't have enough time to review all changes. Please wait for 3~5 days.

But my main concern is... should we do this now?

This change would force users to replace many PyObject with Py<PyObject> since many of our APIs are built around PyObject(e.g., IntoPy<PyObject> is used everywhere).`

However, since PyObject(or Py<PyObject>) would be costlier than PyAny after #679 is solved, we should rewrite lots of PyObject APIs to use PyAny.
So if we do renaming now, users would have to

Replace PyObject with Py<PyObject>
Then (if we refactor Py<PyObject> APIs) replace Py<PyObject> with (new) PyObject...

Isn't it confusing?

Indeed, we cannot avoid this curse of renaming and at last, should replace PyAny with PyObject.
But doing it now(= before refactoring the internal system of PyAny and conversion traits) doesn't seem so good idea to me.

davidhewitt · 2020-04-27T10:03:20Z

@kngwyu that's a good question. I actually had started on this change by working on #679 in totality (both PyAny ownership change and PyAny rename).

It seemed too much to do in one go so I did this bit first.

I'd be happy to instead try out the PyAny ownership change first, if you prefer.

Give me a few days to make a draft PR for that. Don't worry about reviewing this one in the meanwhile 👍

kngwyu · 2020-04-27T13:17:41Z

I'd be happy to instead try out the PyAny ownership change first, if you prefer.

Firstly, how about rethinking the usage of IntoPy<PyObject> or ToPyObject?
These two traits would be major burdens when replacing PyObject with Py<PyObject>.
But I don't any strong opinions about this. It's just a proposal and please feel free to start from other points.

davidhewitt · 2020-04-27T14:14:55Z

Firstly, how about rethinking the usage of IntoPy or ToPyObject?

👍 can have a look there. Though I think we might want to get to the "lightweight" PyAny<'a> before we try to redesign these traits much.

For example I foresee already that IntoPy<PyObject<'a>> is also slightly frustrating to type!

But maybe that particular one cannot be avoided if we are using GIL lifetimes...

davidhewitt · 2020-05-08T16:01:00Z

I've rebased this PR on master, and converted back to draft.

I think it's still interesting to continue to explore this branch, and I will continue to rebase it as we go.

You're right that we can probably improve the design of IntoPy and ToPyObject before we merge this. Also I would advise that we don't merge this in 0.10.0, as that version is getting exciting enough already. Maybe save it for 0.11.0. 😄

birkenfeld · 2020-05-08T16:15:21Z

On the other hand, large incompatible changes with every version are also tiring...

davidhewitt · 2020-05-08T16:32:48Z

That's fair - maybe we'll save it for 0.12.0. 😆

In all serious I'm happy to get this into 0.10.0 if we think we are in favour of moving ahead with this change. I think there are still open questions about what the conversion traits should look like (IntoPy and ToPyObject) - especially as we chose to merge #887 instead of #885.

kngwyu · 2020-05-09T04:30:19Z

I know PyAny is criticized since it's confusing (see #356 and #388), but personally I'm too accustomed to the name. I need more feedbacks from users.

birkenfeld · 2020-05-09T06:29:22Z

Personally, I'm fine with either name, as long as there's only one of them remaining.

(PyAny has the very slight advantage of not clashing with the ffi PyObject, but most users won't ever need the latter.)

davidhewitt · 2020-07-20T08:35:23Z

This draft PR is now massively out-of-date. Also, I think that there are enough breaking changes with this PR that it probably doesn't make sense to push forward as-is with it until we've got a final design for the rest of the conversion traits.

As a result, I'm going to close this. I do still think that the rough objective of this PR is still relevant - it'll shrink PyO3's range of names and types, which is a common criticism of the PyO3 API.

I've got a lot of notes on what I learned from trying this, which I'll write up as an issue later so that we can pick up this and continue to design API improvements.

kngwyu added the help-wanted label Apr 10, 2020

davidhewitt force-pushed the rename-pyobject branch from b9f741a to 6fed82f Compare April 11, 2020 08:33

davidhewitt force-pushed the rename-pyobject branch from 6fed82f to 86185a2 Compare April 17, 2020 20:22

davidhewitt mentioned this pull request Apr 17, 2020

Bikeshedding names: Py and PyObject #873

Open

davidhewitt force-pushed the rename-pyobject branch from 391f156 to 060f094 Compare April 17, 2020 20:59

davidhewitt force-pushed the rename-pyobject branch 2 times, most recently from b796776 to a566b8d Compare April 26, 2020 18:48

davidhewitt commented Apr 26, 2020

View reviewed changes

davidhewitt force-pushed the rename-pyobject branch 2 times, most recently from 4e15824 to 841408b Compare April 26, 2020 19:58

davidhewitt marked this pull request as ready for review April 26, 2020 21:16

davidhewitt removed the help-wanted label Apr 26, 2020

davidhewitt requested a review from kngwyu April 26, 2020 21:16

davidhewitt changed the title ~~[WIP] Remove PyObject, rename PyAny -> PyObject~~ Remove PyObject, rename PyAny -> PyObject Apr 26, 2020

davidhewitt force-pushed the rename-pyobject branch from 841408b to bdf2bb6 Compare May 8, 2020 15:58

davidhewitt marked this pull request as draft May 8, 2020 15:58

davidhewitt force-pushed the rename-pyobject branch from bdf2bb6 to c802a17 Compare May 8, 2020 16:05

davidhewitt added 4 commits May 20, 2020 20:25

Remove PyObject

f90743b

Rename PyAny -> PyObject

ac547fc

Improve guide for new PyObject

fca8ef8

Tidy up examples and PR code

8e90d48

davidhewitt force-pushed the rename-pyobject branch from c802a17 to 8e90d48 Compare May 20, 2020 19:30

davidhewitt closed this Jul 20, 2020

davidhewitt deleted the rename-pyobject branch December 24, 2021 02:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove PyObject, rename PyAny -> PyObject #863

Remove PyObject, rename PyAny -> PyObject #863

davidhewitt commented Apr 10, 2020 •

edited

Loading

birkenfeld commented Apr 10, 2020 •

edited

Loading

kngwyu commented Apr 10, 2020 •

edited

Loading

davidhewitt commented Apr 10, 2020

davidhewitt commented Apr 10, 2020

kngwyu commented Apr 10, 2020

althonos commented Apr 10, 2020 •

edited

Loading

programmerjake commented Apr 10, 2020

davidhewitt commented Apr 11, 2020 •

edited

Loading

davidhewitt commented Apr 17, 2020

davidhewitt commented Apr 17, 2020

davidhewitt Apr 26, 2020

davidhewitt Apr 26, 2020

davidhewitt commented Apr 26, 2020

birkenfeld commented Apr 27, 2020

davidhewitt commented Apr 27, 2020 •

edited

Loading

birkenfeld commented Apr 27, 2020

kngwyu commented Apr 27, 2020 •

edited

Loading

davidhewitt commented Apr 27, 2020

kngwyu commented Apr 27, 2020

davidhewitt commented Apr 27, 2020 •

edited

Loading

davidhewitt commented May 8, 2020

birkenfeld commented May 8, 2020

davidhewitt commented May 8, 2020

kngwyu commented May 9, 2020

birkenfeld commented May 9, 2020

davidhewitt commented Jul 20, 2020

Remove PyObject, rename PyAny -> PyObject #863

Remove PyObject, rename PyAny -> PyObject #863

Conversation

davidhewitt commented Apr 10, 2020 • edited Loading

birkenfeld commented Apr 10, 2020 • edited Loading

kngwyu commented Apr 10, 2020 • edited Loading

davidhewitt commented Apr 10, 2020

davidhewitt commented Apr 10, 2020

kngwyu commented Apr 10, 2020

althonos commented Apr 10, 2020 • edited Loading

programmerjake commented Apr 10, 2020

davidhewitt commented Apr 11, 2020 • edited Loading

davidhewitt commented Apr 17, 2020

davidhewitt commented Apr 17, 2020

davidhewitt Apr 26, 2020

Choose a reason for hiding this comment

davidhewitt Apr 26, 2020

Choose a reason for hiding this comment

davidhewitt commented Apr 26, 2020

birkenfeld commented Apr 27, 2020

davidhewitt commented Apr 27, 2020 • edited Loading

birkenfeld commented Apr 27, 2020

kngwyu commented Apr 27, 2020 • edited Loading

davidhewitt commented Apr 27, 2020

kngwyu commented Apr 27, 2020

davidhewitt commented Apr 27, 2020 • edited Loading

davidhewitt commented May 8, 2020

birkenfeld commented May 8, 2020

davidhewitt commented May 8, 2020

kngwyu commented May 9, 2020

birkenfeld commented May 9, 2020

davidhewitt commented Jul 20, 2020

davidhewitt commented Apr 10, 2020 •

edited

Loading

birkenfeld commented Apr 10, 2020 •

edited

Loading

kngwyu commented Apr 10, 2020 •

edited

Loading

althonos commented Apr 10, 2020 •

edited

Loading

davidhewitt commented Apr 11, 2020 •

edited

Loading

davidhewitt commented Apr 27, 2020 •

edited

Loading

kngwyu commented Apr 27, 2020 •

edited

Loading

davidhewitt commented Apr 27, 2020 •

edited

Loading