Merge subjects and variants into Params, and remove Noop #6170

stuhood · 2018-07-17T23:47:06Z

Problem

As described in #5788: @rules need a way to rely on values that are provided at request time, but which do not necessarily participate in the signatures of all @rules between the root and the consuming @rule. This is related to the existing concept of "variants", but requires an implementation that does not introduce variants into Node identities in subgraphs where they are not required.

Additionally, as described a while back in #4304, it should be possible to generate concrete subgraphs by removing ambiguity from the RuleGraph... but ambiguity is currently a "feature" required for composability of @rules that do not know about one another.

Solution

This change merges Variants and "subjects" into Params, and statically determines which Params are required in each subgraph. In order to handle cases where multiple providers of an @rule type dependency are available with different required input Params, the change "monomorphizes" (duplicates) RuleGraph entries per used parameter set. This allows us to remove runtime Noops, because every RuleGraph entry (and thus Node) has exactly one @rule provider for each of its declared dependencies.

Result

Lays groundwork for #4020 and #5788. Fixes #4304 by monomorphizing RuleGraph entries and removing Noop. Fixes #4027 by... deleting that code.

This change does not yet expose any sort of UX for providing more than one Param in a Get or root request, but it was already way too large, so I've opened #6478 for followup.

illicitonion

Looks like this is getting a lot simpler :) Thanks!

A few questions from having a look through (definitely not close review, as it's WIP :))...

illicitonion · 2018-07-20T10:05:02Z

src/rust/engine/src/core.rs

-/// as sorted string tuples.
+/// Params represent a TypeId->Key map.
+///
+/// For efficiency and hashability, they're stored as sorted Keys (with distinct TypeIds), and


This description made me assume this would be a Cow<BTreeSet<Key>> rather than an Arc<Vec<Key>>...

If we were to use a BTreeSet, we'd still need to verify unique typeids, so would need a wrapper around Key to redefine Ord in terms of typeids.

It's possible that the Arc is overkill here though (since Key is so small).

illicitonion · 2018-07-20T10:07:54Z

src/rust/engine/src/core.rs

+impl Params {
+  pub fn new(mut params: Vec<Key>) -> Params {
+    params.sort_by(|l, r| l.type_id().cmp(r.type_id()));
+    params.dedup_by(|l, r| l.type_id() == r.type_id());


Feels like it should be an error for this line to actually do anything?

Agreed. Will add.

illicitonion · 2018-07-20T10:11:04Z

src/rust/engine/src/nodes.rs

-      entries: edges.entries_for(&select_key),
-    }
+  pub fn new(product: TypeConstraint, params: Params, edges: &rule_graph::RuleEdges) -> Select {
+    Self::new_with_selector(selectors::Select::without_variant(product), params, edges)


Is the plan for Select to disappear in favour of just a TypeConstraint (or maybe a wrapper-type around it), and for tasks_add_select_variant to disappear?

Yeah, now that without_variant doesn't contrast with anything, this feels like a weird API...

Agreed: I'll clean these up today.

illicitonion · 2018-07-20T10:20:03Z

src/rust/engine/src/rule_graph.rs

 use std::io;

 use core::{Function, Key, TypeConstraint, TypeId, Value, ANY_TYPE};
 use externs;
 use selectors::{Get, Select};
 use tasks::{Intrinsic, Task, Tasks};

+type ParamTypes = BTreeSet<TypeId>;


What's the big difference between this and the Params?

The key difference is Params contains Keys (ie, actual python values). But also, in theory performance is less critical in this case because the vast majority of Entries should be created and destroyed during RuleGraph initialization.

stuhood · 2018-07-25T05:27:54Z

This is now reviewable.

It contains a ton of TODOs that I'll either open issues for or resolve, and still has some (trivially, I think) failing tests. But it shouldn't need any fundamental changes.

The Compute used parameters... and Add "AggregationRule"... commits are highly overlapping, and should probably be reviewed together. The rest are relatively independent. Sorry for the big mess!

illicitonion

Where are Aggregations actually used? Can you point at an example (which is tested)?

I'm not convinced all of the tests you're deleting should be deleted... Are some of those to be restored?

I think this could do with a decent prosaic description of what "kinds" of rules we support. Would you be able to put one together (or does one already exist)?

I also have some outstanding comments from my first pass which I think still stand.

illicitonion · 2018-07-25T10:53:39Z

src/rust/engine/src/nodes.rs

-      entries: edges.entries_for(&select_key),
-    }
+  pub fn new(product: TypeConstraint, params: Params, edges: &rule_graph::RuleEdges) -> Select {
+    Self::new_with_selector(selectors::Select::without_variant(product), params, edges)


Yeah, now that without_variant doesn't contrast with anything, this feels like a weird API...

illicitonion · 2018-07-25T10:57:19Z

src/rust/engine/src/nodes.rs

  ///
-  fn gen_nodes(&self, context: &Context) -> Vec<NodeFuture<Value>> {
+  /// TODO: This could take `self` by value and avoid cloning.


Sounds good

Was planning to leave this TODO here long enough to land this, and then inline this method into Select, as the TODO near the end of the method suggests. Reasonable?

illicitonion · 2018-07-25T10:58:35Z

src/rust/engine/src/nodes.rs

+          .core
+          .rule_graph
+          .edges_for_inner(&self.entry)
+          .expect("Expected edges to exist for Aggregation.");


Include a debug output to say what aggregation it is?

Will do. And will try to move most of edges_for_inner(..).expect calls into RuleGraph, as dependency edges not existing would represent a bug there rather than here.

illicitonion · 2018-07-25T11:04:48Z

src/rust/engine/src/nodes.rs

-        let externs::Get(product, subject) = get;
-        let entries = context
+      .map(|externs::Get(product, subject)| {
+        // TODO: The subject of the get is a new parameter, but params from the context should be


Definitely needs a ticket :)

Agreed. I may do this before landing, but will open a ticket otherwise.

illicitonion · 2018-07-25T11:06:06Z

src/rust/engine/src/rule_graph.rs

@@ -20,24 +24,27 @@ impl UnreachableError {
    UnreachableError {
      task_rule: task_rule,
      diagnostic: Diagnostic {
-        subject_type: ANY_TYPE,
+        params: Default::default(),


My pet peeve: ParamTypes::default() :)

illicitonion · 2018-07-25T13:48:54Z

src/rust/engine/src/rule_graph.rs

+          return Ok(ConstructGraphResult::Unfulfillable);
+        }
+      };
+    let simplified_entries_only = simplified_entries


simplified_entries.keys().cloned().collect()?

illicitonion · 2018-07-25T14:01:12Z

src/rust/engine/src/rule_graph.rs

+    let mut diagnostics = Vec::new();
+    for available_params in params_powerset {
+      let available_params = available_params.into_iter().collect();
+      // If a subset of these parameters is already satisfied, skip. This has the effect of


I don't follow this... Is this required? Or an optimisation?

It is required. This dedupes entries that use subsets of one-another's parameters. IE, if an @rule is satisfiable with an Address and Address+OtherThing, we prefer the subset. This also differentiates between the rule that would use only OtherThing and the rule that would use only Address.

But I think it needs unification with the logic in choose_dependency that does a similar thing... and choose_dependency likely needs some sort of unification with rhs. Will look into what can be deduped here before landing.

Did you follow up on this? Did it turn out no unification could happen, or should there be a ticket to follow up?

I followed up, and didn't see an obvious unification. It's possible that when I dive into the tests tomorrow I can tease apart a unification, but I'm not optimistic.

illicitonion · 2018-07-25T14:04:10Z

src/rust/engine/src/rule_graph.rs

+          return Ok(None);
+        }
+        1 => {
+          combination.push((key, *chosen_entries.last().unwrap()));


combination.push((key, *chosen_entries[0]));?

illicitonion · 2018-07-25T14:07:08Z

src/rust/engine/src/rule_graph.rs

+
+    // Prefer a Singleton, then a Param, then the non-ambiguous rule with the smallest set of
+    // input Params.
+    // TODO: We should likely prefer Rules to Params.


Why should we prefer Rules to Params? In fact; why are these directly comparable at all? They feel like they're separate; what's an @rule signature with an example of a Params and Rules where we need to resolve between the two?

Good question. This needs unification with rhs, but: the non-intuitive thing I discovered while implementing this is that in order to keep Node identities small (ie, containing the fewest Params possible), we want to prefer Entries that need fewer parameters. This biases toward avoiding propagating information from callers to callees unless they really need it to be executed (and avoids the case where absolutely anything used anywhere in the graph is potentially a parameter used at the root of the graph: that will continue to be bounded by RootRule).

In short: we would generally prefer a @rule to a Param, because depending on a rule does not affect our identity, while using a Param does (because it comes from the caller).

Maybe add this comment as a code comment?

illicitonion · 2018-07-25T14:11:23Z

src/rust/engine/src/rule_graph.rs

+            rules_by_kind
+              .entry(wd.simplified(BTreeSet::new()))
+              .and_modify(|e| {
+                // TODO: Param set size isn't sufficient to fully differentiate alternatives: would


Why do we prefer larger not smaller?

This comment is stale: this code is actually prefering the smaller set. As mentioned above, needs some unification.

stuhood

Where are Aggregations actually used? Can you point at an example (which is tested)?

They were used in (and necessary for) planners.py, but there was at least one more source of ambiguity in those rules (related to #4005), and I couldn't justify adding more features here.

I plan to heavily extend tests/python/pants_test/engine/test_rules.py today to cover use of AggregationRule, and the new graph ambiguity errors.

I'm not convinced all of the tests you're deleting should be deleted... Are some of those to be restored?

All of the deleted tests consumed planners.py, and about 50% of them were already skipped as we deleted expensive features that they depended on. While I think that there was only one additional feature needed in this diff (a resolution to #4005) to support them, the tests in test_rules.py and etc are significantly easier to maintain, while providing more useful examples of behaviour.

If we feel strongly that we should keep them, I think I'd need to dive in on #4005 to allow planners.py to continue to use the weird ducktyping it is using currently. But I believe that that would actually be redundant with #4535, which proposes to use a more principled approach to extension of a union type.

I think this could do with a decent prosaic description of what "kinds" of rules we support. Would you be able to put one together (or does one already exist)?

That exists in https://github.com/pantsbuild/pants/blob/master/src/python/pants/engine/README.md : I will definitely extend it here to account for AggregationRule.

I also fully expect to be able to remove SingletonRule as the UX for Params is fleshed out (since a singleton is simply a zero-Param @rule).

stuhood · 2018-07-25T15:41:53Z

src/rust/engine/src/core.rs

-/// as sorted string tuples.
+/// Params represent a TypeId->Key map.
+///
+/// For efficiency and hashability, they're stored as sorted Keys (with distinct TypeIds), and


If we were to use a BTreeSet, we'd still need to verify unique typeids, so would need a wrapper around Key to redefine Ord in terms of typeids.

It's possible that the Arc is overkill here though (since Key is so small).

stuhood · 2018-07-25T15:42:02Z

src/rust/engine/src/core.rs

+impl Params {
+  pub fn new(mut params: Vec<Key>) -> Params {
+    params.sort_by(|l, r| l.type_id().cmp(r.type_id()));
+    params.dedup_by(|l, r| l.type_id() == r.type_id());


Agreed. Will add.

stuhood · 2018-07-25T15:42:37Z

src/rust/engine/src/nodes.rs

-      entries: edges.entries_for(&select_key),
-    }
+  pub fn new(product: TypeConstraint, params: Params, edges: &rule_graph::RuleEdges) -> Select {
+    Self::new_with_selector(selectors::Select::without_variant(product), params, edges)


Agreed: I'll clean these up today.

stuhood · 2018-07-25T15:44:11Z

src/rust/engine/src/nodes.rs

  ///
-  fn gen_nodes(&self, context: &Context) -> Vec<NodeFuture<Value>> {
+  /// TODO: This could take `self` by value and avoid cloning.


Was planning to leave this TODO here long enough to land this, and then inline this method into Select, as the TODO near the end of the method suggests. Reasonable?

stuhood · 2018-07-25T15:46:16Z

src/rust/engine/src/nodes.rs

+          .core
+          .rule_graph
+          .edges_for_inner(&self.entry)
+          .expect("Expected edges to exist for Aggregation.");


Will do. And will try to move most of edges_for_inner(..).expect calls into RuleGraph, as dependency edges not existing would represent a bug there rather than here.

stuhood · 2018-07-25T15:58:49Z

src/rust/engine/src/rule_graph.rs

+    let mut diagnostics = Vec::new();
+    for available_params in params_powerset {
+      let available_params = available_params.into_iter().collect();
+      // If a subset of these parameters is already satisfied, skip. This has the effect of


It is required. This dedupes entries that use subsets of one-another's parameters. IE, if an @rule is satisfiable with an Address and Address+OtherThing, we prefer the subset. This also differentiates between the rule that would use only OtherThing and the rule that would use only Address.

But I think it needs unification with the logic in choose_dependency that does a similar thing... and choose_dependency likely needs some sort of unification with rhs. Will look into what can be deduped here before landing.

stuhood · 2018-07-25T15:59:04Z

src/rust/engine/src/rule_graph.rs

+          return Ok(None);
+        }
+        1 => {
+          combination.push((key, *chosen_entries.last().unwrap()));


stuhood · 2018-07-25T16:06:28Z

src/rust/engine/src/rule_graph.rs

+
+    // Prefer a Singleton, then a Param, then the non-ambiguous rule with the smallest set of
+    // input Params.
+    // TODO: We should likely prefer Rules to Params.


Good question. This needs unification with rhs, but: the non-intuitive thing I discovered while implementing this is that in order to keep Node identities small (ie, containing the fewest Params possible), we want to prefer Entries that need fewer parameters. This biases toward avoiding propagating information from callers to callees unless they really need it to be executed (and avoids the case where absolutely anything used anywhere in the graph is potentially a parameter used at the root of the graph: that will continue to be bounded by RootRule).

In short: we would generally prefer a @rule to a Param, because depending on a rule does not affect our identity, while using a Param does (because it comes from the caller).

stuhood · 2018-07-25T16:07:11Z

src/rust/engine/src/rule_graph.rs

+            rules_by_kind
+              .entry(wd.simplified(BTreeSet::new()))
+              .and_modify(|e| {
+                // TODO: Param set size isn't sufficient to fully differentiate alternatives: would


This comment is stale: this code is actually prefering the smaller set. As mentioned above, needs some unification.

stuhood · 2018-07-25T16:08:06Z

src/rust/engine/src/rule_graph.rs

@@ -517,9 +974,9 @@ impl RuleGraph {
  }

  pub fn find_root_edges(&self, subject_type: TypeId, select: Select) -> Option<RuleEdges> {
-    // TODO return Result instead
+    // TODO: Support more than one root parameter... needs some API work.


Will open a followup ticket for multiple-parameter-Get UX.

baroquebobcat

Did a first pass at this. Most of my comments are for small nitpicks.

I like this change quite a bit. I think that, conceptually, param is a much easier thing to explain than subject and variant. And, I think param aligns better with what those things were used for.

baroquebobcat · 2018-07-26T20:08:20Z

src/python/pants/engine/rules.py

+  """A rule that receives the results of all other rules for a product to aggregate them.
+
+  An AggregationRule supports composability of @rules by allowing additional rules to be installed
+  to provide some type without removing or otherwise modifying the installed rules.


instead of to provide some type, maybe that provide the same type?

baroquebobcat · 2018-07-26T20:14:24Z

src/rust/engine/src/rule_graph.rs

+  // The Entry was satisfiable without waiting for any additional nodes to be satisfied. The result
+  // contains copies of the input Entry for each set subset of the parameters that satisfy it.
+  Fulfilled(Vec<EntryWithDeps>),
+  // The Entry was not satifiable with installed rules.


nit: sp satisfiable

baroquebobcat · 2018-07-26T20:16:08Z

src/rust/engine/src/rule_graph.rs

-  pub fn sub_graph(&self, subject_type: TypeId, product_type: &TypeConstraint) -> RuleGraph {
-    if let Some(beginning_root) = self.gen_root_entry(subject_type, product_type) {
-      self._construct_graph(vec![beginning_root])
+  pub fn sub_graph(&self, subject_type: &TypeId, product_type: &TypeConstraint) -> RuleGraph {


nit: rename subject_type to param_type?

ity

reading through this was a learning experience for me! thank you for the absolutely useful changes with a lot of complexity removed. And, would love to see smaller/more digestible changes in the future :)

stuhood · 2018-09-10T00:20:32Z

I've rebased this and applied review feedback: only the topmost commit contains anything new.

Having thought a bit more about what rule graph extensibility should look like, I decided to rebase AggregationRule out of this branch, and to stash it so that it can be considered in the context of implementing #4535 (which needs something that is shaped slightly differently).

I'd like to clean up the tests and land this tomorrow if reviewers agree.

illicitonion

Handful of todos left to go, then let's get this thing merged :D

…ams in Node implementations.

…le-subject fields with multi-Param fields.

…actly one candidate per RuleEdge(s), and remove Noop.

…ue to support.

… time round).

### Problem In #6170, it became possible for a `@rule` to take no `Params`, meaning that its identity did not include any of its context, and it could act as a singleton. Before that change, it had always been the case that a `@rule` would have a "subject" in its identity, necessitating `SingletonRule` in order to avoid creating a new copy of the rule in the graph for each distinct subject. ### Solution Remove `SingletonRule` in favor of definition of zero-parameter `@rules`, which reduces the total number of concepts involved in the engine. Because `SingletonRule` used to allow a declaration to lie about the exact type it was providing (and give an instance of a subclass instead), usage of `SymbolTable` first needed to be cleaned up to remove subclassing.

### Problem The engine `README.md` had not been updated for the merge of "subjects" and `Variants` into `Params` (#6170). ### Solution Overhaul the explanations to refer to `Params`, and expand on how `Params` are propagated from dependents to dependencies. Additionally, simplify and document the codepath in rule graph construction that selects which `@rule` dependencies to use.

stuhood mentioned this pull request Jul 17, 2018

Implement support for @rules that can directly request Subsystems #5788

Closed

illicitonion reviewed Jul 20, 2018

View reviewed changes

stuhood force-pushed the stuhood/subsingliant branch 2 times, most recently from 5378da5 to 438e607 Compare July 25, 2018 04:49

stuhood changed the title ~~WIP: Merge subjects and variants into Params~~ Merge subjects and variants into Params, and remove Noop Jul 25, 2018

stuhood requested review from baroquebobcat, illicitonion, ity and cosmicexplorer July 25, 2018 05:25

illicitonion reviewed Jul 25, 2018

View reviewed changes

stuhood commented Jul 25, 2018

View reviewed changes

baroquebobcat reviewed Jul 26, 2018

View reviewed changes

ity reviewed Aug 10, 2018

View reviewed changes

This was referenced Sep 9, 2018

Specifying deps outside of BUILD files (aka "dep inference") #6449

Closed

Add API for root requests with multiple parameters #6478

Closed

stuhood force-pushed the stuhood/subsingliant branch from 438e607 to 470ce8e Compare September 10, 2018 00:15

illicitonion approved these changes Sep 10, 2018

View reviewed changes

stuhood force-pushed the stuhood/subsingliant branch from ac9c002 to ae9a71c Compare September 19, 2018 20:22

stuhood added 10 commits September 19, 2018 20:45

Rename Variants to Params, and replace subjects and variants with Par…

f76cc8a

…ams in Node implementations.

Rename SubjectIsProduct to Param in the RuleGraph, and replace sing…

532f82d

…le-subject fields with multi-Param fields.

Fix rendering of an empty RuleGraph.

5538b37

Compute used parameters to monomorphize the graph so that there is ex…

f0ae015

…actly one candidate per RuleEdge(s), and remove Noop.

Drop some unnecessary RootRules.

a6ff9eb

Remove planners.py, which is too reliant on ambiguous rules to contin…

4d1c58f

…ue to support.

Review feedback (and remove SelectVariant, which was missed the first…

12546e5

… time round).

Additional review feedback.

69143fe

Fix clippy lints

7d8a7fe

Adjust rule formatting and fix rule tests.

392504d

stuhood added 2 commits September 19, 2018 20:45

Remove HasProducts and Variants

c82c87b

Add a test of ambiguity.

3eb21d1

stuhood force-pushed the stuhood/subsingliant branch from ae9a71c to 3eb21d1 Compare September 20, 2018 03:46

Use smallvec.

8299248

stuhood merged commit c99cb60 into pantsbuild:master Sep 20, 2018

stuhood deleted the stuhood/subsingliant branch September 20, 2018 20:37

stuhood mentioned this pull request Sep 20, 2018

Design v2 Target API #4535

Closed

stuhood mentioned this pull request Apr 2, 2019

Replace SingletonRule with zero-parameter @rules #7479

Merged

stuhood mentioned this pull request Apr 20, 2019

Update engine README for Params #7600

Merged

Merge subjects and variants into Params, and remove Noop #6170

Merge subjects and variants into Params, and remove Noop #6170

Conversation

stuhood commented Jul 17, 2018 • edited Loading

Problem

Solution

Result

illicitonion 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

stuhood Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

stuhood commented Jul 25, 2018 • edited Loading

illicitonion 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

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

baroquebobcat 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

ity left a comment

Choose a reason for hiding this comment

stuhood commented Sep 10, 2018

illicitonion left a comment

Choose a reason for hiding this comment

stuhood commented Jul 17, 2018 •

edited

Loading

stuhood Jul 20, 2018 •

edited

Loading

stuhood commented Jul 25, 2018 •

edited

Loading