Add a --query option for unifying target selection #7350

cosmicexplorer · 2019-03-10T04:49:23Z

Problem

It would be neat to be able to do away with splitting target selection across multiple options and tasks (--owner-of, --changes-since, dependencies, filter, ...), and it would also be great to avoid having to perform multiple pants invocations to extract the final desired target list when running these target selection commands in our internal CI. #7346 was made to discuss one way this could end up looking like.

Please make sure to see #6501 for the impetus and prior discussion of unifying target selection.

Solution

Create a v2 ruleset for converting ChangedRequest into an OwnersRequest to allow for a single engine invocation instead of multiple session.product_request()s.
Create query.py with QueryComponent objects that have rulesets resolving them to BuildFileAddresses and know how to parse themselves from shlexed strings (the parsing method is subject to change, see ./pants --query for unifying target selection mechanisms to improve performance #7346 (comment)).
Add a global --query option and parse it into QueryComponents.
Create owner-of, changes-since, and changes-from-diffspec query functions to mimic the behavior of --owner-of and --changes-* options.

Result

Instead of running ./pants --changes-since=HEAD list, the user can run ./pants --query='changes-since HEAD' list, similarly for owner-of. The first step of the plan being discussed in #7346 is laid out, laying the groundwork for composing multiple queries in a followup PR.

illicitonion

Looks exciting :) Most important question here is around @rule caching. I also have some composition questions, but I'll take those to #7346 :)

src/python/pants/engine/legacy/query.py

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

src/python/pants/scm/subsystems/changed.py

tests/python/pants_test/engine/legacy/test_query_integration.py

stuhood · 2019-03-14T01:55:18Z

Thanks! Will try to take a look before the weekend.

stuhood

Neat! I don't have any objections to this, although I think that actually implementing a form of pipelining will be necessary to make it truly useful. Would recommend getting the second PR "mostly ready" before landing this one.

src/python/pants/scm/subsystems/changed.py

src/python/pants/option/global_options.py

cosmicexplorer · 2019-04-05T03:51:23Z

Would recommend getting the second PR "mostly ready" before landing this one.

@stuhood like this? #7356

stuhood · 2019-04-05T03:59:42Z

Would recommend getting the second PR "mostly ready" before landing this one.

@stuhood like this? #7356

Heh, yes, roughly. Out of draft and with no showstoppers would be good.

cosmicexplorer · 2019-04-05T04:00:13Z

Sounds like a plan!

@olafurpg

### Problem We would like to be able to provide binary versions of all dependencies of specified root targets to an IDE with `export-classpath`, without providing binary versions of the target roots themselves (because the IDE will be indexing those itself, as source files). `--query` from #7350 might help here, but until that's developed a little further, it would be great to have this functionality now. @olafurpg requested this change to help integrate pants with metals. ### Solution - Add `--transitive-only` option to the `export-classpath` task which avoids publishing jars for target roots, and instead only pulls in their dependencies!

stuhood

Thanks: this continues to be a really interesting patch, particularly now that the new Target API has begun to take form (cc @Eric-Arellano). Adjusting this to consume that API rather than target adaptors or the legacy target API is probably the last blocker from my perspective.

src/python/pants/engine/query.py

src/python/pants/init/specs_calculator.py

src/python/pants/option/global_options.py

src/python/pants/engine/query.py

src/python/pants/init/extension_loader.py

Eric-Arellano · 2020-07-21T06:21:00Z

src/python/pants/init/specs_calculator.py

+                raise InvalidSpecConstraint(
+                    # TODO: centralize the error messaging for when an SCM is required, and describe what SCMs
+                    # are supported!
+                    "{} are not available without a recognized SCM (usually git).".format(entity)


Maybe related: the sole SCM that Pants supports right now is Git. I think we'll have better error messages if we reflect that reality and solely mention Git. Users don't need to see the word SCM.

We can keep the actual class SCM of course. And if/when we add a second SCM, we can update these error messages to reflect that new reality. I imagine we will then search for all uses of get_scm() + "git", so we'll find this.

I don't agree with "users don't need to see the word SCM" -- if a user tries to use pants on their mercurial repo and it fails, their next response is not likely to be to open an issue against the pants repo and expect help if we assume git.

I have modified the error message to specify that we currently only support git.

Eric-Arellano · 2020-07-21T06:24:22Z

src/python/pants/init/specs_calculator.py

+        if exprs:
+            # TODO: this should only be necessary for the `changed-since`/etc queries! This can be done by
+            # returning a dummy ScmWrapper if no `changed-*` queries are used!
+            scm = scm("The --query option")
+            # TODO(#7346): deprecate --owner-of and --changed-* in favor of --query versions, allow
+            # pipelining of successive query expressions with the command-line target specs as the initial
+            # input!
+            if len(exprs) > 1:
+                raise ValueError(
+                    "Only one --query argument is currently supported! Received: {}.".format(exprs)
+                )
+
+            scm_wrapper = UncachedScmWrapper.create(scm)
+            (expr_addresses,) = session.product_request(
+                QueryAddresses, [Params(scm_wrapper, exprs[0])]
+            )
+            logger.debug("expr addresses: %s", expr_addresses)
+            dependencies = tuple(
+                SingleAddress(a.spec_path, a.target_name) for a in expr_addresses.addresses
+            )
+            return Specs(
+                address_specs=AddressSpecs(
+                    dependencies=dependencies, exclude_patterns=exclude_patterns, tags=tags
+                ),
+                filesystem_specs=FilesystemSpecs(filesystem_specs),
+            )


You'd want to move this up above all this SCM stuff, and after if not changed_options.provided:.

This file was refactored recently, and now it follows the high level flow of:

if not changed_options.provided: # assume it's direct file/target args, and return those directly # all the SCM logic. Note that there's no else clause because the above `if` early returned.

I'm suggesting putting if exprs: in between those two parts.

I refactored this file in between your comments -- please let me know if you like the result.

src/python/pants/scm/subsystems/changed.py

coveralls · 2020-07-21T06:57:15Z

Coverage remained the same at 0.0% when pulling cc87ef4 on cosmicexplorer:query-option-for-unifying-target-selection into 45ca5b4 on pantsbuild:master.

cosmicexplorer · 2020-07-21T08:37:33Z

src/rust/engine/rule_graph/src/lib.rs

+        msgs.len(),
+        msgs.join("\n\n  ")
+      ))
+    }
  }


Using @_uncacheable_rule with changed_files() in this PR would fail with Exception: Rules with errors: 0 without this change.

Hm... fishy. Would you mind opening a ticket about that?

use the ast module to parse query expressions!! [ci skip-rust-tests] answer some review comments. per-file targets are breaking it now [ci skip-rust-tests] add do_not_generate_subtargets workaround for subtargets [ci skip-rust-tests] review feedback [ci skip-rust-tests] attempt to use @_uncacheable_rule [ci skip-rust-tests] fix @_uncacheable_rule fix error about git as RootRule add a lengthy TODO to fix query_rules() initialization [ci skip-rust-tests]

stuhood

Thanks! Although it remains to be seen what we will do with @_uncacheable_rule, AFAICT this should be correct.

stuhood · 2020-07-21T17:32:47Z

src/python/pants/init/specs_calculator.py

+            # TODO: allow returning @union types to avoid this double synchronous engine invocation!
+            exprs = session.product_request(
+                QueryComponentWrapper, [QueryParseInput(s) for s in query_expr_strings]
+            )
+            exprs = [ex.underlying for ex in exprs]


Alternatively, you might consider not using the engine for parsing? It looks like the parsing is a pure function, so it would likely be cheaper not to bounce in and out like this.

+1. Beyond being cheaper perf wise, it would likely be simpler code as pure functions are easier to work with and test than rules.

stuhood · 2020-07-21T17:33:44Z

src/python/pants/option/global_options.py

+            default=[],
+            metavar="<query-expr>",
+            help="A list of query expressions which process the input target specs in a "
+            "pipeline in order.",


This should probably include a link to a page (maybe a section of https://www.pantsbuild.org/docs/advanced-target-selection or https://www.pantsbuild.org/docs/project-introspection) on the docsite that explains how to use query.

I'd recommend this go into Advanced Target Selection. Maybe have a tooltip on Project Introspection that cross-references the other page.

You'll want to use https://www.pantsbuild.org/v2.0/docs/advanced-target-selection for now because this feature isn't being added to Pants 1.30.

stuhood · 2020-07-21T17:35:57Z

src/rust/engine/rule_graph/src/lib.rs

+        msgs.len(),
+        msgs.join("\n\n  ")
+      ))
+    }
  }


Hm... fishy. Would you mind opening a ticket about that?

stuhood

(Also, if the other reviewers are ready to land this, it would probably be reasonable to add a few tests. I can understand having held off while things were up in the air.)

illicitonion · 2020-07-21T21:47:11Z

I'm not likely to have enough time in the near future to give this particularly meaningful review, but it looks like my one raised. concern of broken caching behaviour has been resolved, so I'll remove myself as a reviewer :) Thanks for pushing forwards with this very exciting feature!

He said he's dismissing the review, only didn't click the appropriate button.

Eric-Arellano

Cool! Could you please update the PR description? It looks like it has some stale parts like --owner-of.

It'd be really helpful to expand the number of examples to demonstrate things like how you specify the dependees behavior of changed_since.

Eric-Arellano · 2020-07-21T17:41:18Z

src/python/pants/scm/subsystems/changed.py

+class ChangedFiles(Collection[str]):
+    pass
+
+
+@dataclass(frozen=True)
+class ChangedFilesRequest:
+    options: 'ChangedOptions'
+    git: Git


Nit: if you move this below ChangedOptions, you can remove the quotes, and the dataclass definitions will also be closer to the rule where they're directly used.

Eric-Arellano · 2020-07-22T01:09:51Z

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

@@ -221,7 +222,7 @@ async def find_owners(owners_request: OwnersRequest) -> Owners:
        if bfa.rel_path not in sources_set and not matching_files:
            continue
        deleted_files_matched = bool(set(matching_files) - all_source_files)
-        if deleted_files_matched:
+        if deleted_files_matched or owners_request.do_not_generate_subtargets:


Totally fine to do this for now. I've found generated subtargets unusually confusing to reason about, and I'm proposing in #10423 that we seriously consider removing the (mis)feature.

If we do stick with generated subtargets, do you have any thoughts on how they should integrate with --query? Generally, we've said that we want to use generated subtargets when there's enough precision to indicate that the user wants it, such as file arguments or --changed-since. The tricky part with --query from what I can tell is that you will sometimes have files via --changed-since, but sometimes have normal addresses. So possibly we should always use normal addresses?

Eric-Arellano · 2020-07-22T01:12:30Z

src/python/pants/engine/query.py

+@dataclass(frozen=True)
+class OwnerOf(QueryComponent):
+    files: Tuple[str]
+
+    function_name = 'owner_of'
+
+    @classmethod
+    def parse_from_args(cls, *args):
+        return cls(files=tuple([str(f) for f in args]))
+
+
+@rule
+async def owner_of_request(owner_of: OwnerOf) -> QueryAddresses:
+    request = OwnersRequest(sources=owner_of.files, do_not_generate_subtargets=True)
+    owners = await Get(Owners, OwnersRequest, request)
+    return QueryAddresses(owners)


--owner-of was replaced via file arguments. I'm wondering if we should still have this as a part of --query. Is the motivation so that you could pipe to other parts of the query, whereas file args wouldn't let you do that?

src/python/pants/init/extension_loader.py

Eric-Arellano · 2020-07-22T02:00:59Z

src/python/pants/init/specs_calculator.py

+        # Parse --query expressions into objects which can be resolved into BuildFileAddresses via
+        # v2 rules.


Stale - We now use Address and it's redundant to say "v2".

Eric-Arellano · 2020-07-22T02:07:28Z

src/python/pants/option/global_options.py

+            default=[],
+            metavar="<query-expr>",
+            help="A list of query expressions which process the input target specs in a "
+            "pipeline in order.",


I'd recommend this go into Advanced Target Selection. Maybe have a tooltip on Project Introspection that cross-references the other page.

You'll want to use https://www.pantsbuild.org/v2.0/docs/advanced-target-selection for now because this feature isn't being added to Pants 1.30.

Eric-Arellano · 2020-07-22T02:09:09Z

src/python/pants/scm/subsystems/changed.py

+@_uncacheable_rule
+def changed_files(request: ChangedFilesRequest) -> ChangedFiles:


This is pretty cool that it's now a rule!

In the future, I think your TODO is saying that we should refactor git to not use subprocess anymore, right? To confirm, it's safe for now to use subprocess because of @_uncacheable_rule?

Eric-Arellano · 2020-07-22T02:10:25Z

src/python/pants/scm/subsystems/changed.py

@@ -33,6 +35,7 @@ class DependeesOption(Enum):
 class ChangedRequest:


It's a little confusing how this is different from ChangedFilesRequest. Would you mind please renaming this to ChangedAddresessRequest?

Eric-Arellano · 2020-07-22T02:11:42Z

src/python/pants/engine/query.py

+    else:
+        raise QueryParseError(
+            f'Query function with name {name} not found (in expr {s})! '
+            f'The known functions are: {known}.')


Eric-Arellano · 2020-07-22T02:13:26Z

src/python/pants/engine/query.py

+    function_name = 'changes_for_diffspec'
+
+    @classmethod
+    def parse_from_args(cls, diffspec, dependees=DependeesOption.NONE):


I'm a little confused how the user would specify dependees. Could you please add an example to the PR description?

Eric-Arellano · 2021-07-19T19:02:17Z

Closing as stale, which we're doing for all changes that haven't been touched in 1+ years to simplify project management.

This would still be a really neat feature, though. Do feel free to reopen. Thank you for showing what --query could look like for Pants!

cosmicexplorer requested review from stuhood, jsirois, illicitonion and Eric-Arellano March 10, 2019 04:49

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch from 4c5db44 to 02890ce Compare March 11, 2019 01:51

cosmicexplorer mentioned this pull request Mar 11, 2019

[WIP] --query pipelining #7356

Closed

illicitonion previously requested changes Mar 11, 2019

View reviewed changes

cosmicexplorer mentioned this pull request Mar 12, 2019

add an enum for IncludeDependees for ChangedRequest and OwnersRequest #7364

Closed

stuhood mentioned this pull request Apr 5, 2019

Dirty the dependents of an uncacheable Node #6598

Closed

stuhood reviewed Apr 5, 2019

View reviewed changes

src/python/pants/scm/subsystems/changed.py Outdated Show resolved Hide resolved

src/python/pants/scm/subsystems/changed.py Outdated Show resolved Hide resolved

src/python/pants/option/global_options.py Outdated Show resolved Hide resolved

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch 2 times, most recently from acfba59 to 81efd62 Compare May 20, 2019 05:30

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch from 81efd62 to 811be9a Compare August 6, 2019 02:58

stuhood mentioned this pull request Aug 15, 2019

Listroots to v2 #8045

Closed

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch 4 times, most recently from 06cd7f2 to 073db8e Compare October 28, 2019 16:48

This was referenced Nov 8, 2019

Non-target specs #8593

Closed

add --transitive-only option to export-classpath! #8605

Merged

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch 2 times, most recently from ead735d to dff3194 Compare November 9, 2019 23:19

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch from dff3194 to 0929282 Compare November 27, 2019 10:08

cosmicexplorer mentioned this pull request Dec 19, 2019

option to output all command lines for all subprocesses #8838

Closed

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch from 666974d to 8d29b35 Compare January 8, 2020 00:04

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch from 407f9cf to 2126b73 Compare March 24, 2020 08:00

stuhood reviewed Apr 11, 2020

View reviewed changes

src/python/pants/engine/query.py Outdated Show resolved Hide resolved

src/python/pants/init/specs_calculator.py Outdated Show resolved Hide resolved

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch 3 times, most recently from cfa310f to ac79df0 Compare July 1, 2020 00:10

stuhood reviewed Jul 1, 2020

View reviewed changes

src/python/pants/option/global_options.py Outdated Show resolved Hide resolved

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch from ac79df0 to 0cec7af Compare July 21, 2020 06:09

Eric-Arellano reviewed Jul 21, 2020

View reviewed changes

src/python/pants/engine/query.py Outdated Show resolved Hide resolved

Eric-Arellano reviewed Jul 21, 2020

View reviewed changes

src/python/pants/init/extension_loader.py Show resolved Hide resolved

Eric-Arellano reviewed Jul 21, 2020

View reviewed changes

src/python/pants/scm/subsystems/changed.py Outdated Show resolved Hide resolved

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch from 0b03c48 to 0e56010 Compare July 21, 2020 08:36

cosmicexplorer commented Jul 21, 2020

View reviewed changes

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch 2 times, most recently from eabbb61 to ae3a2c3 Compare July 21, 2020 08:39

cosmicexplorer force-pushed the query-option-for-unifying-target-selection branch from ae3a2c3 to cc87ef4 Compare July 21, 2020 08:42

stuhood approved these changes Jul 21, 2020

View reviewed changes

stuhood reviewed Jul 21, 2020

View reviewed changes

illicitonion requested review from illicitonion and removed request for illicitonion July 21, 2020 21:47

Eric-Arellano reviewed Jul 22, 2020

View reviewed changes

Base automatically changed from master to main March 19, 2021 19:20

Eric-Arellano closed this Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a --query option for unifying target selection #7350

Add a --query option for unifying target selection #7350

cosmicexplorer commented Mar 10, 2019 •

edited

Loading

illicitonion left a comment

stuhood commented Mar 14, 2019

stuhood left a comment

cosmicexplorer commented Apr 5, 2019 •

edited

Loading

stuhood commented Apr 5, 2019

cosmicexplorer commented Apr 5, 2019

stuhood left a comment

Eric-Arellano Jul 21, 2020

cosmicexplorer Jul 21, 2020

cosmicexplorer Jul 21, 2020

Eric-Arellano Jul 21, 2020

cosmicexplorer Jul 21, 2020

coveralls commented Jul 21, 2020 •

edited

Loading

cosmicexplorer Jul 21, 2020

stuhood Jul 21, 2020

stuhood left a comment

stuhood Jul 21, 2020

Eric-Arellano Jul 22, 2020

stuhood Jul 21, 2020

Eric-Arellano Jul 22, 2020

stuhood Jul 21, 2020

stuhood left a comment

illicitonion commented Jul 21, 2020

Eric-Arellano left a comment

Eric-Arellano Jul 21, 2020

Eric-Arellano Jul 22, 2020

Eric-Arellano Jul 22, 2020

Eric-Arellano Jul 22, 2020

Eric-Arellano Jul 22, 2020

Eric-Arellano Jul 22, 2020

Eric-Arellano Jul 22, 2020

Eric-Arellano Jul 22, 2020

Eric-Arellano Jul 22, 2020

Eric-Arellano commented Jul 19, 2021

		# Parse --query expressions into objects which can be resolved into BuildFileAddresses via
		# v2 rules.

		@_uncacheable_rule
		def changed_files(request: ChangedFilesRequest) -> ChangedFiles:

		@@ -33,6 +35,7 @@ class DependeesOption(Enum):
		class ChangedRequest:

Add a --query option for unifying target selection #7350

Add a --query option for unifying target selection #7350

Conversation

cosmicexplorer commented Mar 10, 2019 • edited Loading

Problem

Solution

Result

illicitonion left a comment

Choose a reason for hiding this comment

stuhood commented Mar 14, 2019

stuhood left a comment

Choose a reason for hiding this comment

cosmicexplorer commented Apr 5, 2019 • edited Loading

stuhood commented Apr 5, 2019

cosmicexplorer commented Apr 5, 2019

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

illicitonion commented Jul 21, 2020

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Jul 19, 2021

cosmicexplorer commented Mar 10, 2019 •

edited

Loading

cosmicexplorer commented Apr 5, 2019 •

edited

Loading

coveralls commented Jul 21, 2020 •

edited

Loading