[internal] Port match_path_globs to PyO3
#12225
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| // Copyright 2021 Pants project contributors (see CONTRIBUTORS.md). | ||
| // Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
|
||
| use std::path::Path; | ||
|
|
||
| use fs::{GlobExpansionConjunction, PathGlobs, PreparedPathGlobs, StrictGlobMatching}; | ||
| use pyo3::exceptions::PyValueError; | ||
| use pyo3::prelude::*; | ||
| use pyo3::wrap_pyfunction; | ||
|
|
||
| pub(crate) fn register(m: &PyModule) -> PyResult<()> { | ||
| m.add_function(wrap_pyfunction!(match_path_globs, m)?)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| struct PyPathGlobs(PathGlobs); | ||
|
|
||
| impl PyPathGlobs { | ||
| fn parse(self) -> PyResult<PreparedPathGlobs> { | ||
| self.0.clone().parse().map_err(|e| { | ||
| PyValueError::new_err(format!( | ||
| "Failed to parse PathGlobs: {:?}\n\nError: {}", | ||
| self.0, e | ||
| )) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl<'source> FromPyObject<'source> for PyPathGlobs { | ||
| fn extract(obj: &'source PyAny) -> PyResult<Self> { | ||
| let globs: Vec<String> = obj.getattr("globs")?.extract()?; | ||
|
|
||
| let description_of_origin_field: String = obj.getattr("description_of_origin")?.extract()?; | ||
| let description_of_origin = if description_of_origin_field.is_empty() { | ||
| None | ||
| } else { | ||
| Some(description_of_origin_field) | ||
| }; | ||
|
|
||
| let match_behavior_str: &str = obj | ||
| .getattr("glob_match_error_behavior")? | ||
| .getattr("value")? | ||
| .extract()?; | ||
| let match_behavior = StrictGlobMatching::create(match_behavior_str, description_of_origin) | ||
|
There was a problem hiding this comment. I was kind of hoping for an API closer to let path_globs: PathGlobs = pyo3_serde::from_object(obj)?;rather than having to get each field by field-name and put them together yourself... That would probably necessitate I guess this is PyO3/pyo3#566 and I definitely wouldn't suggest implementing it on the critical path of this conversion, but it would be nice to avoid that boilerplate... :) There was a problem hiding this comment. It's also worth calling out that this wrapping of a Python object is an alternative to making the object native as we do for a few types in pants/src/rust/engine/src/externs/fs.rs Line 67 in 8824cc3 I'm kindof curious what making the object native would look like with There was a problem hiding this comment. Agreed. I didn't go with a native class because we already have examples of that and to land the port successfully I want to avoid rewriting everything to do that. But after the port is finished, it makes sense. There was a problem hiding this comment. You might be able to do something like this to avoid a lot of the use pyo3::FromPyObject;
#[derive(FromPyObject)]
struct GlobMatchErrorBehavior<'a> {
value: &'a str,
}
#[derive(FromPyObject)]
struct Conjunction<'a> {
value: &'a str,
}
#[derive(FromPyObject)]
struct PyPathGlobs<'a> {
globs: Vec<String>,
description_of_origin: String,
glob_match_error_behaviour: GlobMatchErrorBehavior<'a>,
conjunction: Conjunction<'a>,
} |
||
| .map_err(PyValueError::new_err)?; | ||
|
|
||
| let conjunction_str: &str = obj.getattr("conjunction")?.getattr("value")?.extract()?; | ||
| let conjunction = | ||
| GlobExpansionConjunction::create(conjunction_str).map_err(PyValueError::new_err)?; | ||
|
|
||
| Ok(PyPathGlobs(PathGlobs::new( | ||
| globs, | ||
| match_behavior, | ||
| conjunction, | ||
| ))) | ||
| } | ||
| } | ||
|
|
||
| #[pyfunction] | ||
| fn match_path_globs( | ||
| py_path_globs: PyPathGlobs, | ||
| paths: Vec<String>, | ||
| py: Python, | ||
| ) -> PyResult<Vec<String>> { | ||
| py.allow_threads(|| { | ||
| let path_globs = py_path_globs.parse()?; | ||
| Ok( | ||
| paths | ||
| .into_iter() | ||
| .filter(|p| path_globs.matches(&Path::new(p))) | ||
| .collect(), | ||
| ) | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! This
clone()might explain the slowdown in the contrived benchmark. We clone anArcinstead with Rust-CPython.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, that's not it. And the
PathGlobsis cheap in the example. It's thepaths: Vec<String>that's so huge and would be painful to clone.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this
fntakes ownership ofself, it shouldn't need to clone: it can consumeself(includingself.0).EDIT: Oh. But we clone to render the error message. Hm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the error type is
String- presumably the glob which failed to parse? You might not need toclonethe rest of the globs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, although we want to show what
selfwas in the error message so it's more useful. Not technically necessary, but I think it's fine because thePathGlobsobject is usually pretty small (a vec of a few strings).@stuhood thoughts?