Targets act as aliases for their files#10511
Conversation
a5f37ae to
1722416
Compare
74006fd to
96fff4d
Compare
|
A few tests are still failing, but this should finally be reviewable. |
|
Failing tests / open issues:
|
96fff4d to
4e4bb55
Compare
Eric-Arellano
left a comment
There was a problem hiding this comment.
Exciting! Waiting to approve to see what happens with deleted files.
As discussed over DM, list and filedeps need two tiny changes to use UnexpandedTargets instead of Targets.
| elif filedeps_subsystem.globs: | ||
| targets = await Get(UnexpandedTargets, Addresses, addresses) |
There was a problem hiding this comment.
./pants filedeps --transitive --globs won't behave how I think it should.
Without --transitive, we get this:
▶ ./pants filedeps --globs src/python/pants/util
src/python/pants/util/*.py
src/python/pants/util/BUILD
With --transitive, note that *.py no longer shows up:
▶ ./pants filedeps --globs --transitive src/python/pants/util
3rdparty/python/BUILD
3rdparty/python/requirements.txt
src/python/pants/BUILD
src/python/pants/__init__.py
src/python/pants/util/BUILD
src/python/pants/util/__init__.py
src/python/pants/util/collections.py
src/python/pants/util/contextutil.py
src/python/pants/util/dirutil.py
src/python/pants/util/enums.py
src/python/pants/util/eval.py
src/python/pants/util/filtering.py
src/python/pants/util/frozendict.py
src/python/pants/util/logging.py
src/python/pants/util/memo.py
src/python/pants/util/meta.py
src/python/pants/util/objects.py
src/python/pants/util/ordered_set.py
src/python/pants/util/osutil.py
src/python/pants/util/rwbuf.py
src/python/pants/util/socket.py
src/python/pants/util/strutil.py
Ideally, we would have a type of UnexpandedTransitiveTargets for this. I doubt it's worth that complexity, though.
There was a problem hiding this comment.
One thing that I think we should consider is removing the --globs flag, and/or moving it to another goal that dumps any field from a target.
There was a problem hiding this comment.
There's a long-standing issue to "peek" at the target definition #4861. This might especially be helpful in the age of generated subtargets.
There was a problem hiding this comment.
Maybe... it feels like there is a possible symmetry with filter, where anything we can render we can filter on, and vice-versa?
| address_to_dependees = defaultdict(set) | ||
| for tgt, dependencies in zip(all_explicit_targets, dependencies_per_target): | ||
| for dependency in dependencies: | ||
| # TODO(#10354): teach dependees how to work with generated subtargets. |
There was a problem hiding this comment.
As discussed over DM, I think this experience is still a little finicky. In our repo, ./pants dependees src/python/pants/util:util is empty. I would expect it to return all targets that use any of the files belonging to :util.
If I add an explicit dep on util:util, though, then that will show up.
I think this is likely fine, but unexpected.
There was a problem hiding this comment.
I view the changes to ./pants dependencies and ./pants dependees (ie, that both files and base targets are considered to be nodes in the graph) as a critical and intentional part of this change. I agree that it is unexpected, but I think that that is part of the reason why it was important to make this change before 2.0. Thanks for updating the description correspondingly.
src/python/pants/backend/python/dependency_inference/module_mapper.py
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
| elif filedeps_subsystem.globs: | ||
| targets = await Get(UnexpandedTargets, Addresses, addresses) |
There was a problem hiding this comment.
There's a long-standing issue to "peek" at the target definition #4861. This might especially be helpful in the age of generated subtargets.
b40f921 to
56371ea
Compare
…o the subtargets: find_owners uses matching targets. [ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
…subtargets 1) as dependencies for the base target, 2) as replacements for the base target during expansion. [ci skip-rust] [ci skip-build-wheels]
…s, to avoid cycles with codegen. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
[ci skip-rust] [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
cf1a8fb to
cb6620d
Compare
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Eric-Arellano
left a comment
There was a problem hiding this comment.
Excellent work! I'm really glad how this turned out, both for the end-user experience and for the actual code changes. I appreciate all your hard work on this :)
…ference is unused (#10582) ### Problem As of #10511, we now use file addresses / generated subtargets everywhere, except for the graph introspection rules. By default, generated subtargets do not depend on the sibling files in their base target, i.e. the other files in the original `sources` field. Those dependencies will only be included if dependency inference added it, or if the user explicitly added something like `./sibling.txt` to the base target's `dependencies` field. We generally do not want to automatically depend on sibling files. This would mean that file addresses (and file dependencies) do not actually have any benefit, because `./pants dependencies $file` would resolve the same results as `./pants dependencies $sibling`, i.e. we would not get finer-grained caching. Further, the conceptual model of "targets as metadata applied to files"—rather than "a collection of files with metadata"—would imply that we should not depend on sibling files automatically. However, for users who are not using dep inference and/or explicit file deps, many things will break if we don't automatically depend on sibling files. In Pants 1.x, it was a safe assumption that you could access any file in the target's `sources` field without having to add a dependency. It's majorly disruptive to break this assumption, and in effect, they would need to use dependency inference to be ergonomic. ### Solution Unless `--python-infer-imports` is True, and the target in question has `PythonSources`, then we will automatically add dependencies on all sibling files for file addresses. This means that every non-Python target (e.g. `files`, `bash_library`) will depend on its siblings, regardless of using dependency inference. [ci skip-rust] [ci skip-build-wheels]
…10619) In #10603, we moved `conftest.py` from `python_tests` to `python_library`. This is because Pants now runs file-at-a-time for Pytest thanks to the model change in #10511. This causes `conftest.py` to error because it doesn't have any test files. We decided in #10603 that `conftest.py` should be under `python_library` because it is not actual tests. For example, test util code goes under a `python_library` target already. However, `python_library` owning a `conftest.py` by default ended up being problematic, as it caused test code to be mixed with production code, which is generally not desired. See #10613 for an approach to fix this. Instead of adding a `conftest` target, this PR instead moves back `conftest.py` to `python_tests` and special cases `pytest_runner.py` to skip `conftest.py`. Even though this is less correct, it's simpler for users and it avoids making 1.x users having to change a bunch of things. Conceptually, `conftest.py` can be seen as "config" for tests, rather than traditional "test utils" like you'd have in a `python_library`. [ci skip-rust]
Problem
After extensive discussion, we think that in Pants 2.0, the definition of a target in a BUILD file will be approximately: "some metadata applied to a set of files, which can be used as an alias for those files". This is a fundamental change from the previous definition, which was something like: "a target is a collection of files with metadata that must always be built together".
But as described in #10455 and #10423, when "sub" (aka "file") targets are used inconsistently, it becomes necessary to worry about both:
...whereas before file dependencies existed, only the former was necessary.
Solution
Given the new definition, Pants ever operating on "the target as defined in the BUILD file" as the atomic unit no longer makes sense. Instead, for consistency: files should always be the new atomic unit (with batching applied only where it is possible to do so safely and transparently).
To accomplish this without also making significant changes to the
TargetAPI, this change adds expansion logic to the creation ofTargetsinstances that replaces base targets with their corresponding file/sub targets. Additionally, base targets are adapted to always depend on their subtargets. This means that although all nodes in the build graph are stillTargetobjects, post-expansion inTargets, eachTargetobject will own either zero or one file.Graph introspection goals mostly continue to operate directly on the targets as defined in BUILD files, and so an
UnexpandedTargetstype is introduced to be used in cases where the BUILD definitions should be used verbatim. Other cases where these are used include:Owners: a deleted file is matched againstUnexpandedTargetsto ensure that we detect what the owner "would have been" if the file had not been deleted.filedepsgoal, when the--globsoption is used: it's likely that this case should migrate to a goal that is capable of introspecting the fields of allTargetobjects, possibly in a way that is symmetrical to thefiltergoal.Unfortunately, this differentiation is not typesafe: both
TargetsandUnexpandedTargetscontainTargetobjects, but with different guarantees. We will likely want to follow up in the future to adapt theTargetAPI to make this case typesafe.Result
From a user perspective, the most visible impact of this change will be that a goal like
./pants testwill now always operate per-file. This will mean that it is critically important for us to optimize per-test-run overhead before 2.0, and to work with users to determine whether implementing dynamic batching strategies is necessary.dependenciesanddependeeswill now result in both file and target addresses showing up, unless the target has no source files. This will happen even if the user is not using explicit file deps and dep inference.Fixes #10354, and fixes #10455.