Skip to content
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

GIL Refs API Removal - Tracking Issue #3960

Closed
42 of 51 tasks
davidhewitt opened this issue Mar 15, 2024 · 9 comments
Closed
42 of 51 tasks

GIL Refs API Removal - Tracking Issue #3960

davidhewitt opened this issue Mar 15, 2024 · 9 comments
Milestone

Comments

@davidhewitt
Copy link
Member

davidhewitt commented Mar 15, 2024

The main steps remaining to remove GIL Refs:

For 0.22

In 0.22 the gil-refs feature will continue to be available, but with two major differences from 0.21. The deprecation warnings will be unconditional and the APIs will be gated behind the feature.

For 0.23

  • Mass deletion of deprecated APIs🚀
  • For _bound variants like PyTuple::new_bound:
    • Add plain variants like PyTuple::new again.
    • Add deprecation markers on PyTuple::new_bound to nudge users to the plain versions again.

Optional polishing tasks from #3684

  • Split PyModuleMethods to move functionality related to creating / building new modules into a PyModuleBuilder? implement PyModuleMethods #3703 (comment)
  • Generic IntoPy<Py<T>> for Bound<'_, T>? implement PyTupleMethods #3681 (comment)
  • Should Borrowed<'py, 'py, T>::into_gil_ref become public API as well?
  • FromPyObject could gain a lifetime 'a to its argument &'a Bound<'py>, allowing:
    • downcasting the Bound to allow impl FromPyObject for &Bound
    • impl FromPyObject<'a, '_> for &'a str without the pool; as the lifetime of the string borrow depends on the lifetime of the input, not the GIL lifetime
    • would allow PyRef and PyRefMut to store &'a Bound<'py, T> (or borrowed) and avoid reference count overhead
  • Update py.None(), py.Ellipsis() and py.NotImplemented() to return Borrowed<'py, 'py, PyNone> (etc.)
  • It would be nice to accept #[pyo3(from_py_with = "PyAnyMethods::len")] (and similar)
@alex
Copy link
Contributor

alex commented May 3, 2024

Question: What do we need to do with PyClass, which currently has the following definition: pub trait PyClass: PyTypeInfo<AsRefTarget = crate::PyCell<Self>> + PyClassImpl { ... }.

Except PyCell is deprecated!

@davidhewitt
Copy link
Member Author

I think AsRefTarget can also be conditional as that's a GIL Ref concept which needs to be removed.

It might mean it's necessary to have an extra level of indirection e.g. pub trait PyClass: PyClassConstraints where PyClassConstraints is conditional.

@alex
Copy link
Contributor

alex commented May 4, 2024

Pain point here: https://github.com/PyO3/pyo3/blob/main/pyo3-macros-backend/src/pyclass.rs#L1281-L1285

This unconditionally generates emits this, and currently gil-refs is not wired into pyo3-macros-backend.

@Icxolu
Copy link
Contributor

Icxolu commented May 4, 2024

Another pain point is as_gil_ref and especially into_gil_ref which are used a lot in the gil-ref inherent methods now. Maybe we need to split all these impl blocks and feature gate all these inherent methods, except for the new _bound constructors.

@alex
Copy link
Contributor

alex commented May 27, 2024

There's an item for 0.22, " Update documentation on Python type". A quick review of the struct-level docs didn't indicate anything obviously wrong. Anyone recall what that's a reference to?

@davidhewitt
Copy link
Member Author

I think what I wanted is just this: #4274

@ngoldbaum
Copy link
Contributor

Presumably all the documentation about PyCell in https://pyo3.rs/main/doc/pyo3/pycell/ needs to be deleted?

@mejrs
Copy link
Member

mejrs commented Sep 9, 2024

Presumably all the documentation about PyCell in https://pyo3.rs/main/doc/pyo3/pycell/ needs to be deleted?

Right, I remember this came up in discord. I think it's important that this prose exists somewhere in the api docs, and that the current place is the right place - IMO it's too specific to go on top of Py or Bound. We should definitely remove the references to PyCell itself though, and probably rename the module.

@davidhewitt davidhewitt mentioned this issue Sep 13, 2024
3 tasks
@davidhewitt
Copy link
Member Author

As 0.23 is nearly ready and I believe we have cleaned up everything (thanks @Icxolu!), I think this tracking issue doesn't have much value left. Let's close, and if there are small cleanups remaining, they can be done ad-hoc. Well done everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants