Targets act as aliases for their files #10511

stuhood · 2020-07-30T05:24:59Z

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:

whether a target builds as an atomic unit
whether each file in the target builds as an atomic unit

...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 Target API, this change adds expansion logic to the creation of Targets instances 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 still Target objects, post-expansion in Targets, each Target object 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 UnexpandedTargets type is introduced to be used in cases where the BUILD definitions should be used verbatim. Other cases where these are used include:

deleted files in Owners: a deleted file is matched against UnexpandedTargets to ensure that we detect what the owner "would have been" if the file had not been deleted.
in the filedeps goal, when the --globs option is used: it's likely that this case should migrate to a goal that is capable of introspecting the fields of all Target objects, possibly in a way that is symmetrical to the filter goal.

Unfortunately, this differentiation is not typesafe: both Targets and UnexpandedTargets contain Target objects, but with different guarantees. We will likely want to follow up in the future to adapt the Target API to make this case typesafe.

Result

From a user perspective, the most visible impact of this change will be that a goal like ./pants test will 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.

dependencies and dependees will 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.

stuhood · 2020-08-05T00:35:19Z

A few tests are still failing, but this should finally be reviewable.

stuhood · 2020-08-05T17:02:46Z

Failing tests / open issues:

rule graph error:
- ~~src/python/pants/backend/python/rules/python_sources_test.py:tests~~
- ~~src/python/pants/init/load_backends_integration_test.py:integration~~
expects a target with globs to exist:
- ~~src/python/pants/backend/project_info/filedeps_test.py:tests~~
operating on a single target rather than a group:
- ~~src/python/pants/backend/python/lint/pylint/rules_integration_test.py:integration~~
- ~~src/python/pants/backend/python/typecheck/mypy/rules_integration_test.py:integration~~
other fixes:
- ~~src/python/pants/build_graph/address_test.py:tests~~
- src/python/pants/backend/codegen/protobuf/python/rules_integration_test.py:tests
- src/python/pants/backend/python/rules/run_setup_py_test.py:tests
- src/python/pants/engine/internals/graph_test.py:tests
- ~~src/python/pants/engine/target_test.py:tests~~
- ~~tests/python/pants_test/integration/changed_integration_test.py:changed_integration~~

Eric-Arellano

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.

Eric-Arellano · 2020-08-05T23:19:18Z

src/python/pants/backend/project_info/filedeps.py

+    elif filedeps_subsystem.globs:
+        targets = await Get(UnexpandedTargets, Addresses, addresses)


./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.

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's a long-standing issue to "peek" at the target definition #4861. This might especially be helpful in the age of generated subtargets.

Maybe... it feels like there is a possible symmetry with filter, where anything we can render we can filter on, and vice-versa?

Eric-Arellano · 2020-08-05T23:20:58Z

src/python/pants/backend/project_info/dependees.py

@@ -35,8 +35,6 @@ async def map_addresses_to_dependees() -> AddressToDependees:
    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.


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.

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

src/python/pants/backend/python/dependency_inference/rules_test.py

src/python/pants/core/target_types.py

src/python/pants/engine/internals/graph.py

src/python/pants/engine/target.py

src/python/pants/engine/internals/graph.py

src/python/pants/backend/project_info/dependees.py

Eric-Arellano · 2020-08-06T00:34:53Z

src/python/pants/backend/project_info/filedeps.py

+    elif filedeps_subsystem.globs:
+        targets = await Get(UnexpandedTargets, Addresses, addresses)


There's a long-standing issue to "peek" at the target definition #4861. This might especially be helpful in the age of generated subtargets.

src/python/pants/engine/target_test.py

src/python/pants/engine/internals/graph.py

…o the subtargets: find_owners uses matching targets. [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]

Eric-Arellano

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 :)

coveralls · 2020-08-06T23:20:51Z

Coverage remained the same at 0.0% when pulling b5cc97d on stuhood:stuhood/base-target-depends-on-subtargets into de0f887 on pantsbuild:master.

…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]

…`fmt` (#10630) Thanks to #10511, targets are no longer an atomic unit Pants runs on. Instead, files are the atomic unit. This option rename reflects that model change. [ci skip-rust] [ci skip-build-wheels]

stuhood force-pushed the stuhood/base-target-depends-on-subtargets branch from a5f37ae to 1722416 Compare July 30, 2020 17:59

stuhood mentioned this pull request Jul 31, 2020

Always use generated subtargets #10455

Closed

stuhood force-pushed the stuhood/base-target-depends-on-subtargets branch 5 times, most recently from 74006fd to 96fff4d Compare August 5, 2020 00:03

stuhood requested review from Eric-Arellano, jsirois and benjyw August 5, 2020 00:33

stuhood changed the title ~~Targets depend on their subtargets~~ Targets depend on their files Aug 5, 2020

stuhood changed the title ~~Targets depend on their files~~ Targets act as aliases for their files Aug 5, 2020

stuhood marked this pull request as ready for review August 5, 2020 00:34

benjyw approved these changes Aug 5, 2020

View reviewed changes

stuhood force-pushed the stuhood/base-target-depends-on-subtargets branch from 96fff4d to 4e4bb55 Compare August 5, 2020 21:18

Eric-Arellano reviewed Aug 5, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

Eric-Arellano reviewed Aug 6, 2020

View reviewed changes

stuhood force-pushed the stuhood/base-target-depends-on-subtargets branch from b40f921 to 56371ea Compare August 6, 2020 19:11

stuhood added 9 commits August 6, 2020 14:07

resolve_target always generates subtargets, and moves all sources t…

ac9ecb3

…o the subtargets: find_owners uses matching targets. [ci skip-rust] [ci skip-build-wheels]

Implement alias expansion at the Target level.

d85e278

[ci skip-rust] [ci skip-build-wheels]

Fix pantsbuild#10354.

0d5d5af

[ci skip-rust] [ci skip-build-wheels]

Test fixes.

d879b2a

[ci skip-rust] [ci skip-build-wheels]

Leave base targets as their underlying type in the graph, and inject …

5f1a9f8

…subtargets 1) as dependencies for the base target, 2) as replacements for the base target during expansion. [ci skip-rust] [ci skip-build-wheels]

Use direct SourcesField->Snapshot expansion to create hydrated target…

c28431d

…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]

Fix tests, and adjust use of expanded/Unexpanded Targets in a few goals.

64215c4

# 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]

Test fixes and review feedback.

a2b05bc

[ci skip-rust] [ci skip-build-wheels]

Apply Eric's suggested changes.

cb6620d

# 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]

stuhood force-pushed the stuhood/base-target-depends-on-subtargets branch from cf1a8fb to cb6620d Compare August 6, 2020 21:07

stuhood added 2 commits August 6, 2020 15:03

Skip setup_py test due to pantsbuild#10564.

88689ec

# 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]

Apply more of Eric's suggested changes.

b5cc97d

# 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 approved these changes Aug 6, 2020

View reviewed changes

stuhood merged commit 015a1e6 into pantsbuild:master Aug 6, 2020

stuhood deleted the stuhood/base-target-depends-on-subtargets branch August 6, 2020 23:20

Eric-Arellano mentioned this pull request Aug 10, 2020

Automatically inject dependencies on sibling files when dependency inference is unused #10582

Merged

Eric-Arellano mentioned this pull request Aug 15, 2020

Revert #10603 so that conftest.py belongs to python_tests again #10619

Merged

Eric-Arellano mentioned this pull request Aug 16, 2020

Rename --per-target-caching to --per-file-caching for lint and fmt #10630

Merged

This was referenced Sep 14, 2021

[internal] Refactor "file targets" / "subtargets" to use more generic Target Generation #12860

Merged

[wip] Stop removing target generators from project introspection like ./pants list :: #12952

Closed

This was referenced Oct 2, 2021

Proposal: get rid of UnexpandedTargets #13086

Closed

Target generators should not depend on generated targets? #13118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Targets act as aliases for their files #10511

Targets act as aliases for their files #10511

stuhood commented Jul 30, 2020 •

edited

Loading

stuhood commented Aug 5, 2020

stuhood commented Aug 5, 2020 •

edited

Loading

Eric-Arellano left a comment

Eric-Arellano Aug 5, 2020

stuhood Aug 5, 2020

Eric-Arellano Aug 6, 2020

stuhood Aug 6, 2020

Eric-Arellano Aug 5, 2020

stuhood Aug 6, 2020

This comment has been minimized.

Eric-Arellano Aug 6, 2020

Eric-Arellano left a comment

coveralls commented Aug 6, 2020

		elif filedeps_subsystem.globs:
		targets = await Get(UnexpandedTargets, Addresses, addresses)

Targets act as aliases for their files #10511

Targets act as aliases for their files #10511

Conversation

stuhood commented Jul 30, 2020 • edited Loading

Problem

Solution

Result

stuhood commented Aug 5, 2020

stuhood commented Aug 5, 2020 • edited Loading

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Aug 5, 2020

Choose a reason for hiding this comment

stuhood Aug 5, 2020

Choose a reason for hiding this comment

Eric-Arellano Aug 6, 2020

Choose a reason for hiding this comment

stuhood Aug 6, 2020

Choose a reason for hiding this comment

Eric-Arellano Aug 5, 2020

Choose a reason for hiding this comment

stuhood Aug 6, 2020

Choose a reason for hiding this comment

This comment has been minimized.

Eric-Arellano Aug 6, 2020

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

coveralls commented Aug 6, 2020

stuhood commented Jul 30, 2020 •

edited

Loading

stuhood commented Aug 5, 2020 •

edited

Loading