Skip to content

Commit

Permalink
PyO3: upgrade to v0.23.x (#21657)
Browse files Browse the repository at this point in the history
[Upgrade to v0.23.x of the `pyo3`
crate](#21671):

- Functions in PyO3 with `_bound` suffixes existed in PyO3 only for
easing migration to the `Bound` API. They are deprecated now and new
methods without the `_bound` suffixes have been re-introduced in PyO3.
This PR renames call sites accordingly and updates code to also reflect
that some of the new APIs (e.g., `PyTuple::new`) are now fallible.

- The `IntoPy` and `ToPyObject` traits are deprecated in favor of the
new `IntoPyObject` trait (which is fallible). To ease migration, this PR
only disables deprecation warnings as errors in the affected files.
(Unfortunately, `IntoPyObject` was not introduced in the current v0.22.x
version and so we cannot migrate ahead of the upgrade (unlike what was
done in #21670 for the new
`pyclass` Sync requirement).) Migration will take place in follow-on
PRs.
 
- This PR does add an implementation of `IntoPyObject` for `&Value` to
support some existing call sites.
  • Loading branch information
tdyas authored Nov 21, 2024
1 parent 924c177 commit 58aea21
Show file tree
Hide file tree
Showing 20 changed files with 163 additions and 112 deletions.
22 changes: 11 additions & 11 deletions src/rust/engine/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/rust/engine/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,8 @@ prodash = { git = "https://github.com/stuhood/prodash", rev = "stuhood/raw-messa
prost = "0.13"
prost-build = "0.13"
prost-types = "0.13"
pyo3 = "0.22"
pyo3-build-config = "0.22"
pyo3 = "0.23.1"
pyo3-build-config = "0.23.1"
rand = "0.8"
regex = "1"
rlimit = "0.8"
Expand Down
23 changes: 10 additions & 13 deletions src/rust/engine/src/externs/address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,43 +29,40 @@ pub fn register(py: Python, m: &Bound<'_, PyModule>) -> PyResult<()> {

m.add(
"AddressParseException",
py.get_type_bound::<AddressParseException>(),
)?;
m.add(
"InvalidAddressError",
py.get_type_bound::<InvalidAddressError>(),
py.get_type::<AddressParseException>(),
)?;
m.add("InvalidAddressError", py.get_type::<InvalidAddressError>())?;
m.add(
"InvalidSpecPathError",
py.get_type_bound::<InvalidSpecPathError>(),
py.get_type::<InvalidSpecPathError>(),
)?;
m.add(
"InvalidTargetNameError",
py.get_type_bound::<InvalidTargetNameError>(),
py.get_type::<InvalidTargetNameError>(),
)?;
m.add(
"InvalidParametersError",
py.get_type_bound::<InvalidParametersError>(),
py.get_type::<InvalidParametersError>(),
)?;
m.add(
"UnsupportedWildcardError",
py.get_type_bound::<UnsupportedWildcardError>(),
py.get_type::<UnsupportedWildcardError>(),
)?;

m.add_class::<AddressInput>()?;
m.add_class::<Address>()?;

m.add(
"BANNED_CHARS_IN_TARGET_NAME",
PyFrozenSet::new_bound(py, BANNED_CHARS_IN_TARGET_NAME.iter())?,
PyFrozenSet::new(py, BANNED_CHARS_IN_TARGET_NAME.iter())?,
)?;
m.add(
"BANNED_CHARS_IN_GENERATED_NAME",
PyFrozenSet::new_bound(py, BANNED_CHARS_IN_GENERATED_NAME.iter())?,
PyFrozenSet::new(py, BANNED_CHARS_IN_GENERATED_NAME.iter())?,
)?;
m.add(
"BANNED_CHARS_IN_PARAMETERS",
PyFrozenSet::new_bound(py, BANNED_CHARS_IN_PARAMETERS.iter())?,
PyFrozenSet::new(py, BANNED_CHARS_IN_PARAMETERS.iter())?,
)?;

Ok(())
Expand Down Expand Up @@ -786,7 +783,7 @@ impl Address {
}

fn metadata<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyDict>> {
let dict = PyDict::new_bound(py);
let dict = PyDict::new(py);
dict.set_item(pyo3::intern!(py, "address"), self.spec())?;
Ok(dict)
}
Expand Down
41 changes: 24 additions & 17 deletions src/rust/engine/src/externs/fs.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Copyright 2020 Pants project contributors (see CONTRIBUTORS.md).
// Licensed under the Apache License, Version 2.0 (see LICENSE).

// Temporary: Allow deprecated items while we migrate to PyO3 v0.23.x.
#![allow(deprecated)]

use std::collections::hash_map::DefaultHasher;
use std::fmt;
use std::hash::{Hash, Hasher};
Expand Down Expand Up @@ -213,50 +216,54 @@ impl PySnapshot {
}

#[getter]
fn files<'py>(&self, py: Python<'py>) -> Bound<'py, PyTuple> {
fn files<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyTuple>> {
let files = self.0.files();
PyTuple::new_bound(
PyTuple::new(
py,
files
.into_iter()
.map(|path| PyString::new_bound(py, &path.to_string_lossy()))
.map(|path| PyString::new(py, &path.to_string_lossy()))
.collect::<Vec<_>>(),
)
}

#[getter]
fn dirs<'py>(&self, py: Python<'py>) -> Bound<'py, PyTuple> {
fn dirs<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyTuple>> {
let dirs = self.0.directories();
PyTuple::new_bound(
PyTuple::new(
py,
dirs.into_iter()
.map(|path| PyString::new_bound(py, &path.to_string_lossy()))
.map(|path| PyString::new(py, &path.to_string_lossy()))
.collect::<Vec<_>>(),
)
}

// NB: Prefix with underscore. The Python call will be hidden behind a helper which returns a much
// richer type.
fn _diff<'py>(&self, other: &Bound<'py, PySnapshot>, py: Python<'py>) -> Bound<'py, PyTuple> {
fn _diff<'py>(
&self,
other: &Bound<'py, PySnapshot>,
py: Python<'py>,
) -> PyResult<Bound<'py, PyTuple>> {
let result = self.0.tree.diff(&other.borrow().0.tree);

let into_tuple = |x: &Vec<PathBuf>| -> Bound<'py, PyTuple> {
PyTuple::new_bound(
let into_tuple = |x: &Vec<PathBuf>| -> PyResult<Bound<'py, PyTuple>> {
PyTuple::new(
py,
x.iter()
.map(|path| PyString::new_bound(py, &path.to_string_lossy()))
.map(|path| PyString::new(py, &path.to_string_lossy()))
.collect::<Vec<_>>(),
)
};

PyTuple::new_bound(
PyTuple::new(
py,
vec![
into_tuple(&result.our_unique_files),
into_tuple(&result.our_unique_dirs),
into_tuple(&result.their_unique_files),
into_tuple(&result.their_unique_dirs),
into_tuple(&result.changed_files),
into_tuple(&result.our_unique_files)?,
into_tuple(&result.our_unique_dirs)?,
into_tuple(&result.their_unique_files)?,
into_tuple(&result.their_unique_dirs)?,
into_tuple(&result.changed_files)?,
],
)
}
Expand All @@ -270,7 +277,7 @@ pub struct PyMergeDigests(pub Vec<DirectoryDigest>);
impl PyMergeDigests {
#[new]
fn __new__(digests: &Bound<'_, PyAny>, _py: Python) -> PyResult<Self> {
let digests: PyResult<Vec<DirectoryDigest>> = PyIterator::from_bound_object(digests)?
let digests: PyResult<Vec<DirectoryDigest>> = PyIterator::from_object(digests)?
.map(|v| {
let py_digest = v?.extract::<PyDigest>()?;
Ok(py_digest.0)
Expand Down
53 changes: 31 additions & 22 deletions src/rust/engine/src/externs/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

// File-specific allowances to silence internal warnings of `[pyclass]`.
#![allow(clippy::used_underscore_binding)]
// Temporary: Allow deprecated items while we migrate to PyO3 v0.23.x.
#![allow(deprecated)]

/// This crate is a wrapper around the engine crate which exposes a Python module via PyO3.
use std::any::Any;
Expand Down Expand Up @@ -76,7 +78,7 @@ fn native_engine(py: Python, m: &Bound<'_, PyModule>) -> PyO3Result<()> {
externs::workunits::register(m)?;
externs::dep_inference::register(m)?;

m.add("PollTimeout", py.get_type_bound::<PollTimeout>())?;
m.add("PollTimeout", py.get_type::<PollTimeout>())?;

m.add_class::<PyExecutionRequest>()?;
m.add_class::<PyExecutionStrategyOptions>()?;
Expand Down Expand Up @@ -223,9 +225,9 @@ impl PyTypes {
py: Python,
) -> Self {
Self(GILProtected::new(RefCell::new(Some(Types {
directory_digest: TypeId::new(&py.get_type_bound::<externs::fs::PyDigest>()),
file_digest: TypeId::new(&py.get_type_bound::<externs::fs::PyFileDigest>()),
snapshot: TypeId::new(&py.get_type_bound::<externs::fs::PySnapshot>()),
directory_digest: TypeId::new(&py.get_type::<externs::fs::PyDigest>()),
file_digest: TypeId::new(&py.get_type::<externs::fs::PyFileDigest>()),
snapshot: TypeId::new(&py.get_type::<externs::fs::PySnapshot>()),
paths: TypeId::new(paths),
path_metadata_request: TypeId::new(path_metadata_request),
path_metadata_result: TypeId::new(path_metadata_result),
Expand All @@ -236,17 +238,17 @@ impl PyTypes {
digest_contents: TypeId::new(digest_contents),
digest_entries: TypeId::new(digest_entries),
path_globs: TypeId::new(path_globs),
merge_digests: TypeId::new(&py.get_type_bound::<externs::fs::PyMergeDigests>()),
add_prefix: TypeId::new(&py.get_type_bound::<externs::fs::PyAddPrefix>()),
remove_prefix: TypeId::new(&py.get_type_bound::<externs::fs::PyRemovePrefix>()),
merge_digests: TypeId::new(&py.get_type::<externs::fs::PyMergeDigests>()),
add_prefix: TypeId::new(&py.get_type::<externs::fs::PyAddPrefix>()),
remove_prefix: TypeId::new(&py.get_type::<externs::fs::PyRemovePrefix>()),
create_digest: TypeId::new(create_digest),
digest_subset: TypeId::new(digest_subset),
native_download_file: TypeId::new(native_download_file),
platform: TypeId::new(platform),
process: TypeId::new(process),
process_result: TypeId::new(process_result),
process_config_from_environment: TypeId::new(
&py.get_type_bound::<externs::process::PyProcessExecutionEnvironment>(),
&py.get_type::<externs::process::PyProcessExecutionEnvironment>(),
),
process_result_metadata: TypeId::new(process_result_metadata),
coroutine: TypeId::new(coroutine),
Expand All @@ -264,7 +266,7 @@ impl PyTypes {
parsed_javascript_deps_candidate_result,
),
deps_request: TypeId::new(
&py.get_type_bound::<externs::dep_inference::PyNativeDependenciesRequest>(),
&py.get_type::<externs::dep_inference::PyNativeDependenciesRequest>(),
),
}))))
}
Expand Down Expand Up @@ -627,9 +629,17 @@ fn nailgun_server_create(
let executor = py_executor.0.clone();
nailgun::Server::new(executor, port, move |exe: nailgun::RawFdExecution| {
Python::with_gil(|py| {
let args_tuple = match PyTuple::new(py, exe.cmd.args) {
Ok(t) => t,
Err(e) => {
error!("PyTuple construction failure: {e:?}");
return nailgun::ExitCode(1);
}
};

let result = runner.bind(py).call1((
exe.cmd.command,
PyTuple::new_bound(py, exe.cmd.args),
args_tuple,
exe.cmd.env.into_iter().collect::<HashMap<String, String>>(),
exe.cmd.working_dir,
PySessionCancellationLatch(exe.cancelled),
Expand Down Expand Up @@ -824,7 +834,7 @@ async fn workunit_to_py_value(
))
})?;
let has_parent_ids = !workunit.parent_ids.is_empty();
let mut dict_entries = Python::with_gil(|py| {
let mut dict_entries = Python::with_gil(|py| -> PyO3Result<Vec<(Value, Value)>> {
let mut dict_entries = vec![
(
externs::store_utf8(py, "name"),
Expand Down Expand Up @@ -852,7 +862,7 @@ async fn workunit_to_py_value(
}
dict_entries.push((
externs::store_utf8(py, "parent_ids"),
externs::store_tuple(py, parent_ids),
externs::store_tuple(py, parent_ids)?,
));

match workunit.state {
Expand Down Expand Up @@ -899,8 +909,8 @@ async fn workunit_to_py_value(
externs::store_utf8(py, desc),
));
}
dict_entries
});
Ok(dict_entries)
})?;

let mut artifact_entries = Vec::new();

Expand Down Expand Up @@ -1021,7 +1031,7 @@ async fn workunits_to_py_tuple_value(
workunit_values.push(py_value);
}

Ok(externs::store_tuple(py, workunit_values))
externs::store_tuple(py, workunit_values)
}

#[pyfunction]
Expand Down Expand Up @@ -1070,7 +1080,7 @@ fn session_poll_workunits(
completed,
&core,
))?;
Ok(externs::store_tuple(py, vec![started_val, completed_val]).into())
Ok(externs::store_tuple(py, vec![started_val, completed_val])?.into())
})
})
})
Expand Down Expand Up @@ -1433,13 +1443,12 @@ fn session_get_observation_histograms<'py>(
.map_err(PyException::new_err)
})?;

let encoded_observations = PyDict::new_bound(py);
let encoded_observations = PyDict::new(py);
for (metric, encoded_histogram) in &observations {
encoded_observations
.set_item(metric, PyBytes::new_bound(py, &encoded_histogram[..]))?;
encoded_observations.set_item(metric, PyBytes::new(py, &encoded_histogram[..]))?;
}

let result = PyDict::new_bound(py);
let result = PyDict::new(py);
result.set_item("version", OBSERVATIONS_VERSION)?;
result.set_item("histograms", encoded_observations)?;
Ok(result)
Expand Down Expand Up @@ -1535,7 +1544,7 @@ fn rule_graph_rule_gets<'py>(
) -> PyO3Result<Bound<'py, PyDict>> {
let core = &py_scheduler.borrow().0.core;
core.executor.enter(|| {
let result = PyDict::new_bound(py);
let result = PyDict::new(py);
for (rule, rule_dependencies) in core.rule_graph.rule_dependencies() {
let task = rule.0;
let function = &task.func;
Expand Down Expand Up @@ -1825,7 +1834,7 @@ fn single_file_digests_to_bytes<'py>(
.map(|values| values.into_iter().map(|val| val.into()).collect())
.map_err(possible_store_missing_digest)?;

let output_list = PyList::new_bound(py, &bytes_values);
let output_list = PyList::new(py, &bytes_values)?;
Ok(output_list)
})
}
Expand Down
Loading

0 comments on commit 58aea21

Please sign in to comment.