Skip to content

Commit

Permalink
PyO3: Rename Pants-specific helper methods to remove "bound" from the…
Browse files Browse the repository at this point in the history
… name (#21683)

While migrating from PyO3 v0.21.x to v0.23.x, we renamed many functions
to include "_bound" in the name so that we could migrate to the `Bound`
API in a piecemeal fashion. The upgrade to v0.23.x is done so all of
these functions can be renamed back to their original names before the
migration.

This PR only contains function renames.
  • Loading branch information
tdyas authored Nov 23, 2024
1 parent 2e176f0 commit e751c71
Show file tree
Hide file tree
Showing 11 changed files with 119 additions and 142 deletions.
8 changes: 4 additions & 4 deletions src/rust/engine/src/externs/engine_aware.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::sync::Arc;

use crate::externs;
use crate::externs::fs::PyFileDigest;
use crate::nodes::{lift_directory_digest_bound, lift_file_digest_bound};
use crate::nodes::{lift_directory_digest, lift_file_digest};
use crate::Value;

use pyo3::prelude::*;
Expand Down Expand Up @@ -50,7 +50,7 @@ impl EngineAwareReturnType {
if level_val.is_none() {
return None;
}
externs::val_to_log_level_bound(&level_val).ok()
externs::val_to_log_level(&level_val).ok()
}

fn message(obj: &Bound<'_, PyAny>) -> Option<String> {
Expand All @@ -73,10 +73,10 @@ impl EngineAwareReturnType {
for kv_pair in artifacts_dict.items().into_iter() {
let (key, value): (String, Bound<'_, PyAny>) = kv_pair.extract().ok()?;
let artifact_output = if value.is_instance_of::<PyFileDigest>() {
lift_file_digest_bound(&value).map(ArtifactOutput::FileDigest)
lift_file_digest(&value).map(ArtifactOutput::FileDigest)
} else {
let digest_value = value.getattr("digest").ok()?;
lift_directory_digest_bound(&digest_value)
lift_directory_digest(&digest_value)
.map(|dd| ArtifactOutput::Snapshot(Arc::new(dd)))
}
.ok()?;
Expand Down
28 changes: 13 additions & 15 deletions src/rust/engine/src/externs/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1532,7 +1532,7 @@ fn rule_graph_consumed_types<'py>(
Ok(subgraph
.consumed_types()
.into_iter()
.map(|type_id| type_id.as_py_type_bound(py))
.map(|type_id| type_id.as_py_type(py))
.collect())
})
}
Expand Down Expand Up @@ -1569,10 +1569,10 @@ fn rule_graph_rule_gets<'py>(
let provided_params = dependency_key
.provided_params
.iter()
.map(|p| p.as_py_type_bound(py))
.map(|p| p.as_py_type(py))
.collect::<Vec<_>>();
dependencies.push((
dependency_key.product.as_py_type_bound(py),
dependency_key.product.as_py_type(py),
provided_params,
function.0.value.into_pyobject(py)?.into_any().unbind(),
));
Expand Down Expand Up @@ -1721,23 +1721,22 @@ fn capture_snapshots(
// TODO: A parent_id should be an explicit argument.
session.workunit_store().init_thread_state(None);

let values = externs::collect_iterable_bound(path_globs_and_root_tuple_wrapper).unwrap();
let values = externs::collect_iterable(path_globs_and_root_tuple_wrapper).unwrap();
let path_globs_and_roots = values
.into_iter()
.map(|value| {
let root: PathBuf = externs::getattr_bound(&value, "root")?;
let root: PathBuf = externs::getattr(&value, "root")?;
let path_globs = {
let path_globs_py_value =
externs::getattr_bound::<Bound<'_, PyAny>>(&value, "path_globs")?;
nodes::Snapshot::lift_prepared_path_globs_bound(&path_globs_py_value)
externs::getattr::<Bound<'_, PyAny>>(&value, "path_globs")?;
nodes::Snapshot::lift_prepared_path_globs(&path_globs_py_value)
};
let digest_hint = {
let maybe_digest: Bound<'_, PyAny> =
externs::getattr_bound(&value, "digest_hint")?;
let maybe_digest: Bound<'_, PyAny> = externs::getattr(&value, "digest_hint")?;
if maybe_digest.is_none() {
None
} else {
Some(nodes::lift_directory_digest_bound(&maybe_digest)?)
Some(nodes::lift_directory_digest(&maybe_digest)?)
}
};
path_globs.map(|path_globs| (path_globs, root, digest_hint))
Expand Down Expand Up @@ -1782,9 +1781,9 @@ fn ensure_remote_has_recursive(
let digests: Vec<Digest> = py_digests
.iter()
.map(|value| {
crate::nodes::lift_directory_digest_bound(&value)
crate::nodes::lift_directory_digest(&value)
.map(|dd| dd.as_digest())
.or_else(|_| crate::nodes::lift_file_digest_bound(&value))
.or_else(|_| crate::nodes::lift_file_digest(&value))
})
.collect::<Result<Vec<Digest>, _>>()
.map_err(PyException::new_err)?;
Expand All @@ -1807,7 +1806,7 @@ fn ensure_directory_digest_persisted(
let core = &py_scheduler.borrow().0.core;
core.executor.enter(|| {
let digest =
crate::nodes::lift_directory_digest_bound(py_digest).map_err(PyException::new_err)?;
crate::nodes::lift_directory_digest(py_digest).map_err(PyException::new_err)?;

py.allow_threads(|| {
core.executor
Expand Down Expand Up @@ -1877,8 +1876,7 @@ fn write_digest(
// TODO: A parent_id should be an explicit argument.
session.workunit_store().init_thread_state(None);

let lifted_digest =
nodes::lift_directory_digest_bound(digest).map_err(PyValueError::new_err)?;
let lifted_digest = nodes::lift_directory_digest(digest).map_err(PyValueError::new_err)?;

// Python will have already validated that path_prefix is a relative path.
let path_prefix = Path::new(&path_prefix);
Expand Down
42 changes: 18 additions & 24 deletions src/rust/engine/src/externs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ pub fn store_bool(py: Python, val: bool) -> Value {
///
/// Gets an attribute of the given value as the given type.
///
pub fn getattr_bound<'py, T>(value: &Bound<'py, PyAny>, field: &str) -> Result<T, String>
pub fn getattr<'py, T>(value: &Bound<'py, PyAny>, field: &str) -> Result<T, String>
where
T: FromPyObject<'py>,
{
Expand All @@ -188,17 +188,15 @@ where
///
/// Collect the Values contained within an outer Python Iterable PyObject.
///
pub fn collect_iterable_bound<'py>(
value: &Bound<'py, PyAny>,
) -> Result<Vec<Bound<'py, PyAny>>, String> {
pub fn collect_iterable<'py>(value: &Bound<'py, PyAny>) -> Result<Vec<Bound<'py, PyAny>>, String> {
match value.try_iter() {
Ok(py_iter) => py_iter
.enumerate()
.map(|(i, py_res)| {
py_res.map_err(|py_err| {
format!(
"Could not iterate {}, failed to extract {}th item: {:?}",
val_to_str_bound(value),
val_to_str(value),
i,
py_err
)
Expand All @@ -207,27 +205,27 @@ pub fn collect_iterable_bound<'py>(
.collect(),
Err(py_err) => Err(format!(
"Could not iterate {}: {:?}",
val_to_str_bound(value),
val_to_str(value),
py_err
)),
}
}

/// Read a `FrozenDict[str, T]`.
pub fn getattr_from_str_frozendict_bound<'py, T: FromPyObject<'py>>(
pub fn getattr_from_str_frozendict<'py, T: FromPyObject<'py>>(
value: &Bound<'py, PyAny>,
field: &str,
) -> BTreeMap<String, T> {
let frozendict: Bound<PyAny> = getattr_bound(value, field).unwrap();
let pydict: Bound<PyDict> = getattr_bound(&frozendict, "_data").unwrap();
let frozendict: Bound<PyAny> = getattr(value, field).unwrap();
let pydict: Bound<PyDict> = getattr(&frozendict, "_data").unwrap();
pydict
.items()
.into_iter()
.map(|kv_pair| kv_pair.extract().unwrap())
.collect()
}

pub fn getattr_as_optional_string_bound(
pub fn getattr_as_optional_string(
value: &Bound<'_, PyAny>,
field: &str,
) -> PyResult<Option<String>> {
Expand All @@ -239,22 +237,18 @@ pub fn getattr_as_optional_string_bound(
/// Call the equivalent of `str()` on an arbitrary Python object.
///
/// Converts `None` to the empty string.
pub fn val_to_str_bound(obj: &Bound<'_, PyAny>) -> String {
pub fn val_to_str(obj: &Bound<'_, PyAny>) -> String {
if obj.is_none() {
return "".to_string();
}
obj.str().unwrap().extract().unwrap()
}

pub fn val_to_log_level_bound(obj: &Bound<'_, PyAny>) -> Result<log::Level, String> {
let res: Result<PythonLogLevel, String> = getattr_bound(obj, "_level").and_then(|n: u64| {
pub fn val_to_log_level(obj: &Bound<'_, PyAny>) -> Result<log::Level, String> {
let res: Result<PythonLogLevel, String> = getattr(obj, "_level").and_then(|n: u64| {
n.try_into()
.map_err(|e: num_enum::TryFromPrimitiveError<_>| {
format!(
"Could not parse {:?} as a LogLevel: {}",
val_to_str_bound(obj),
e
)
format!("Could not parse {:?} as a LogLevel: {}", val_to_str(obj), e)
})
});
res.map(|py_level| py_level.into())
Expand Down Expand Up @@ -365,7 +359,7 @@ pub(crate) fn generator_send(
let gog = gog?;
// TODO: Find a better way to check whether something is a coroutine... this seems
// unnecessarily awkward.
if gog.is_instance(&generator_type.as_py_type_bound(py))? {
if gog.is_instance(&generator_type.as_py_type(py))? {
Ok(GetOrGenerator::Generator(Value::new(gog.unbind())))
} else if let Ok(get) = gog.extract::<PyRef<PyGeneratorResponseGet>>() {
Ok(GetOrGenerator::Get(
Expand All @@ -392,7 +386,7 @@ pub(crate) fn generator_send(
/// NB: Panics on failure. Only recommended for use with built-in types, such as
/// those configured in types::Types.
pub fn unsafe_call(py: Python, type_id: TypeId, args: &[Value]) -> Value {
let py_type = type_id.as_py_type_bound(py);
let py_type = type_id.as_py_type(py);
let args_tuple = PyTuple::new(py, args.iter().map(|v| v.bind(py))).unwrap_or_else(|e| {
panic!("Core type constructor `PyTuple` failed: {e:?}",);
});
Expand Down Expand Up @@ -582,7 +576,7 @@ impl PyGeneratorResponseCall {

#[getter]
fn output_type<'py>(&self, py: Python<'py>) -> PyResult<Bound<'py, PyType>> {
Ok(self.borrow_inner(py)?.output_type.as_py_type_bound(py))
Ok(self.borrow_inner(py)?.output_type.as_py_type(py))
}

#[getter]
Expand All @@ -591,7 +585,7 @@ impl PyGeneratorResponseCall {
.borrow_inner(py)?
.input_types
.iter()
.map(|t| t.as_py_type_bound(py))
.map(|t| t.as_py_type(py))
.collect())
}

Expand Down Expand Up @@ -674,7 +668,7 @@ impl PyGeneratorResponseGet {
)
})?
.output
.as_py_type_bound(py))
.as_py_type(py))
}

#[getter]
Expand All @@ -691,7 +685,7 @@ impl PyGeneratorResponseGet {
})?
.input_types
.iter()
.map(|t| t.as_py_type_bound(py))
.map(|t| t.as_py_type(py))
.collect())
}

Expand Down
43 changes: 21 additions & 22 deletions src/rust/engine/src/intrinsics/digests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ use crate::externs::fs::{
};
use crate::externs::PyGeneratorResponseNativeCall;
use crate::nodes::{
lift_directory_digest_bound, task_get_context, unmatched_globs_additional_context,
DownloadedFile, NodeResult, PathMetadataNode, Snapshot, SubjectPath,
lift_directory_digest, task_get_context, unmatched_globs_additional_context, DownloadedFile,
NodeResult, PathMetadataNode, Snapshot, SubjectPath,
};
use crate::python::{throw, Key, Value};
use crate::Failure;
Expand Down Expand Up @@ -50,7 +50,7 @@ fn get_digest_contents(digest: Value) -> PyGeneratorResponseNativeCall {

let digest = Python::with_gil(|py| {
let py_digest = digest.bind(py);
lift_directory_digest_bound(py_digest)
lift_directory_digest(py_digest)
})?;

let digest_contents = context.core.store().contents_for_directory(digest).await?;
Expand All @@ -68,7 +68,7 @@ fn get_digest_entries(digest: Value) -> PyGeneratorResponseNativeCall {

let digest = Python::with_gil(|py| {
let py_digest = digest.bind(py);
lift_directory_digest_bound(py_digest)
lift_directory_digest(py_digest)
})?;
let digest_entries = context.core.store().entries_for_directory(digest).await?;
Ok::<_, Failure>(Python::with_gil(|py| {
Expand Down Expand Up @@ -130,7 +130,7 @@ fn digest_to_snapshot(digest: Value) -> PyGeneratorResponseNativeCall {

let digest = Python::with_gil(|py| {
let py_digest = digest.bind(py);
lift_directory_digest_bound(py_digest)
lift_directory_digest(py_digest)
})?;
let snapshot = store::Snapshot::from_digest(store, digest).await?;
Ok::<_, Failure>(Python::with_gil(|py| {
Expand Down Expand Up @@ -181,7 +181,7 @@ fn path_globs_to_digest(path_globs: Value) -> PyGeneratorResponseNativeCall {

let path_globs = Python::with_gil(|py| {
let py_path_globs = path_globs.bind(py);
Snapshot::lift_path_globs_bound(py_path_globs)
Snapshot::lift_path_globs(py_path_globs)
})
.map_err(|e| throw(format!("Failed to parse PathGlobs: {e}")))?;
let snapshot = context.get(Snapshot::from_path_globs(path_globs)).await?;
Expand All @@ -199,7 +199,7 @@ fn path_globs_to_paths(path_globs: Value) -> PyGeneratorResponseNativeCall {

let path_globs = Python::with_gil(|py| {
let py_path_globs = path_globs.bind(py);
Snapshot::lift_path_globs_bound(py_path_globs)
Snapshot::lift_path_globs(py_path_globs)
})
.map_err(|e| throw(format!("Failed to parse PathGlobs: {e}")))?;

Expand Down Expand Up @@ -255,28 +255,28 @@ fn create_digest(py: Python, create_digest: Value) -> PyGeneratorResponseNativeC
let items: Vec<CreateDigestItem> = {
Python::with_gil(|py| {
let py_create_digest = create_digest.bind(py);
externs::collect_iterable_bound(py_create_digest)
externs::collect_iterable(py_create_digest)
.unwrap()
.into_iter()
.map(|obj| {
let raw_path: String = externs::getattr_bound(&obj, "path").unwrap();
let raw_path: String = externs::getattr(&obj, "path").unwrap();
let path = RelativePath::new(PathBuf::from(raw_path)).unwrap();
if obj.hasattr("content").unwrap() {
let bytes = bytes::Bytes::from(
externs::getattr_bound::<Vec<u8>>(&obj, "content").unwrap(),
externs::getattr::<Vec<u8>>(&obj, "content").unwrap(),
);
let is_executable: bool =
externs::getattr_bound(&obj, "is_executable").unwrap();
externs::getattr(&obj, "is_executable").unwrap();
new_file_count += 1;
CreateDigestItem::FileContent(path, bytes, is_executable)
} else if obj.hasattr("file_digest").unwrap() {
let py_file_digest: PyFileDigest =
externs::getattr_bound(&obj, "file_digest").unwrap();
externs::getattr(&obj, "file_digest").unwrap();
let is_executable: bool =
externs::getattr_bound(&obj, "is_executable").unwrap();
externs::getattr(&obj, "is_executable").unwrap();
CreateDigestItem::FileEntry(path, py_file_digest.0, is_executable)
} else if obj.hasattr("target").unwrap() {
let target: String = externs::getattr_bound(&obj, "target").unwrap();
let target: String = externs::getattr(&obj, "target").unwrap();
CreateDigestItem::SymlinkEntry(path, PathBuf::from(target))
} else {
CreateDigestItem::Dir(path)
Expand Down Expand Up @@ -343,12 +343,11 @@ fn digest_subset_to_digest(digest_subset: Value) -> PyGeneratorResponseNativeCal
let (path_globs, original_digest) = Python::with_gil(|py| {
let py_digest_subset = digest_subset.bind(py);
let py_path_globs: Bound<'_, PyAny> =
externs::getattr_bound(py_digest_subset, "globs").unwrap();
let py_digest: Bound<'_, PyAny> =
externs::getattr_bound(py_digest_subset, "digest").unwrap();
externs::getattr(py_digest_subset, "globs").unwrap();
let py_digest: Bound<'_, PyAny> = externs::getattr(py_digest_subset, "digest").unwrap();
let res: NodeResult<_> = Ok((
Snapshot::lift_prepared_path_globs_bound(&py_path_globs)?,
lift_directory_digest_bound(&py_digest)?,
Snapshot::lift_prepared_path_globs(&py_path_globs)?,
lift_directory_digest(&py_digest)?,
));
res
})?;
Expand All @@ -365,11 +364,11 @@ fn path_metadata_request(single_path: Value) -> PyGeneratorResponseNativeCall {
PyGeneratorResponseNativeCall::new(async move {
let subject_path = Python::with_gil(|py| -> Result<_, String> {
let arg = single_path.bind(py);
let path = externs::getattr_as_optional_string_bound(arg, "path")
let path = externs::getattr_as_optional_string(arg, "path")
.map_err(|e| format!("Failed to get `path` for field: {e}"))?;
let path = path.ok_or_else(|| "Path must not be `None`.".to_string())?;

let namespace: PyPathNamespace = externs::getattr_bound(arg, "namespace")
let namespace: PyPathNamespace = externs::getattr(arg, "namespace")
.map_err(|e| format!("Failed to get `namespace` for field: {e}"))?;
match namespace {
PyPathNamespace::Workspace => SubjectPath::new_workspace(&path).map_err(|_| {
Expand Down Expand Up @@ -400,7 +399,7 @@ fn path_metadata_request(single_path: Value) -> PyGeneratorResponseNativeCall {
None => py.None().into_bound(py),
};

let py_type = context.core.types.path_metadata_result.as_py_type_bound(py);
let py_type = context.core.types.path_metadata_result.as_py_type(py);
let args_tuple = PyTuple::new(py, &[path_metadata_opt]).unwrap_or_else(|e| {
panic!("Core type constructor `PyTuple` failed: {e:?}");
});
Expand Down
Loading

0 comments on commit e751c71

Please sign in to comment.