-
Notifications
You must be signed in to change notification settings - Fork 804
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
deprecate PyCell::new
in favor of Py::new
or Bound::new
#3872
Conversation
CodSpeed Performance ReportMerging #3872 will degrade performances by 15.02%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! What did you think of moving from PyCell
to Py
/ Bound
.
I'm a little torn. I like the fact that it simplifies PyO3's API and makes Py
/ Bound
the only way to interact with all Python objects, regardless of whether they are native or #[pyclass]
types.
On the other hand, the "PyCell" concept is easy for users to see the link to RefCell
, and I think we will lose this a bit by removing this type. To be honest, that might not matter so much if we also go further and make #[pyclass(frozen)]
the default, because then users will see this concept less anyway. Then we can have a section of the guide dedicated to #[pyclass(frozen = false)]
to explain it in detail, even if it's hidden away a bit.
I guess even further down the road, with nogil
I half expect that PyCell
borrow tracking doesn't work so well because it'll be much easier to hit conflicts with concurrency.
... so maybe in the end I convinced myself it's not such a bad thing if we start slowly killing off the "PyCell" concept now 😂
Other than that, just two small comments.
@@ -217,8 +217,8 @@ impl Number { | |||
|
|||
# fn main() -> PyResult<()> { | |||
# Python::with_gil(|py| { | |||
# let x = PyCell::new(py, Number(4))?; | |||
# let y = PyCell::new(py, Number(4))?; | |||
# let x = &Bound::new(py, Number(4))?.into_any(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These .as_any()
and .into_any()
calls are probably a consequence of the same thing as #3867 (comment)
I suggest we merge here anyway and then can tidy up a bunch of .as_any()
and .into_any()
calls after figuring out a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've just added some thoughts to that thread.
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
I think it makes sense to try to push to make The functionality will still be there with |
Part of #3684
This deprecates
PyCell::new
in favor ofPy::new
orBound::new
, depending on whether the GIL attachment is required.For the tests I used
Py
if that was enough, andBound
otherwise. In the docs I updated the test-only code and examples unrelated toPyCell
. ThePyCell
specific sections need separate updating, so I#[allow(deprecated)]
usage there, same forPyCell
specific tests.