Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Teach dependees how to work with generated subtargets #10354

Closed
Eric-Arellano opened this issue Jul 14, 2020 · 4 comments · Fixed by #10511
Closed

Teach dependees how to work with generated subtargets #10354

Eric-Arellano opened this issue Jul 14, 2020 · 4 comments · Fixed by #10511
Assignees
@stuhood
Milestone
2.0.0.alpha0

Comments

@Eric-Arellano
Copy link
Contributor

Right now, ./pants dependees will always return original addresses, rather than generated subtargets:

▶ ./pants dependees src/python/pants/util
build-support/migration-support:tests
pants-plugins/src/python/internal_backend/utilities:utilities
src/python/pants/auth:auth
src/python/pants/backend/native/config:config
src/python/pants/backend/native/subsystems:subsystems
src/python/pants/backend/native/subsystems/utils:utils
src/python/pants/backend/pants_info:pants_info
src/python/pants/backend/project_info:project_info
src/python/pants/backend/python:python
src/python/pants/backend/python/dependency_inference:dependency_inference
src/python/pants/backend/python/dependency_inference:tests
src/python/pants/backend/python/lint/bandit:bandit
src/python/pants/backend/python/lint/black:black
src/python/pants/backend/python/lint/docformatter:docformatter
src/python/pants/backend/python/lint/flake8:flake8
src/python/pants/backend/python/lint/isort:isort
src/python/pants/backend/python/lint/pylint:pylint
src/python/pants/backend/python/rules:integration
src/python/pants/backend/python/rules:rules
src/python/pants/backend/python/rules:tests
src/python/pants/backend/python/subsystems:subsystems
src/python/pants/backend/python/typecheck/mypy:mypy

In contrast, dependencies understands generated subtargets:

▶ ./pants dependencies src/python/pants/backend/python/lint/isort:integration
src/python/pants/backend/python/lint/isort/rules.py
src/python/pants/backend/python/target_types.py
src/python/pants/base/specs.py
src/python/pants/core/goals/fmt.py
src/python/pants/core/goals/lint.py
src/python/pants/core/util_rules/determine_source_files.py
src/python/pants/engine/addresses.py
src/python/pants/engine/fs.py
src/python/pants/engine/rules.py
src/python/pants/engine/selectors.py
src/python/pants/engine/target.py
src/python/pants/testutil/external_tool_test_base.py
src/python/pants/testutil/option/util.py

It would be helpful if dependees also understood generated subtargets. The output would be more consistent with the rest of Pants and it would result in finer grained caching when using --changed-dependees.

The tricky part is that the goal must continue to understand traditional targets that are not generated.
@Eric-Arellano Eric-Arellano mentioned this issue Jul 14, 2020
Merged
@stuhood
Copy link
Member

stuhood commented Jul 14, 2020 •
edited
Loading

One possible internal factoring would be to have targets always depend on their subtargets, rather than owning any files of their own. That might work more naturally with a merged ./pants dependencies that supported rendering both files and targets (each optionally) both directly and transitively. And likewise, it could give ./pants dependees natural support for "file dependees".

Eric-Arellano added a commit that referenced this issue Jul 14, 2020
@Eric-Arellano
Fix generated subtargets not working with dependees (#10355)
248a903 
Because it's tricky to get right and also not critical for Pants to work, we, for now, punt on `dependees` using generated subtargets and instead convert back to the original base target. #10354 tracks doing the better thing.

[ci skip-rust-tests]
Eric-Arellano added a commit to Eric-Arellano/pants that referenced this issue Jul 15, 2020
@Eric-Arellano
Fix generated subtargets not working with dependees (pantsbuild#10355)
c7beceb 
Because it's tricky to get right and also not critical for Pants to work, we, for now, punt on `dependees` using generated subtargets and instead convert back to the original base target. pantsbuild#10354 tracks doing the better thing.

[ci skip-rust-tests]
@stuhood stuhood mentioned this issue Jul 15, 2020
Closed
This was referenced Jul 17, 2020
Closed
Closed
@Eric-Arellano
Copy link
Contributor Author

Generated subtargets are being removed per #10423 in favor of #10423.

@Eric-Arellano
Copy link
Contributor Author

We're trying to lean in on generated subtargets. Fixing this is no longer an enhancement - it's key to removing the concerns in #10423, especially the UX concerns of sometimes having files and sometimes targets.

@Eric-Arellano Eric-Arellano reopened this Jul 27, 2020
@Eric-Arellano Eric-Arellano added this to the 2.0.0.alpha0 milestone Jul 27, 2020
@stuhood
Copy link
Member

stuhood commented Jul 27, 2020

Partial dupe of #10455 I think, but will keep both open and linked to ensure that both are resolved by the changes.

stuhood added a commit to stuhood/pants that referenced this issue Jul 30, 2020
@stuhood
Fix pantsbuild#10354.
5b0ee72 
[ci skip-rust]

[ci skip-build-wheels]
@stuhood stuhood mentioned this issue Jul 31, 2020
Closed
stuhood added a commit to stuhood/pants that referenced this issue Jul 31, 2020
@stuhood
Fix pantsbuild#10354.
436e904 
[ci skip-rust]

[ci skip-build-wheels]
stuhood added a commit to stuhood/pants that referenced this issue Aug 4, 2020
@stuhood
Fix pantsbuild#10354.
da280c4 
[ci skip-rust]

[ci skip-build-wheels]
stuhood added a commit to stuhood/pants that referenced this issue Aug 4, 2020
@stuhood
Fix pantsbuild#10354.
ec9d3be 
[ci skip-rust]

[ci skip-build-wheels]
stuhood added a commit to stuhood/pants that referenced this issue Aug 4, 2020
@stuhood
Fix pantsbuild#10354.
4e5d491 
[ci skip-rust]

[ci skip-build-wheels]
stuhood added a commit to stuhood/pants that referenced this issue Aug 4, 2020
@stuhood
Fix pantsbuild#10354.
988b93c 
[ci skip-rust]

[ci skip-build-wheels]
stuhood added a commit to stuhood/pants that referenced this issue Aug 5, 2020
@stuhood
Fix pantsbuild#10354.
931bee5 
[ci skip-rust]

[ci skip-build-wheels]
@stuhood stuhood mentioned this issue Aug 5, 2020
Merged
stuhood added a commit to stuhood/pants that referenced this issue Aug 5, 2020
@stuhood
Fix pantsbuild#10354.
0ca4be9 
[ci skip-rust]

[ci skip-build-wheels]
stuhood added a commit to stuhood/pants that referenced this issue Aug 6, 2020
@stuhood
Fix pantsbuild#10354.
0d5d5af 
[ci skip-rust]

[ci skip-build-wheels]
stuhood added a commit that referenced this issue Aug 6, 2020
@stuhood
Targets act as aliases for their files (#10511)
015a1e6 
### 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:
1. whether a target builds as an atomic unit
2. 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:
1. 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.
2. 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stuhood stuhood

Labels
None yet
Projects
None yet
Milestone
2.0.0.alpha0
Development

Successfully merging a pull request may close this issue.

Targets act as aliases for their files stuhood/pants
2 participants
@stuhood @Eric-Arellano