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.

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

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

Merged

Conversation

Projects
5 participants
@stuhood
Copy link
Member

commented Dec 7, 2018

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.

@stuhood stuhood requested review from jsirois, benjyw, illicitonion and kwlzn Dec 7, 2018

@stuhood

This comment was marked as outdated.

Copy link
Member Author

commented Dec 7, 2018

This will definitely fail a bunch of tests, because the --no-v1 --v2 split doesn't currently allow for replacing Tasks with @console_rules one at a time. Will fix that tomorrow.

EDIT: This part is now fixed, although it needs polish.

@stuhood stuhood added this to To Do in Python Pipeline Porting via automation Dec 7, 2018

@stuhood stuhood added the P3 - M1 label Dec 7, 2018

@stuhood stuhood moved this from To Do to In Progress in Python Pipeline Porting Dec 7, 2018

@stuhood stuhood removed request for jsirois, benjyw, illicitonion and kwlzn Dec 7, 2018

@benjyw

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

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

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2018

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.

@stuhood stuhood force-pushed the twitter:stuhood/console-rules-consume-options branch 2 times, most recently from 1081b02 to 29b22f9 Dec 8, 2018

@benjyw

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2018

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
Copy link
Contributor

left a comment

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!

Show resolved Hide resolved pants-plugins/src/python/internal_backend/rules_for_testing/register.py Outdated
Show resolved Hide resolved pants-plugins/src/python/internal_backend/rules_for_testing/register.py Outdated
Show resolved Hide resolved src/python/pants/bin/goal_runner.py Outdated
Show resolved Hide resolved src/python/pants/core_tasks/register.py
Show resolved Hide resolved src/python/pants/engine/rules.py
Show resolved Hide resolved src/python/pants/rules/core/list_targets.py Outdated
Show resolved Hide resolved src/python/pants/rules/core/list_targets.py Outdated
Show resolved Hide resolved src/python/pants/rules/core/list_targets.py Outdated
Show resolved Hide resolved src/python/pants/rules/core/list_targets.py Outdated
Show resolved Hide resolved src/python/pants/rules/core/fastlist.py Outdated
@stuhood

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2018

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.

@stuhood

This comment was marked as outdated.

Copy link
Member Author

commented Dec 13, 2018

I know that this one already got a review, but I'm going to do some rebasing in here anyway to try and make it more cohesive. Sorry!

@stuhood stuhood force-pushed the twitter:stuhood/console-rules-consume-options branch from 29b22f9 to 512b64f Dec 14, 2018

@stuhood

This comment was marked as outdated.

Copy link
Member Author

commented Dec 14, 2018

This is still unlikely to pass tests, but I've resolved about half of Daniel's comments. Hoping to have it reviewable tomorrow.

@illicitonion
Copy link
Contributor

left a comment

This is looking great! Thanks!

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

This comment has been minimized.

Copy link
@illicitonion

illicitonion Dec 14, 2018

Contributor

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

This comment has been minimized.

Copy link
@benjyw

benjyw Apr 15, 2019

Contributor

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?

@stuhood stuhood force-pushed the twitter:stuhood/console-rules-consume-options branch from 512b64f to b5c99a6 Dec 18, 2018

@stuhood

This comment was marked as outdated.

Copy link
Member Author

commented Dec 19, 2018

Made a bit more progress here today while on recess from jury duty... down to two bits of feedback, but still not quite reviewable.

@stuhood stuhood force-pushed the twitter:stuhood/console-rules-consume-options branch from d251819 to 7161ddd Apr 12, 2019

@stuhood stuhood requested review from benjyw and jsirois Apr 12, 2019

@stuhood

This comment was marked as outdated.

Copy link
Member Author

commented Apr 12, 2019

Although I haven't nailed down the strategy for getting these rendered in ./pants goals (...might need to deprecate it in favor of something integrated with ./pants help...), this should now be reviewable.

@illicitonion
Copy link
Contributor

left a comment

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

@stuhood stuhood force-pushed the twitter:stuhood/console-rules-consume-options branch from 1ace292 to e1fc575 Apr 18, 2019

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

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:

  1. the synthetic _GoalProduct class that we were creating under the covers to be the "actual" output type of a @console_rule
  2. 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
Copy link
Contributor

left a comment

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.

"""

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

This comment has been minimized.

Copy link
@illicitonion

illicitonion Apr 18, 2019

Contributor

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

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 18, 2019

Author Member

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

Show resolved Hide resolved src/python/pants/rules/core/filedeps.py Outdated
Show resolved Hide resolved tests/python/pants_test/rules/test_filedeps.py
@stuhood

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

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

benjyw approved these changes Apr 18, 2019

Copy link
Contributor

left a comment

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!

@stuhood stuhood force-pushed the twitter:stuhood/console-rules-consume-options branch from e1fc575 to 34c96b0 Apr 18, 2019

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

stuhood added a commit that referenced this pull request Apr 20, 2019

Replace the `goals` goal with a `help` plugin which supports both v1 …
…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.

stuhood added some commits Dec 8, 2018

Backfill existing options, by: 1) optionally add a deprecated depende…
…ncy on a scoped `CacheSetup` instance, 2) adding a `LineOriented` mixin for `Goal`.
Require a new Optionable subclass `Goal` as the first argument to `@c…
…onsole_rule` in order to provide help, options, and a CLI scope.
Overhaul Goal and @console_rule such that @console_rules actually pro…
…duce an instance of their Goal type, and options are provided by an inner class of Goal.

@stuhood stuhood force-pushed the twitter:stuhood/console-rules-consume-options branch from 34c96b0 to 68165cd Apr 20, 2019

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Python Pipeline Porting automation moved this from In Progress to Done Apr 21, 2019

@stuhood stuhood deleted the twitter:stuhood/console-rules-consume-options branch Apr 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.