-
-
Notifications
You must be signed in to change notification settings - Fork 689
fix issue with optional fields in dependency validator #23121
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
Open
tdyas
wants to merge
13
commits into
main
Choose a base branch
from
tdyas/fix-dep-validator-issue
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+279
−15
Open
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
ad91248
fix issue with optional fields in dependency validator
tdyas 7ba7d64
pants fix
tdyas c7299df
typing fix
tdyas a8b8903
update release notes
tdyas 6eb6409
fix incorrect python-default resolve
tdyas 0e36570
more export python_distribution fixes
tdyas e87392e
Merge branch 'main' into tdyas/fix-dep-validator-issue
tdyas 25ed647
Merge branch 'main' into tdyas/fix-dep-validator-issue
tdyas d8e55ba
none_on_absence_fields
tdyas c44e094
fmt and remove export test work-arounds
tdyas 24d3ccb
runtime check for "correct" field set types
tdyas 43d6af9
Merge branch 'main' into tdyas/fix-dep-validator-issue
tdyas 67b0d2e
Merge branch 'main' into tdyas/fix-dep-validator-issue
tdyas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Fixing this bug resulting in these uses of
python_distributiondefaulting topython-defaultresolve even though onlyaandbresolves are defined.python_distributiondoes not appear to haveresolvefield, is that intentional?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.
cc @benjyw re the above question
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.
Presumably it is intentional, since the
python_distributionis just some metadata on how to build a wheel, and the resolve doesn't enter into that?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.
Then it seems incorrect for the PR's solution to even apply the notion of a resolve to
python_distributionthen. Do you concur?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.
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 | NoneinDependencyValidationFieldSetor that optionality is incorrect andFieldSets should never have optional types?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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dug in more on the issue today. The issue here is that when an optional field in a
FieldSetis missing from a target, then the field is instantiated with its default value instead of being set toNoneas a result ofTarget.get.So
DependencyValidationFieldSetis trying to use a semantic (i.e., returnNonefor missing fields) whichFieldSetdoes not support at all. And that is why the resolve-specific ICs are not applied atpants/src/python/pants/backend/python/target_types_rules.py
Lines 649 to 651 in 73d7a82
resolveonDependencyValidationFieldSetis alwaysNone.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the original fix, the code assumes the default resolve. With the modified fix in today's commits, the
FieldSetmachinery is able to returnNoneforPythonResolveField | Noneas a reader might have assumed to be the case. So with the modified fix (properly supporting optionality), there is no need forpython_distributionto gain aresolvefield.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.
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_distributiontarget.