pants: disambiguate test file dependencies #5833
Merged
Conversation
This file contains 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
cognifloyd
requested review from
winem,
arm4b,
nzlosh,
rush-skills,
amanda11 and
a team
pull-request-size
bot
added
the
size/M
We do large sets of files because maintaining manual per-file mapping of dependencies would be a lot of pointless work. Hopefully a future version of pants will have features that make disambiguation more precise, integrating better with dependency inferrence. When that happens we can revert this commit in favor of telling pants how to infer the ambiguous deps instead of hard-coding the deps for a large set of files that might not need the dep.
cognifloyd
force-pushed
the
pants-disambiguate-deps
branch
from
f5e1908
to
adf89a2
Compare
|
Note: This PR needs to be merged before I can add pylint. Otherwise, pants will run into these ambiguous deps whenever it runs pylint, and pylint will subsequently fail when pants doesn't add all of the required files/deps to the sandbox it runs pylint in. |
|
For reference, here are the warnings this resolves: |
Eric-Arellano
approved these changes
Dec 4, 2022
There was a problem hiding this comment.
However, sometimes that inference gets confused. For example, in several places our test files import tests.base, but we have multiple **/tests/base.py files, so pants doesn't know which one we're talking about, even though python itself understands exactly what we're talking about.
FYI this is because Pants lets you import from anywhere in your monorepo, which is a key benefit of monorepo. Whereas often when running Python directly, you only run in a subproject of your repo. So, Pants creates a global "module mapping" of e.g. the module to tests.base to ("dir1/tests/base.py, "dir2/tests/base.py")`
*In Pants 2.16, we added visibility support, where you can ban certain files from importing others.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Background
This is another part of introducing pants, as discussed in various TSC meetings.
Related PRs can be found in:
Overview of this PR
Pants uses dependency inference, which the docs describe as:
However, sometimes that inference gets confused. For example, in several places our test files import
tests.base, but we have multiple**/tests/base.pyfiles, so pants doesn't know which one we're talking about, even though python itself understands exactly what we're talking about.So, this PR resolves that ambiguity by explicitly defining dependencies in the BUILD files. In a few cases, we can add that dependency to the one file that needs it. But in other cases, lots of test files have the "ambiguous" import on
tests.baseortests.unit.base. Adding the dep to each of those files would be painful to maintain, so I added it to all of the test files in the affected directories.Aside: In each case, I left a comment about why the dep was "ambiguous". I hope that a future version of pants will give us some knobs to tell the inference system how to resolve dependencies it thinks are ambiguous instead of adding it explicitly to more files than necessary. Including "ambiguous" in the BUILD file comment will make it easier to grep and figure out where we can adjust things once pants includes a feature like that.
Relevant Pants documentation