Add support for v2-only goals, and replace list with a @console_rule #6880

stuhood · 2018-12-07T02:53:33Z

Problem

While @console_rule has existed for a while now, it has not yet been possible to install a @console_rule as the exclusive implementer of a goal like ./pants list (see #6918 and #6651). Additionally, replacing existing goals comes with a need to backwards-compatibly consume options.

Solution

Run goals that are unambiguously --v1 or --v2 without passing those flags: this means that passing the --v1 vs --v2 flags is only necessary in cases where both v1 and v2 goals are installed. Additionally, create an engine.goal.Goal class with an inner Optionable instance in order to claim a CLI scope (and have options), and require one for all @console_rules. Finally, add a ConsoleRuleTestBase class to easily unit test @console_rules.

Result

The list goal is now implemented as a @console_rule, and consequently: is faster. ./pants help list is unchanged due to a deprecated dependency on a CacheSetup subsystem, and an optional Goal-mixin for wrapping line-oriented output around a Console. Fixes #6918 and fixes #6651.

benjyw · 2018-12-07T23:40:27Z

So how does option equality work here? Presumably we only want to invalidate on changes to options with fingerprint=True, right?

stuhood · 2018-12-08T00:00:01Z

So how does option equality work here? Presumably we only want to invalidate on changes to options with fingerprint=True, right?

fingerprint=True is used for v1 caching, but v2 caching is intended to be 100% "arguments to a subprocess" based.

Option/value equality in the engine's graph is used for memoization in the in-memory graph only: so it's essentially only used to avoid re-running things with pantsd. And in that case, any changes to a @(console_)rules arguments are relevant, because otherwise it would not re-run at all.

pants-plugins/src/python/internal_backend/rules_for_testing/register.py

benjyw · 2018-12-08T18:16:49Z

So any option change will trigger a rebuild of the graph, but if the option change didn't happen to cause any input changes to any subprocesses, they won't re-run?

illicitonion

This is super exciting, great to see!

My only major point which may need discussion is about the tight coupling/overloading of subsystem with v2 goal.

Otherwise a few broken corners, a few bits of boilerplate it would be nice to remove, but looks great!

pants-plugins/src/python/internal_backend/rules_for_testing/register.py

src/python/pants/bin/goal_runner.py

src/python/pants/core_tasks/register.py

src/python/pants/engine/rules.py

src/python/pants/rules/core/fastlist.py

stuhood · 2018-12-13T19:20:48Z

So any option change will trigger a rebuild of the graph, but if the option change didn't happen to cause any input changes to any subprocesses, they won't re-run?

Correct.

illicitonion

This is looking great! Thanks!

illicitonion · 2018-12-14T08:29:04Z

src/python/pants/engine/goal.py

+  @classproperty
+  def options_scope(cls):
+    if not cls.name:
+      # TODO: Would it be unnecessarily magical to have `cls.__name__.lower()` always be the name?


We'd want snake_case, rather than lower, and probably want it to be overridable? Or maybe not overridable... It is a bit magical, but it's magic I kind of like...

I think that would be a fine default. It should probably be overridable though.
Devil's advocate: Should Goals necessarily be optionable at all? Do we want that coupling? Why not let authors define subsystems where needed?

illicitonion

Looks good :) I still have a handful of questions, but a much smaller handful!

src/python/pants/backend/project_info/rules/source_file_validator.py

src/python/pants/engine/goal.py

src/python/pants/rules/core/filedeps.py

src/python/pants/rules/core/list_targets.py

tests/python/pants_test/console_rule_test_base.py

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

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

stuhood · 2019-04-18T06:40:09Z

I've added one commit on top that resolves @illicitonion's last round of concerns, and (most significantly,) applies the syntax changes from #6880 (comment)

This unifies/replaces:

the synthetic _GoalProduct class that we were creating under the covers to be the "actual" output type of a @console_rule
GracefulTerminationException

...with having @console_rules yield a concrete instance of their Goal that contains an exit code.

I'm really, really happy with how that clarifies and simplifies the syntax... the one downside was that the inner Goal.Options class has a pretty awkward implementation. But it feels worth it to me assuming that usage is well documented and examples are clear.

illicitonion

That's a really cute solution, very nice!

My only open question now is: Should exit_code be optional (i.e. None means "I was fine" and 0 means "Eagerly terminate with exit code 0"? I feel like allowing eager termination is probably a bad thing, but also, using exit_code to conditionally mean "exit with this code" or "ignore this because it's a magic value" feels a little odd.

illicitonion · 2019-04-18T12:23:52Z

src/python/pants/engine/rules.py

  """

+  is_goal_cls = isinstance(output_type, type) and issubclass(output_type, Goal)
+  if is_goal_cls == cacheable:


Can we remove the cacheable arg, and define cacheability exactly based on whether a Goal is returned, rather than having this check?

Yes, but I think that that would make @console_rule unnecessary, and that feels like something we should wait until #6598 to nail down.

src/python/pants/rules/core/filedeps.py

tests/python/pants_test/rules/test_filedeps.py

stuhood · 2019-04-18T17:46:03Z

My only open question now is: Should exit_code be optional (i.e. None means "I was fine" and 0 means "Eagerly terminate with exit code 0"? I feel like allowing eager termination is probably a bad thing, but also, using exit_code to conditionally mean "exit with this code" or "ignore this because it's a magic value" feels a little odd.

A default exit code would probably be fine, yea...

Alternatively, singletons for success and failure?

class Goal(..):
  @memoized_classproperty
  def Success(cls):
    return cls(exit_code=0)
  
  ...

...

yield MyGoal.Success

benjyw

I like how this turned out.

I'm of two minds on whether we need Goal.Options at all. We could just require rule authors to create whatever subsystems they need in order to provide options to their console_rules. OTOH I can see how it might be a useful idiom in practice to have an implicit options scope attached to every console_rule, and then making it easy to register options on that scope without having to explicitly define a separate subsystem with a matching scope, so I'm fine with this on balance.

Great stuff!

src/python/pants/engine/goal.py

…and v2 (#7598) ### Problem In #6880, there will be two types of Goals: 1. v1 `pants.goal.goal.Goal`s, registered on a singleton 2. v2 `pants.engine.goal.Goal`s, registered as a `ScopeInfo` category via `@console_rules`. But `./pants goals` supports only the first form (via access to the singleton), and would need to dig deep into `self.context` in order to get access to the scope info (...like `./pants options` does: but that's a story for another day!). ### Solution Convert `./pants goals` into a plugin to the arg splitter, similar to `./pants help`. While the arg-splitter method is not incredibly scalable, it seems like it should be sufficiently scalable to support most of our "meta-goals". ### Result v1 and v2 Goals are supported, we have one fewer v1 Task, and #6880 is unblocked.

…onsole_rule` in order to provide help, options, and a CLI scope.

…nd dedupe scope registrations.

…empty specs.

…ncy on a scoped `CacheSetup` instance, 2) adding a `LineOriented` mixin for `Goal`.

…duce an instance of their Goal type, and options are provided by an inner class of Goal.

…xits more uniform.

…iledeps` (#8184) ### Problem We want to finish replacing V1 `filedeps` with V2 `fast-filedeps`. Keeping both goals around gives us added complexity, e.g. more time in CI for tests. To finish the replacement, `fast-filedeps` must first get support for `--absolute` and `--globs`. ### Solution Add support for `--absolute` and `--globs`. Also, fix and refactor the unit tests, which were skipped in #6880 and never restored. Update the integration test to mirror the V1 test. We can't yet actually drop the V1 goal yet due to two features that need to be implemented for full feature parity: 1) handling `scala_library`'s `java_sources` and 2) supporting `jvm_app`s. This PR adds failing unit tests for both those cases. Once those tests can be un-skipped, then we can drop V1.

stuhood requested review from jsirois, benjyw, illicitonion and kwlzn December 7, 2018 02:53

This comment has been minimized.

Sign in to view

stuhood added the P3 - M1 label Dec 7, 2018

stuhood removed request for jsirois, benjyw, illicitonion and kwlzn December 7, 2018 18:33

stuhood mentioned this pull request Dec 7, 2018

v2 goals must shadow names of v1 goals #6651

Closed

stuhood force-pushed the stuhood/console-rules-consume-options branch 2 times, most recently from 1081b02 to 29b22f9 Compare December 8, 2018 02:35

cosmicexplorer reviewed Dec 8, 2018

View reviewed changes

pants-plugins/src/python/internal_backend/rules_for_testing/register.py Show resolved Hide resolved

cosmicexplorer mentioned this pull request Dec 10, 2018

make a rust ruleset to wrap cargo and build the native engine #6891

Closed

illicitonion requested changes Dec 10, 2018

View reviewed changes

illicitonion mentioned this pull request Dec 13, 2018

./pants --no-v1 --v2 doesn't complain about no goals specified #6918

Closed

This comment has been minimized.

Sign in to view

stuhood force-pushed the stuhood/console-rules-consume-options branch from 29b22f9 to 512b64f Compare December 14, 2018 06:24

This comment has been minimized.

Sign in to view

illicitonion reviewed Dec 14, 2018

View reviewed changes

stuhood force-pushed the stuhood/console-rules-consume-options branch from 512b64f to b5c99a6 Compare December 18, 2018 22:58

This comment has been minimized.

Sign in to view

stuhood mentioned this pull request Dec 20, 2018

Add support for deprecating scoped SubsystemDependencies #6961

Merged

stuhood force-pushed the stuhood/console-rules-consume-options branch from d251819 to 7161ddd Compare April 12, 2019 06:53

stuhood requested review from benjyw and jsirois April 12, 2019 07:11

This comment has been minimized.

Sign in to view

illicitonion reviewed Apr 15, 2019

View reviewed changes

stuhood mentioned this pull request Apr 17, 2019

Support de-quieting the context logger in ConsoleTasks #7585

Closed

stuhood force-pushed the stuhood/console-rules-consume-options branch from 1ace292 to e1fc575 Compare April 18, 2019 06:33

illicitonion approved these changes Apr 18, 2019

View reviewed changes

stuhood mentioned this pull request Apr 18, 2019

Dirty the dependents of an uncacheable Node #6598

Closed

benjyw approved these changes Apr 18, 2019

View reviewed changes

stuhood force-pushed the stuhood/console-rules-consume-options branch from e1fc575 to 34c96b0 Compare April 18, 2019 19:44

stuhood mentioned this pull request Apr 20, 2019

Replace the goals goal with a help plugin which supports both v1 and v2 #7598

Merged

jsirois approved these changes Apr 20, 2019

View reviewed changes

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

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

stuhood added 7 commits April 20, 2019 14:28

Require a new Optionable subclass Goal as the first argument to `@c…

201980d

…onsole_rule` in order to provide help, options, and a CLI scope.

Run unambiguous goals regardless of the --v1 and --v2 settings, a…

ef86a50

…nd dedupe scope registrations.

Remove excess wrapping from Specs creation to fix assertion error on …

10fd0d8

…empty specs.

Port ListTargets test to a new ConsoleRuleTestBase base class.

f69c6f4

Backfill existing options, by: 1) optionally add a deprecated depende…

0d32b84

…ncy on a scoped `CacheSetup` instance, 2) adding a `LineOriented` mixin for `Goal`.

Overhaul Goal and @console_rule such that @console_rules actually pro…

e40fe1e

…duce an instance of their Goal type, and options are provided by an inner class of Goal.

Test fixes: ensure that RunTracker runs are created, and make v1/v2 e…

68165cd

…xits more uniform.

stuhood force-pushed the stuhood/console-rules-consume-options branch from 34c96b0 to 68165cd Compare April 20, 2019 21:41

line_oriented as classmethod

6af8b78

stuhood merged commit d064804 into pantsbuild:master Apr 21, 2019

stuhood deleted the stuhood/console-rules-consume-options branch April 21, 2019 00:04

Eric-Arellano mentioned this pull request Aug 18, 2019

Add options to fast-filedeps to bring it close to parity with V1 filedeps #8184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for v2-only goals, and replace list with a @console_rule #6880

Add support for v2-only goals, and replace list with a @console_rule #6880

stuhood commented Dec 7, 2018 •

edited

Loading

This comment has been minimized.

benjyw commented Dec 7, 2018

stuhood commented Dec 8, 2018

benjyw commented Dec 8, 2018

illicitonion left a comment

stuhood commented Dec 13, 2018

This comment has been minimized.

This comment has been minimized.

illicitonion left a comment

illicitonion Dec 14, 2018

benjyw Apr 15, 2019

This comment has been minimized.

This comment has been minimized.

illicitonion left a comment

stuhood commented Apr 18, 2019 •

edited

Loading

illicitonion left a comment

illicitonion Apr 18, 2019

stuhood Apr 18, 2019

stuhood commented Apr 18, 2019

benjyw left a comment

Add support for v2-only goals, and replace list with a @console_rule #6880

Add support for v2-only goals, and replace list with a @console_rule #6880

Conversation

stuhood commented Dec 7, 2018 • edited Loading

Problem

Solution

Result

This comment has been minimized.

benjyw commented Dec 7, 2018

stuhood commented Dec 8, 2018

benjyw commented Dec 8, 2018

illicitonion left a comment

Choose a reason for hiding this comment

stuhood commented Dec 13, 2018

This comment has been minimized.

This comment has been minimized.

illicitonion left a comment

Choose a reason for hiding this comment

illicitonion Dec 14, 2018

Choose a reason for hiding this comment

benjyw Apr 15, 2019

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

illicitonion left a comment

Choose a reason for hiding this comment

stuhood commented Apr 18, 2019 • edited Loading

illicitonion left a comment

Choose a reason for hiding this comment

illicitonion Apr 18, 2019

Choose a reason for hiding this comment

stuhood Apr 18, 2019

Choose a reason for hiding this comment

stuhood commented Apr 18, 2019

benjyw left a comment

Choose a reason for hiding this comment

stuhood commented Dec 7, 2018 •

edited

Loading

stuhood commented Apr 18, 2019 •

edited

Loading