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

PyO3: Rename Pants-specific helper methods to remove "bound" from the name #21683

Merged
merged 13 commits into from
Nov 23, 2024
Merged
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
Loading