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

Introduce support for per field instance default values. #20949

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/notes/2.22.x.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ Setting [the `orphan_files_behaviour = "ignore"` option](https://www.pantsbuild.

The `PythonToolRequirementsBase` and `PythonToolBase` classes now have a new `help_short` field. Subclasses should now use `help_short` instead of the `help` field. The `help` field will be automatically generated using `help_short`, and will include the tool's default package version and provide instructions on how to override this version using a custom lockfile.

Target `Field` types can now have per instance default values, overriding the class var `default` value declared on the `Field` subclass by wrapping the field value in a `FieldDefaultValue` when creating the field instance.

## Full Changelog

For the full changelog, see the individual GitHub Releases for this series: https://github.com/pantsbuild/pants/releases
23 changes: 23 additions & 0 deletions src/python/pants/engine/internals/native_engine.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,29 @@ class _NoValue:
# Marker for unspecified field values that should use the default value if applicable.
NO_VALUE: _NoValue

class FieldDefaultValue:
"""Field value wrapper to indicate this should be treated as the default value for this instance
of the field.

Example:
class SomeField:
default = "some"

field = SomeField(raw_value=FieldDefaultValue("other"), address=addr)
field.default == "other"
field.value == "other"

The value is unwrapped and assigned to the `default` property on the field instance. During
`compute_value`, the wrapped value is passed as `raw_value`, while the unwrapped value is
returned from `super().compute_value(...)`.
"""

value: ImmutableValue

def __init__(self, value: ImmutableValue) -> None: ...
def __repr__(self) -> str: ...
def __str__(self) -> str: ...

class Field:
"""A Field.

Expand Down
3 changes: 3 additions & 0 deletions src/python/pants/engine/target.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@
)
from pants.engine.internals.native_engine import NO_VALUE as NO_VALUE # noqa: F401
from pants.engine.internals.native_engine import Field as Field # noqa: F401
from pants.engine.internals.native_engine import ( # noqa: F401
FieldDefaultValue as FieldDefaultValue,
)
from pants.engine.unions import UnionMembership, UnionRule, distinct_union_type_per_subclass, union
from pants.option.global_options import UnmatchedBuildFileGlobs
from pants.source.filespec import Filespec, FilespecMatcher
Expand Down
25 changes: 25 additions & 0 deletions src/python/pants/engine/target_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
DictStringToStringSequenceField,
ExplicitlyProvidedDependencies,
Field,
FieldDefaultValue,
FieldSet,
FloatField,
GeneratedTargets,
Expand Down Expand Up @@ -471,6 +472,30 @@ def test_target_residence_dir() -> None:
)


def test_field_dynamic_default() -> None:
class SomeField(Field):
alias: ClassVar[str] = "some"
default: ClassVar[str] = "some-default"

@classmethod
def compute_value(cls, raw_value: Optional[str], address: Address) -> str:
value_or_default = super().compute_value(raw_value, address)
assert isinstance(value_or_default, str)
return value_or_default

def check_field(fld: SomeField, value: str, default: str) -> None:
# The class var should never change.
assert SomeField.default == "some-default"
assert fld.value == value
assert fld.default == default

addr = Address("a")
check_field(SomeField("regular", addr), "regular", SomeField.default)
check_field(
SomeField(FieldDefaultValue("other-default"), addr), "other-default", "other-default"
)


# -----------------------------------------------------------------------------------------------
# Test CoarsenedTarget
# -----------------------------------------------------------------------------------------------
Expand Down
132 changes: 100 additions & 32 deletions src/rust/engine/src/externs/target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,18 @@

use std::fmt::Write;

use crate::externs::address::Address;
use pyo3::basic::CompareOp;
use pyo3::exceptions::PyValueError;
use pyo3::ffi;
use pyo3::intern;
use pyo3::prelude::*;
use pyo3::types::PyType;

use crate::externs::address::Address;

pub fn register(m: &PyModule) -> PyResult<()> {
m.add_class::<Field>()?;
m.add_class::<NoFieldValue>()?;

m.add_class::<FieldDefaultValue>()?;
m.add("NO_VALUE", NoFieldValue)?;

Ok(())
Expand All @@ -35,9 +35,34 @@ impl NoFieldValue {
}
}

#[pyclass]
#[derive(Clone)]
struct FieldDefaultValue {
value: PyObject,
}

#[pymethods]
impl FieldDefaultValue {
#[new]
fn __new__(value: PyObject) -> Self {
FieldDefaultValue { value }
}

fn __str__(self_: &PyCell<Self>) -> PyResult<String> {
Ok(format!("default({})", self_.borrow().value))
}

fn __repr__(self_: &PyCell<Self>) -> PyResult<String> {
Ok(format!("<FieldDefaultValue({})>", self_.borrow().value))
}
}

#[pyclass(subclass)]
#[derive(Clone)]
pub struct Field {
value: PyObject,
// Overrides the class var default per instance when set.
default: Option<PyObject>,
}

#[pymethods]
Expand All @@ -59,17 +84,28 @@ impl Field {
let raw_value = match raw_value {
Some(value)
if value.extract::<NoFieldValue>(py).is_ok()
&& !Self::cls_none_is_valid_value(cls)? =>
&& !Self::cls_none_is_valid_value(cls, py)? =>
{
None
}
rv => rv,
};
let maybe_default = match raw_value {
Some(ref value) => {
if let Ok(default) = value.extract::<FieldDefaultValue>(py) {
Some(default.value)
} else {
None
}
}
_ => None,
};

Ok(Self {
value: cls
.call_method(intern!(py, "compute_value"), (raw_value, address), None)?
.into(),
default: maybe_default,
})
}

Expand Down Expand Up @@ -112,26 +148,32 @@ impl Field {
py: Python,
) -> PyResult<PyObject> {
let default = || -> PyResult<PyObject> {
if Self::cls_required(cls)? {
if Self::cls_required(cls, py)? {
// TODO: Should be `RequiredFieldMissingException`.
Err(PyValueError::new_err(format!(
"The `{}` field in target {} must be defined.",
Self::cls_alias(cls)?,
Self::cls_alias(cls, py)?,
*address,
)))
} else {
Self::cls_default(cls)
Self::cls_default(cls, py)
}
};

let none_is_valid_value = Self::cls_none_is_valid_value(cls)?;
let none_is_valid_value = Self::cls_none_is_valid_value(cls, py)?;
match raw_value {
Some(value) if none_is_valid_value && value.extract::<NoFieldValue>(py).is_ok() => {
default()
}
None if none_is_valid_value => Ok(py.None()),
None => default(),
Some(value) => Ok(value),
Some(value) => {
if let Ok(dyn_default) = value.extract::<FieldDefaultValue>(py) {
Ok(dyn_default.value)
} else {
Ok(value)
}
}
}
}

Expand All @@ -144,28 +186,28 @@ impl Field {
Ok(self_.get_type().hash()? & self_.borrow().value.as_ref(py).hash()?)
}

fn __repr__(self_: &PyCell<Self>) -> PyResult<String> {
fn __repr__(self_: &PyCell<Self>, py: Python) -> PyResult<String> {
let mut result = String::new();
write!(
result,
"{}(alias={}, value={}",
self_.get_type(),
Self::cls_alias(self_)?,
Self::cls_alias(self_, py)?,
self_.borrow().value
)
.unwrap();
if let Ok(default) = self_.getattr("default") {
if let Ok(default) = self_.getattr(intern!(py, "default")) {
write!(result, ", default={})", default).unwrap();
} else {
write!(result, ")").unwrap();
}
Ok(result)
}

fn __str__(self_: &PyCell<Self>) -> PyResult<String> {
fn __str__(self_: &PyCell<Self>, py: Python) -> PyResult<String> {
Ok(format!(
"{}={}",
Self::cls_alias(self_)?,
Self::cls_alias(self_, py)?,
self_.borrow().value
))
}
Expand All @@ -188,32 +230,58 @@ impl Field {
_ => Ok(py.NotImplemented()),
}
}

fn __getattribute__<'a>(
self_: &'a PyCell<Self>,
name: String,
py: Python<'a>,
) -> PyResult<*mut ffi::PyObject> {
if name == "default" {
if let Some(default) = &self_.extract::<Self>()?.default {
// Return instance default, overriding the field class var default.
return Ok(default.as_ptr());
}
}

unsafe {
// The ffi::PyObject_GenericGetAttr() call is unsafe, so we need to be in an unsafe
// context to call it.
let slf = self_.borrow_mut().into_ptr();
let attr = name.into_py(py).into_ptr();
let res = ffi::PyObject_GenericGetAttr(slf, attr);
Copy link
Member Author

@kaos kaos May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call (along with how to handle the returned value) was taken from https://github.com/PyO3/pyo3/blob/dc4f114d6772337d8b1568c6dbb0bf35deaa84dd/src/impl_/pyclass.rs#L212

if res.is_null() {
Err(PyErr::fetch(py))
} else {
Ok(res)
}
}
}
Comment on lines +246 to +258
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

impl Field {
fn cls_none_is_valid_value(cls: &PyAny) -> PyResult<bool> {
cls.getattr("none_is_valid_value")?.extract::<bool>()
fn cls_none_is_valid_value(cls: &PyAny, py: Python) -> PyResult<bool> {
cls.getattr(intern!(py, "none_is_valid_value"))?
.extract::<bool>()
}

fn cls_default(cls: &PyAny) -> PyResult<PyObject> {
cls.getattr("default")?.extract()
fn cls_default(cls: &PyAny, py: Python) -> PyResult<PyObject> {
cls.getattr(intern!(py, "default"))?.extract()
}

fn cls_required(cls: &PyAny) -> PyResult<bool> {
cls.getattr("required")?.extract()
fn cls_required(cls: &PyAny, py: Python) -> PyResult<bool> {
cls.getattr(intern!(py, "required"))?.extract()
}

fn cls_alias(cls: &PyAny) -> PyResult<&str> {
// TODO: All of these methods should use interned attr names.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed ^^ while I was at it..

cls.getattr("alias")?.extract()
fn cls_alias<'a>(cls: &'a PyAny, py: Python<'a>) -> PyResult<&'a str> {
cls.getattr(intern!(py, "alias"))?.extract()
}

fn cls_removal_version(cls: &PyAny) -> PyResult<Option<&str>> {
cls.getattr("removal_version")?.extract()
fn cls_removal_version<'a>(cls: &'a PyAny, py: Python<'a>) -> PyResult<Option<&'a str>> {
cls.getattr(intern!(py, "removal_version"))?.extract()
}

fn cls_removal_hint(cls: &PyAny) -> PyResult<Option<&str>> {
cls.getattr("removal_hint")?.extract()
fn cls_removal_hint<'a>(cls: &'a PyAny, py: Python<'a>) -> PyResult<Option<&'a str>> {
cls.getattr(intern!(py, "removal_hint"))?.extract()
}

fn check_deprecated(
Expand All @@ -225,24 +293,24 @@ impl Field {
if address.is_generated_target() {
return Ok(());
}
let Some(removal_version) = Self::cls_removal_version(cls)? else {
let Some(removal_version) = Self::cls_removal_version(cls, py)? else {
return Ok(());
};
match raw_value {
Some(value) if value.extract::<NoFieldValue>(py).is_ok() => return Ok(()),
_ => (),
}

let Some(removal_hint) = Self::cls_removal_hint(cls)? else {
let Some(removal_hint) = Self::cls_removal_hint(cls, py)? else {
return Err(PyValueError::new_err(
"You specified `removal_version` for {cls:?}, but not the class \
property `removal_hint`.",
));
};

let alias = Self::cls_alias(cls)?;
let deprecated = PyModule::import(py, "pants.base.deprecated")?;
deprecated.getattr("warn_or_error")?.call(
let alias = Self::cls_alias(cls, py)?;
let deprecated = PyModule::import(py, intern!(py, "pants.base.deprecated"))?;
deprecated.getattr(intern!(py, "warn_or_error"))?.call(
(
removal_version,
format!("the {alias} field"),
Expand Down
Loading