-
Notifications
You must be signed in to change notification settings - Fork 729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Confusing APIs around PyObject #356
Comments
Thanks for reporting! However, I completely agree with things around |
I changed your issue title but please feel free to change it again if you dislike it. |
I agree that Afaik we could replace |
I think we can avoid much of the confusion without changing the api by explaining the types properly in the guide. |
Well, the crate is still pre-1.0, but I agree, renaming PyObject now would break everybody's code. Regarding |
With the latest 0.6.0-alpha.4, I'm having trouble with storing references to other Python instances from within a pyclass. What is the proper type to use inside structs now ? With the current situation, I'm having trouble upgrading &self or &mut self to Py<Self>. Does the new API account for this ? Here is the code that broke. |
@jothan |
Going forward, all types used to reference Python objects should probably be reduced or documented thoroughly. From memory I can name Py, PyRef, PyRefMut, PyObjectRef and PyObject. This is too much for me. Working with PyO3, I found that I spent most of my time figuring out how to convert object references. Once this is sorted out, I would like to update the docs with a matrix stating:
|
I can't agree with this more strongly, I'd love to see a re-working of the API to be more clear about this and I'd be happy to chime in. See for example this StackOverflow question, which is characteristic of my experience with PyO3. From the Rust side, I think a lot of this is clearer, but from the Python side there seem to be many, many ways to return values to Python, all of which seem to work from the Python side, but where I am not clear on the differences. |
Also, as I noted in this comment, but I guess never created an issue for, the RAII approach for It's probably worth taking a look at that in a separate issue, but essentially the lifetime of |
I agree that the api of pyo3 is overly complex and poorly documented. That being said I'd be very happy about pull request improving the api, the guide and/or the docstrings. E.g. having a matrix explaining all the different types and their usage would be great. The wasm-bindgen book should also give some inspiration. Something that's related to the wrapper types are the conversions between them. Most of pyo3's current conversions traits ( |
I'm sorry, but conceptual API design is the job of crate owners. If you allow pull requests from random strangers to decide that for you, nothing good will come out of your project. If you want my opinion, I would suggest deciding between:
Then stick to that approach. Using both at the same time is unpalatable. |
Obviously it's the crate owner's job to make decisions about the final outcome, but I think it's a good idea to encourage contributors to make their opinions known, and as specifically as possible, and to work to implement their ideas. I very frequently will make pull requests that I have no intention of getting merged, just as a proof of concept of what my proposed API would look like in situ. Personally, if I understood the current API well enough to change it, I'd be happy to make a PR to improve the API. |
@pganssle: Even if implementation is offloaded to someone else, project owners need to make design decisions first. Just sending a sweeping change PR without prior buy-in from the owners is almost certain to be waste of work. And if they just accept it... well, I personally would have trust issues with such a project. |
I agree with we need more documents, but let's enjoy refactoring so far 👍 |
I think that the design of the wrapper types in general is good. They are the minimal set of types to ergonomically represent all cases and they have no soundness holes as far as I can tell. I think the big problem is that we need more documentation and more examples. The wasm-bindgen book is an example for top-notch documentation, especially with it's supported types chapter. There's lots of functionality that people don't know about because it's not in the guide and it's not in the examples. E.g. the There's also many areas that needs to be refactored and renamed, especially the aforementioned conversion traits. But I think it would be most effective to document the existing functionality first. Trying to explain something also often helps to understand what's good or necessary and what needs to be changed.
If you have doubts whether the PR will be accepted, simply open an issue describing the changes you plan first.
In earlier versions of pyo3, a With the |
I think to replace |
What about |
@konstin |
Even though it's not the perfect name (as |
I have to agree with the original sentiment -- except that now there is yet another unexplained type, I'd be happy with a nice list of types, and when to use them, on the main PyO3 doc page, and in the guide, but I can't offer to write it (yet). |
And I'm happy to make them, if I reach the a state where I feel comfortable enough with the internals. |
Looks Github automatically closed this 😕 |
So Okay, to make this a more constructive criticism, here's the taxonomy and explanation that I would find non-confusing: "Python-managed object" - an object allocated on Python heap, whose lifetime is controlled by the built-in reference count. There are two types of Python-managed objects:
|
I'm going to close this issue; the types mentioned in the OP have more-or-less entirely been reworked at this point. Perhaps the last hurdle is that I'm sure there are things that can still be improved, but let's track that in new issues that are more up-to-date. |
I started learning PyO3 a couple of days ago, and for the longest time I was confused by the fact that
PyObject
represents a "free-floating" reference to a Python object, whereas the test of Py<thing>'s represent concrete Python types that are only accessible with'py
lifetime.It might be too late now to change all that, but... it would have been much easier to grok PyO3's API structure if
PyObject
andPyObjectRef
meanings were swapped. Furthermore, I would suggest movingPyObject
's convenience methods toPy
and makingPyObjectRef
just an alias forPy<PyObject>
.At the very least, all this should have been explained somewhere close to the top of the user guide.
The text was updated successfully, but these errors were encountered: