Skip to content

Commit

Permalink
[pyo3-gil-refs] Bound<'py, T> migration
Browse files Browse the repository at this point in the history
Summary:
## Background
We need to migrate `pyo3` to version [`0.22.6`](https://github.com/PyO3/pyo3/releases/tag/v0.22.6) both to stay current since we are still on `0.21.2` but also because many `third-party/pypi` packages can not be upgraded unless we are on `pyo3 = 0.22` ([post](https://fb.workplace.com/groups/1735223876972656/permalink/2115333798961660/)). Migrating to `0.22` coincides with the deprecation of the [`gil-refs`](https://pyo3.rs/v0.23.3/migration.html#deactivating-the-gil-refs-feature) feature (introduced with `0.21`) whilst upgrading to `0.23` will completely remove the feature all together. At its core, the `gil-refs` feature entails adopting the new [`Bound<'py, T>`](https://pyo3.rs/v0.23.3/types.html#boundpy-t) which has been around since `0.21`. We can thus migrate to the new API today in preparation of the `0.22` upgrade. What follows is a list of changes for supporting the `Bound<'_, T>` API, broadly following 2 migration guides from upstream:
1. [Deactivating the `gil-refs` feature](https://pyo3.rs/v0.23.3/migration.html#deactivating-the-gil-refs-feature)
2. [From `0.21` to `0.22`](https://pyo3.rs/v0.23.3/migration.html#from-021-to-022)

In particular, all changes in this diff will fall in one of following 2 categories:

## [1] Changes needed for `pyo3=0.22`
* Replacing function and method arg types of `&Py*` with `Bound<'_, Py*>` or `&Bound<'_, Py*>`. This applies to `#[pymodule]` decorated functions as well e.g.
```rust
// BEFORE:
#[pymodule]
pub fn some(_py: Python, m: &PyModule) -> PyResult<()> {}

// AFTER:
#[pymodule]
pub fn some(_py: Python, m: &Bound<'_, PyModule>) -> PyResult<()> {}

// BEFORE:
#[pyfunction]
fn some(t: &PyType) -> PyResult<&PyType> {}

// AFTER:
#[pyfunction]
fn some(t: Bound<'_, PyType>) -> PyResult<Bound<'_, PyType>> {}
```
* Replacing `.as_ref(..)` with `.bind(..)` since we are now handling on `Bound<'_, Py*>` and not `Py*` instances.
* Replacing the `extract` function implementation for the `FromPyObject` trait with an `extract_bound` implementation ([reference](https://pyo3.rs/v0.23.3/migration.html#migrating-frompyobject-implementations)).
* Replacing `Py*::new` with `Py*::new_bound` and `Python::import` with `Python::import_bound` ([reference](https://pyo3.rs/v0.23.3/migration.html#migrating-from-the-gil-refs-api-to-boundt)) e.g.
```
// BEFORE:
PyTyple::new(..)

// AFTER:
PyTyple::new_bound(..)

// BEFORE:
py.import(..) // where py: Python<'_>

// AFTER:
py.import_bound(..)
```
* Replacing `T<&str>` with `T<PyBackedStr>` for method and function args as well as struct fields where `T` is a container type like `Vec` or `HashSet` ([reference](https://pyo3.rs/v0.23.3/migration.html#deactivating-the-gil-refs-feature)). For this change in particular there are many cases where one would be better off passing `Vec<String>` but I tried to preserve the downstream API as best I could so I used [`PyBackedStr`](https://docs.rs/pyo3/0.22.6/pyo3/pybacked/struct.PyBackedStr.html) wherever possible.

## [2] Other changes
* Replaced `pyo3::sync::GILOnceCell::get` calls that contain an implicit set with `pyo3::sync::GILOnceCell::get_or_try_init`.
* Silencing `__pyfunction_flatten_json_script::SIGNATURE` deprecation warnings for `Option<T>` args by including `#[pyo3(signature = (..)`.

## Notes
* In an ideal world it would be possible to just do these changes and then remove the `gil-refs` feature before doing the `0.22` upgrade in order to let the dust settle and fix all missed migration points. That is not feasible though since by removing `gil-refs` several `third-pypi/packages` will have to be patched or upgraded since they are not compatible with the new `Bound<'_, T> `API (see errors in D68274679).

Test Plan:
Both on this diff & and rebasing on top of [5b0763e611](https://www.internalfb.com/intern/commit/cloud/FBS/5b0763e6117e03d2d8f0d640c915d966772424d0), spot checking via `buck2 build ...` on most affected directories and running:

```
~/fbcode/common/rust/tools/scripts/check_all.sh
```

Reviewed By: manav-a, dtolnay

Differential Revision: D68165150

fbshipit-source-id: 2932668887ba1798929c9fbb9e0a10fc01ddb40f
  • Loading branch information
diliop authored and facebook-github-bot committed Jan 22, 2025
1 parent 2eb076f commit 370cf44
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 16 deletions.
11 changes: 8 additions & 3 deletions antlir/artifacts_dir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,27 @@ fn ensure_path_in_repo(py: Python<'_>, path_in_repo: Option<PathBuf>) -> PyResul
match path_in_repo {
Some(p) => Ok(p),
None => {
let argv0: String = py.import("sys")?.getattr("argv")?.get_item(0)?.extract()?;
let argv0: String = py
.import_bound("sys")?
.getattr("argv")?
.get_item(0)?
.extract()?;
let argv0 = PathBuf::from(argv0);
Ok(argv0.canonicalize()?)
}
}
}

#[pymodule]
pub fn artifacts_dir(py: Python<'_>, m: &PyModule) -> PyResult<()> {
m.add("SigilNotFound", py.get_type::<SigilNotFound>())?;
pub fn artifacts_dir(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
m.add("SigilNotFound", py.get_type_bound::<SigilNotFound>())?;

/// find_repo_root($self, path_in_repo = None)
/// --
///
/// Find the path of the VCS repository root.
#[pyfn(m)]
#[pyo3(signature = (path_in_repo=None))]
fn find_repo_root(py: Python<'_>, path_in_repo: Option<AntlirPath>) -> PyResult<AntlirPath> {
let path_in_repo = ensure_path_in_repo(py, path_in_repo.map(|p| p.into()))?;
match find_root::find_repo_root(path_in_repo) {
Expand Down
14 changes: 7 additions & 7 deletions antlir/fs_utils_rs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ impl IntoPy<PyObject> for AntlirPath {
// Python->Rust->Python, but it's a necessary indirection evil
// until/unless we replace antlir.fs_utils.Path with a Rust
// implementation
let fs_utils =
PyModule::import(py, "antlir.fs_utils").expect("antlir.fs_utils must be available");
let bytes = PyBytes::new(py, self.0.as_os_str().as_bytes());
let fs_utils = PyModule::import_bound(py, "antlir.fs_utils")
.expect("antlir.fs_utils must be available");
let bytes = PyBytes::new_bound(py, self.0.as_os_str().as_bytes());
// finally, create a fs_utils.Path from the bytes object
fs_utils
.getattr("Path")
Expand All @@ -67,8 +67,8 @@ impl IntoPy<PyObject> for AntlirPath {
}
}

impl<'source> FromPyObject<'source> for AntlirPath {
fn extract(p: &'source PyAny) -> PyResult<Self> {
impl<'py> FromPyObject<'py> for AntlirPath {
fn extract_bound(p: &Bound<'py, PyAny>) -> PyResult<Self> {
// first attempt to get a raw bytes string, which most paths should
// already be
if let Ok(bytes) = p.downcast::<PyBytes>() {
Expand All @@ -88,13 +88,13 @@ impl<'source> FromPyObject<'source> for AntlirPath {
}

#[pymodule]
pub fn fs_utils_rs(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
pub fn fs_utils_rs(_py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
/// Largely just useful for tests from Python, this will take the given
/// input and attempt to round-trip it through [Path] and back into an
/// `antlir.fs_utils.Path`
#[pyfn(m)]
#[pyo3(name = "Path")]
fn path(p: &PyAny) -> PyResult<AntlirPath> {
fn path(p: &Bound<PyAny>) -> PyResult<AntlirPath> {
p.extract()
}

Expand Down
2 changes: 1 addition & 1 deletion antlir/rust/BUCK
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ write_file(
out = "register_modules.rs",
content = [
"use pyo3::prelude::*;",
"pub(crate) fn register_modules(py: Python<'_>, m: &PyModule) -> PyResult<()> {",
"pub(crate) fn register_modules(py: Python<'_>, m: &Bound<PyModule>) -> PyResult<()> {",
] + [
'submodule!({}, "{}", py, m)?;'.format(crate, module)
for crate, module in extension_modules.items()
Expand Down
10 changes: 5 additions & 5 deletions antlir/rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ use pyo3::prelude::*;
/// (https://pyo3.rs/v0.16.4/module.html)
macro_rules! submodule {
($module:ident, $full_module_name:literal, $py:ident, $parent_module:ident) => {{
let child_module = PyModule::new($py, stringify!($module))?;
$module::$module($py, child_module)?;
$parent_module.add_submodule(child_module)?;
let child_module = PyModule::new_bound($py, stringify!($module))?;
$module::$module($py, &child_module)?;
$parent_module.add_submodule(&child_module)?;

// Small hack to enable intuitive Python import resolution. All
// submodules technically come from loading `antlir.rust`, but we want
Expand All @@ -26,7 +26,7 @@ macro_rules! submodule {
// there might be in pure-Python module imports, since all the
// initialization is done as part of importing the top level
// `antlir.rust` module in the first place.
$py.import("sys")?
$py.import_bound("sys")?
.getattr("modules")?
.set_item($full_module_name, child_module)?;
Ok::<_, pyo3::PyErr>(())
Expand All @@ -37,7 +37,7 @@ mod register_modules;

#[pymodule]
#[pyo3(name = "native_antlir_impl")]
fn native(py: Python<'_>, m: &PyModule) -> PyResult<()> {
fn native(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
register_modules::register_modules(py, m)?;
Ok(())
}

0 comments on commit 370cf44

Please sign in to comment.