Add support for consuming Subsystems from @rules #6993

stuhood · 2018-12-26T18:25:54Z

Problem

#6872 laid groundwork for consuming (Scoped)Options from @rules, but did not actually expose a facility for a @(console_)rule to consume a Subsystem.

Solution

Expose ScopedOptions parsing to @console_rules, and add a helper method that creates an @rule to construct a Subsystem. Finally, use this facility to have v2 list consume a Subsystem that holds options that define (most) of the behaviour that would allow it to replace the Task/v1 implementation of ./pants list.

Result

@rules and @console_rules can consume Subsystems. This is a prerequisite for #6880, which will build atop this facility to fully support v2-only goals, and replace v1 list with v2 list.

stuhood · 2019-01-02T21:57:30Z

This is now reviewable! Commits should be independently useful to review.

benjyw

Looks great, although it in some places exceeds the boundaries of my knowledge of this part of the code (which means it's good that I'm reviewing these, for my own benefit...)

benjyw · 2019-01-02T23:05:47Z

src/python/pants/build_graph/build_configuration.py

  def register_subsystems(self, subsystems):
+    return self.register_optionables(subsystems)
+
+  def register_optionables(self, optionables):
    """Registers the given subsystem types.


Docstring should mention "optionable" now, not "subsystem"?

Also, why is this bit necessary? What are the non-subsystem optionables we care about in this context?

Subsystem.scoped(cls) results in a SubsystemDependency, which implements OptionableFactory in this PR. And in #6880, Goal implements Optionable rather than Subsystem.

Eventually we might be able to shrink the Optionable/Subsystem split... but this PR widens that gap a bit.

benjyw · 2019-01-02T23:11:55Z

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

+
+
+@console_rule('list', [Select(Console), Select(ListOptions), Select(Specs)])
+def fast_list(console, options, specs):
  """A fast variant of `./pants list` with a reduced feature set."""


Can you remove the "reduced feature set" caveat? I don't see anything missing here now.

It's missing some of the options that used to be inherited from ConsoleTask: that will be resolved in #6880.

benjyw · 2019-01-02T23:33:22Z

src/python/pants/init/engine_initializer.py

    # Console rule can only have one subject.
    assert len(subjects) == 1
    for goal in goals:
      try:
        goal_product = self.goal_map[goal]
+        params = Params(subjects[0], options_bootstrapper, console)


This is my ignorance of the new engine talking, but where are these params consumed? I.e., what is using the options_bootstrapper in the console rule?

When the engine is invoked with a request for some type (a synthetic type ListGoalExecution in this case), the engine selects a root @rule to execute that produces that type given a non-ambiguous subset of the input params.

In this case, ListGoalExecution actually uses all 3 of these parameters to execute: a portion of the rendered rulegraph looks like:

That much I understood already :) What I'm missing is where in the code the bootstrapper param is used in the production of the type.

@benjyw : See the screenshot. It shows that:

fast_list depends on

construct_scope_fastlist (a synthetic method generated by this patch), which depends on

scope_options, which depends on

parse_options which consumes the OptionsBootstrapper.

OK, the piece I was missing was the code already in src/pants/src/python/pants/engine/legacy/options_parsing.py.

benjyw · 2019-01-02T23:35:26Z

src/python/pants/pantsd/service/scheduler_service.py

@@ -196,7 +196,7 @@ def _prefork_loop(self, session, options):
        continue
    return target_roots

-  def _prefork_body(self, session, options):
+  def _prefork_body(self, session, options, options_bootstrapper):


There's something a bit jarring about passing around an options along with the options_bootstrapper that created it, since, naively, the role of the latter is create the former, so one might wonder why you still need the bootstrapper around, or, if you do, why you can't generate options from it. Can you clarify in a comment somewhere? Or at least on the review, for my own edification?

Getting Options from an OptionsBootstrapper requires (re-)running the OptionsInitializer to collect known scopes and call get_full_options... the in-engine options parsing current re-does that work if requested. But as we port more code to v2, we should be able to push TargetRoots calculation and other options-consuming prerequisite code into the engine and make that the only instance of options parsing in a v2-only run.

I think that this options instance is actually only the global options and could be renamed to make that more obvious, but assuming this CI run goes green, I'd rather do that followup in #6880 instead.

Ah, that makes sense. Definitely don't rename it in this change.

…rules

…rules, and consume a ListOptions Subsystem in order to implement the rest of `list`.

stuhood · 2019-01-04T01:36:08Z

One confirmed flake. Merging.

illicitonion · 2019-01-07T15:24:40Z

src/python/pants/init/engine_initializer.py

    # Console rule can only have one subject.
    assert len(subjects) == 1
    for goal in goals:
      try:
        goal_product = self.goal_map[goal]
+        params = Params(subjects[0], options_bootstrapper, console)


What's the story with Console being a Param rather than a Singleton? I thought we wanted to minimise Params?

In this case, the Console being a Param is:

more semantically correct, because in the case of pantsd, each run has a "new" console (we mutate global variables to accomplish this elsewhere, but we wouldn't technically need to with appropriate use of injection like this)

it's easier to test, because we can inject a mock Console rather than using the mutation as above

@benjyw

### Problem When looking at @benjyw's [comment thread](#6993 (comment)) with @stuhood on #6993, it seemed like it was difficult to create effective screenshots of the rule graph. This seemed to be for two reasons: 1. The nodes of the drawn graph are exceedingly wide due to the text being written in a single line. 2. The context of each node isn't clear (root nodes are outlined in blue, which is hard to make out compared to black against a white background, especially as the HSV coloration that `dot` outputs or that the OSX Preview app displays seems to be a bit off compared to other HSV selectors). ### Solution - Insert some newlines in the formatting of nodes in the rule graph to make it skinnier. - Color many different types of nodes by printing the node name, then its attributes, before printing the node again within a pair of `{ ... }` in the `.dot` file. ### TODO - [x] Use `Display` impls as [noted by @stuhood below](#7024 (comment)), and/or make clear the difference between string representations for diagnostics and errors vs strings for graph drawing. - [x] Color intrinsics the way we now do with Params, Singletons, and root nodes. This would require modifying `entry_node_str_with_attrs()` in much the same way as is done with `entry_str()` and `entry_node_str_with_attrs()` here. - [x] Remove the stub `__str__()` implementations which currently have `???`s. - [ ] Consider using more formatting attributes, such as `height`, `width`, `fixed-size` -- see `dot(1)`. - [x] **(#9016)** Weight *edges* with `Get`s, as described in #7024 (comment). ### Result ![graph-thin](https://user-images.githubusercontent.com/1305167/73127432-270e4380-3fb8-11ea-880e-5104083c20c8.png) <img width="1425" alt="graph-wider" src="https://user-images.githubusercontent.com/1305167/73127433-270e4380-3fb8-11ea-9ebb-c319f1080abb.png"> <img width="1407" alt="graph-thicc" src="https://user-images.githubusercontent.com/1305167/73127431-270e4380-3fb8-11ea-8bf0-09c28f6c3229.png">

stuhood force-pushed the stuhood/optionables-for-rules branch 2 times, most recently from 2b7cf9c to 842ce46 Compare December 27, 2018 01:22

stuhood mentioned this pull request Dec 28, 2018

Incremental strategy for running/skipping targets via tag #6947

Closed

stuhood force-pushed the stuhood/optionables-for-rules branch from 842ce46 to 7838a88 Compare January 2, 2019 21:46

stuhood requested review from illicitonion, benjyw and cosmicexplorer January 2, 2019 21:56

stuhood mentioned this pull request Jan 2, 2019

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

Merged

benjyw approved these changes Jan 2, 2019

View reviewed changes

stuhood force-pushed the stuhood/optionables-for-rules branch from 7838a88 to 112ba46 Compare January 3, 2019 01:44

stuhood added 4 commits January 3, 2019 11:13

Expose ScopedOptions to @console_rules.

843fe77

Move ScopedOptions to pants.option.scope.

8f6f0af

Deprecate BuildConfiguration.subsystems in favor of optionables.

909a255

Add support for constructing Optionables (including Subsystems) via @…

9f68fc6

…rules, and consume a ListOptions Subsystem in order to implement the rest of `list`.

stuhood force-pushed the stuhood/optionables-for-rules branch from 112ba46 to 9f68fc6 Compare January 3, 2019 19:27

stuhood merged commit 2947e97 into pantsbuild:master Jan 4, 2019

stuhood deleted the stuhood/optionables-for-rules branch January 4, 2019 01:36

cosmicexplorer mentioned this pull request Jan 4, 2019

Nicer rule graph viz #7024

Merged

5 tasks

illicitonion reviewed Jan 7, 2019

View reviewed changes

stuhood mentioned this pull request Apr 8, 2019

A @console_rule for validating source files against regexes. #7515

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for consuming Subsystems from @rules #6993

Add support for consuming Subsystems from @rules #6993

stuhood commented Dec 26, 2018 •

edited

Loading

stuhood commented Jan 2, 2019

benjyw left a comment

benjyw Jan 2, 2019

benjyw Jan 2, 2019

stuhood Jan 2, 2019

benjyw Jan 2, 2019

stuhood Jan 4, 2019

benjyw Jan 2, 2019

stuhood Jan 3, 2019

benjyw Jan 3, 2019

stuhood Jan 3, 2019

benjyw Jan 4, 2019

benjyw Jan 2, 2019

stuhood Jan 4, 2019 •

edited

Loading

benjyw Jan 4, 2019

stuhood commented Jan 4, 2019

illicitonion Jan 7, 2019

stuhood Jan 7, 2019 •

edited

Loading

Add support for consuming Subsystems from @rules #6993

Add support for consuming Subsystems from @rules #6993

Conversation

stuhood commented Dec 26, 2018 • edited Loading

Problem

Solution

Result

stuhood commented Jan 2, 2019

benjyw 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Jan 4, 2019

Choose a reason for hiding this comment

stuhood Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

stuhood commented Dec 26, 2018 •

edited

Loading

stuhood Jan 4, 2019 •

edited

Loading

stuhood Jan 7, 2019 •

edited

Loading