fix issue with optional fields in dependency validator#23121
fix issue with optional fields in dependency validator#23121
Conversation
tdyas
left a comment
There was a problem hiding this comment.
My main question here is whether Field | None is even valid for FieldSet. The issue is caused by DependencyValidationFieldSet.
If the syntax is not valid, then we probably need an alternate solution than the one in this PR which is to extract a Field from Field | None.
| [ | ||
| *pants_args_for_python_lockfiles, | ||
| f"--python-interpreter-constraints=['=={current_interpreter}']", | ||
| "--python-default-resolve=a", |
There was a problem hiding this comment.
Fixing this bug resulting in these uses of python_distribution defaulting to python-default resolve even though only a and b resolves are defined. python_distribution does not appear to have resolve field, is that intentional?
There was a problem hiding this comment.
Presumably it is intentional, since the python_distribution is just some metadata on how to build a wheel, and the resolve doesn't enter into that?
There was a problem hiding this comment.
Then it seems incorrect for the PR's solution to even apply the notion of a resolve to python_distribution then. Do you concur?
There was a problem hiding this comment.
These tests began failing with this PR's solution in place and I added default resolve options to "fix" the test failures. Maybe this PR needs to account for the optionality of PythonResolveField | None in DependencyValidationFieldSet or that optionality is incorrect and FieldSets should never have optional types?
There was a problem hiding this comment.
I think that is the only Pythonic target type with no resolve field, so maybe even just for uniformity... The trouble is that "empty resolve field" defaults to the repo default, IIRC, and if that is not compatible with the resolve of the dependencies then things would blow up, right?
There was a problem hiding this comment.
I dug in more on the issue today. The issue here is that when an optional field in a FieldSet is missing from a target, then the field is instantiated with its default value instead of being set to None as a result of Target.get.
So DependencyValidationFieldSet is trying to use a semantic (i.e., return None for missing fields) which FieldSet does not support at all. And that is why the resolve-specific ICs are not applied at
pants/src/python/pants/backend/python/target_types_rules.py
Lines 649 to 651 in 73d7a82
resolve on DependencyValidationFieldSet is always None.
So potential solutions:
-
python_distributionappears to be the only Python target without aresolvefield. Yet, the source code for the distribution would need to be specified asdependenciesand would need to be from a single resolve, right? So one view is thatpython_distributionshould have aresolvefield. Could apython_distributionever be compatible with dependency targets from multiple resolves? -
We keep
python_distributionwithout aresolvefield and just fix theFieldSetsystem to understandField | None. I have that support already in place locally. It will be pushed after this comment. It also validates that allFieldSets have fields annotated with types eitherFieldorField | None.
There was a problem hiding this comment.
Given what you've discovered, seems like 1. should work? IIUC the behavior is effectively this already (since the target doesn't have this field, we assume the default resolve, not no resolve)?
There was a problem hiding this comment.
With the original fix, the code assumes the default resolve. With the modified fix in today's commits, the FieldSet machinery is able to return None for PythonResolveField | None as a reader might have assumed to be the case. So with the modified fix (properly supporting optionality), there is no need for python_distribution to gain a resolve field.
There was a problem hiding this comment.
Also, the original fix could have broken existing users by requiring them to set a valid default resolve even though their code base wasn't using the default resolve if they had a python_distribution target.
Summary
Fix
FieldSetannotation handling so optional field annotations likeSomeField | Noneare recognized asFieldSetfields.This addresses a bug in Python dependency validation where
DependencyValidationFieldSet.resolve(annotated asPythonResolveField | None) was being skipped byFieldSet.fields(), causing dependency validation to lose the depender’s resolve and potentially fall back to global interpreter constraints.The issue was observed in the
shoalsoft-pants-opentelemetry-pluginwhen trying to upgrade it to support Pants v2.32 & Python 3.14. With[python].default_to_resolve_interpreter_constraintsset totrue, unspecifiedinterpreter_constraintsonparametrized targets were defaulting to the global ICs and not the resolve-specific ICs (set via[python].resolves_to_interpreter_constraints) as expected, leading to errors like the following:Root cause and solution
FieldSet.fields()only collected annotations that were direct subclasses ofField:resolve: PythonResolveField-> recognizedresolve: PythonResolveField | None-> skippedAs a result,
DependencyValidationFieldSet.create(tgt)populatedresolve=Noneeven when the target had a resolve.Solution
Update
FieldSet.fields()to unwrap union annotations and extract the underlying Field type when the annotation is a union containing exactly one Field subclass (e.g.Field|None/Optional[Field]).Testing
This PR includes a unit test demonstrating the issue and which passes with the fix.
AI Usage
Codex was used to diagnose the issue, write the reproduction test, and then write the solution.