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 API for root requests with multiple parameters #6478

Closed
4 tasks done
stuhood opened this issue Sep 10, 2018 · 11 comments
Closed
4 tasks done

Add API for root requests with multiple parameters #6478

stuhood opened this issue Sep 10, 2018 · 11 comments
Assignees

Comments

@stuhood
Copy link
Member

stuhood commented Sep 10, 2018

In order to implement #5869, root requests will need to provide multiple input Params. #6170 added the ability for Nodes to have multiple parameters, but did not yet expose that via Get or Scheduler.product_request.

This issue should add the ability to make a root product request with multiple Params, but it should probably not expose that ability in the context of yield Get (since that does not block #5869, afaik).

To avoid a tight coupling where callers need to know exactly what Params their callees need (both in a root Scheduler.product_request call and in a Get), Params which are not used by a subgraph should be ignored. As an example, the OptionsParseRequest that will be passed into @console_rule execution might be ignored by a particular subgraph... but the caller should not need to know that, which will allow it to safely pass it for every Scheduler.product_request.

This issue should also handle updating the engine README for the new UX, and replacing the mentions of Variants.


This PR will primarily involve addressing a few TODOs from #6170:

  • Safely constructing instances of Params containing keys with distinct TypeIds will require adding a constructor that validates that the input Keys' types are distinct (and failing politely otherwise).

  • Methods between Scheduler.product_request and the TODO in RuleGraph::find_root_edges will need to be updated in order to pass down Params collections, rather than individual "subject" Keys.

  • All usage of the Params::expect_single method should be replaced with consuming the relevant Param: since Params contain distinct Keys per type, this will usually involve calling Params::find(type_we_need) and asserting that it is present.

  • Finally, to avoid propagating Params into subgraphs where they are not required, all of the constructors of Select should use rule_graph::Entry::params() to filter the set of Params (probably by adding a helper method to Params) that they are given to just what is actually required by the Entry. This will resolve a TODO in the Select constructors and half of the TODO in gen_get (the other half will be resolved when we add UX for passing multiple Params with Get).


At the end of this PR, you should be able to call something like:

# the subjects argument becomes a params argument which is a list of "params" dicts:
scheduler.product_request(HydratedTargets, [{Specs: Specs(..), OptionsParseRequest: OptionsParseRequest(..)}, ..])
# OR
# the subjects argument becomes a params argument which is a list of "params" tuples:
scheduler.product_request(HydratedTargets, [(Specs(..), OptionsParseRequest(..)), ..])
# OR
# the subjects argument becomes a params argument which is a list of a new Params datatype:
scheduler.product_request(HydratedTargets, [Params(Specs(..), OptionsParseRequest(..)), ..])
# etc

...to trigger a @rule that requires two inputs.

@stuhood stuhood added the engine label Sep 10, 2018
stuhood pushed a commit that referenced this issue Sep 20, 2018
### 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 `@rule`s 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 `Noop`s, 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.
@ity ity self-assigned this Sep 28, 2018
@stuhood stuhood assigned stuhood and unassigned ity and stuhood Oct 6, 2018
@stuhood
Copy link
Member Author

stuhood commented Oct 6, 2018

Considered working on this for a bit, but given the time I put into expanding the description, it would be good if someone else could grab it this week.

@ity ity self-assigned this Oct 12, 2018
@ity
Copy link
Contributor

ity commented Oct 17, 2018

@stuhood trying to show this with an example. at the end of this change, we will go from:

from (subject, product) pairs:
subject = Specs(dependencies=(SingleAddress(directory=u'', name=u'compiler-interface'),),matcher=SpecsMatcher(tags=(), exclude_patterns=()))
product = <class 'pants.engine.legacy.graph.TransitiveHydratedTargets'>]

to (list of subject/params, product) pairs:
subject = [Specs(dependencies=(SingleAddress(directory=u'', name=u'compiler-interface'))), Specs(dependencies=(SingleAddress(directory=u'', name=u'zinc-extractor'))]
product = <class 'pants.engine.legacy.graph.TransitiveHydratedTargets'>]

So, on the Python side we would have to accumulate the subject entries in a way that they are distinct before they are even crossing over?

will require adding a constructor that validates that the input Keys' types are distinct (and failing politely otherwise).

or when writing this ^ were you thinking of letting the invocations come through and have validation later?

@stuhood
Copy link
Member Author

stuhood commented Oct 17, 2018

at the end of this change, we will go from...

Hm... I don't think so.

It's more like going from our current API, which is:

# the subjects argument is a list of a subject values
scheduler.product_request(HydratedTargets, [Specs(..), ..])

to one of the syntaxes mentioned in the description. Expanding them a bit here and using HydratedTargets and Specs:

# the subjects argument becomes a params argument which is a list of "params" dicts:
scheduler.product_request(HydratedTargets, [{Specs: Specs(..), OptionsParseRequest: OptionsParseRequest(..)}, ..])
# OR
# the subjects argument becomes a params argument which is a list of "params" tuples:
scheduler.product_request(HydratedTargets, [(Specs(..), OptionsParseRequest(..)), ..])
# OR
# the subjects argument becomes a params argument which is a list of a new Params datatype:
scheduler.product_request(HydratedTargets, [Params(Specs(..), OptionsParseRequest(..)), ..])
# etc

So, on the Python side we would have to accumulate the subject entries in a way that they are distinct before they are even crossing over?

No: it's only the members of an individual "Params" instance that need to be distinct. In the expanded example above, one instance of Params contains Specs and OptionsParseRequest (which are distinct types).

or when writing this ^ were you thinking of letting the invocations come through and have validation later?

Since this can fail if I were to do something like Params(Specs(..), Specs(..)) (ie, one set of Params with two instances of the same type), we do need to do validation. But if you want to just have it panic for now, we can leave a TODO about cleaning it up.

@ity
Copy link
Contributor

ity commented Nov 6, 2018

this got put on the backburner in lieu of other higher priority issues - picking this back up tomorrow

@stuhood
Copy link
Member Author

stuhood commented Nov 6, 2018

Thank you!

@ity
Copy link
Contributor

ity commented Nov 10, 2018

i will get this branch to an unfinished but working state this weekend and post an update to this so its easier for anyone who wants to pick it up

@ity
Copy link
Contributor

ity commented Nov 12, 2018

Added my old wip branch here - https://github.com/ity/pants/tree/ity/p3 If I get some more time today, I will clean it up, otherwise these are all the changes I had so far against this issue.

@stuhood stuhood unassigned ity Nov 12, 2018
@stuhood stuhood changed the title Add UX for root requests with multiple parameters Add API for root requests with multiple parameters Nov 13, 2018
@stuhood stuhood self-assigned this Nov 13, 2018
stuhood pushed a commit that referenced this issue Nov 27, 2018
### Problem

`Params::expect_single` was a compatibility API that was meant to simplify the transition from "Node has a single Subject" to "Node has a set of Params", but usage of that API inherently blocks exposing the ability to pass multiple `Params` in a run (and thus blocks #6478).

### Solution

Remove the three distinct usages of `Params::expect_single` by:

1. simplifying the signature of `scheduler_execute`, which was returning a copy of the subject that was unused anyway
2. directly displaying `Params` rather than looking for a single subject value
3. inlining `Select::gen_node` and resolving a TODO to consume `Entry::Param` and `Params::find` to expect parameter values

### Result

Progress toward #6478, and a ~5% speedup due to taking better advantage of the static rule_graph in `Select`.
@stuhood
Copy link
Member Author

stuhood commented Nov 27, 2018

#6766 landed to address the third and fourth paragraph from the description. Have converted to checkboxes and ticked them to make that clear.

I'm going to put this down for the evening to more directly investigate what blocks demoability.

@stuhood stuhood removed their assignment Nov 27, 2018
@stuhood
Copy link
Member Author

stuhood commented Nov 27, 2018

I'm going to put this down for the evening to more directly investigate what blocks demoability.

Annnd, after a brief investigation, it looks like this might still be the largest blocker for demoability. Resuming.

@stuhood stuhood self-assigned this Nov 27, 2018
@stuhood
Copy link
Member Author

stuhood commented Nov 27, 2018

I might start tackling #5869 concurrently with this issue, in order to better experiment with the API.

@stuhood stuhood removed their assignment Nov 28, 2018
@stuhood
Copy link
Member Author

stuhood commented Nov 28, 2018

Putting this down, because it looks like #5869 is not blocked on it.

@stuhood stuhood self-assigned this Dec 3, 2018
stuhood pushed a commit that referenced this issue Dec 6, 2018
### Problem

Although consumption of multiple `Params` is now supported, there is currently no way to actually provide them to a request. See #6478 for more info.

### Solution

Expose and test the ability to pass in a small `Params` datatype as a subject, which is flattened into a corresponding set of rust-side `Params` in `add_root_select`. 

### Result

Fixes #6478.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants