From 370cf44dad9663f55a2ece644fb179313423e705 Mon Sep 17 00:00:00 2001 From: Dimitris Iliopoulos Date: Wed, 22 Jan 2025 11:32:08 -0800 Subject: [PATCH] [pyo3-gil-refs] Bound<'py, T> migration 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> {} ``` * 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` 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` 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` 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 --- antlir/artifacts_dir.rs | 11 ++++++++--- antlir/fs_utils_rs.rs | 14 +++++++------- antlir/rust/BUCK | 2 +- antlir/rust/src/lib.rs | 10 +++++----- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/antlir/artifacts_dir.rs b/antlir/artifacts_dir.rs index d95e746d920..63aa2828fb1 100644 --- a/antlir/artifacts_dir.rs +++ b/antlir/artifacts_dir.rs @@ -18,7 +18,11 @@ fn ensure_path_in_repo(py: Python<'_>, path_in_repo: Option) -> 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()?) } @@ -26,14 +30,15 @@ fn ensure_path_in_repo(py: Python<'_>, path_in_repo: Option) -> PyResul } #[pymodule] -pub fn artifacts_dir(py: Python<'_>, m: &PyModule) -> PyResult<()> { - m.add("SigilNotFound", py.get_type::())?; +pub fn artifacts_dir(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { + m.add("SigilNotFound", py.get_type_bound::())?; /// 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) -> PyResult { 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) { diff --git a/antlir/fs_utils_rs.rs b/antlir/fs_utils_rs.rs index ea4b387d7fa..6dd5662cce9 100644 --- a/antlir/fs_utils_rs.rs +++ b/antlir/fs_utils_rs.rs @@ -54,9 +54,9 @@ impl IntoPy 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") @@ -67,8 +67,8 @@ impl IntoPy for AntlirPath { } } -impl<'source> FromPyObject<'source> for AntlirPath { - fn extract(p: &'source PyAny) -> PyResult { +impl<'py> FromPyObject<'py> for AntlirPath { + fn extract_bound(p: &Bound<'py, PyAny>) -> PyResult { // first attempt to get a raw bytes string, which most paths should // already be if let Ok(bytes) = p.downcast::() { @@ -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 { + fn path(p: &Bound) -> PyResult { p.extract() } diff --git a/antlir/rust/BUCK b/antlir/rust/BUCK index 4c426f68cd0..a92636cb207 100644 --- a/antlir/rust/BUCK +++ b/antlir/rust/BUCK @@ -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) -> PyResult<()> {", ] + [ 'submodule!({}, "{}", py, m)?;'.format(crate, module) for crate, module in extension_modules.items() diff --git a/antlir/rust/src/lib.rs b/antlir/rust/src/lib.rs index 089240998ab..346dcae963d 100644 --- a/antlir/rust/src/lib.rs +++ b/antlir/rust/src/lib.rs @@ -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 @@ -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>(()) @@ -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(()) }