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

Complete support for variants #4020

Closed
stuhood opened this issue Nov 2, 2016 · 3 comments
Closed

Complete support for variants #4020

stuhood opened this issue Nov 2, 2016 · 3 comments

Comments

@stuhood
Copy link
Member

stuhood commented Nov 2, 2016

The native engine currently has a few stubbings that prevent Variants from working (marked with TODOs mentioning this ticket):

  1. Variant merging is implemented by core::Variants, but not used.
  2. Requesting the Variants product is implemented by nodes::Select, but not merging them (in particular: this will either require lifting the user-defined variants into rust for merging, or pushing down rust's variants to python for merging)
  3. nodes::SelectDependencies does not parse Variants from Address objects (the new path/to:target@key=value syntax) like we used to:
    for dependency in getattr(dep_product, self.field or 'dependencies'):
    variants = self.variants
    if isinstance(dependency, Address):
    # If a subject has literal variants for particular dependencies, they win over all else.
    dependency, literal_variants = parse_variants(dependency)
    variants = Variants.merge(variants, literal_variants)

Additionally, it has recently occurred to me that the RuleGraph gives us the ability to statically determine whether there are any consumers of Variants in a subgraph. This would allow us to eliminate multiple inefficiencies from the original implementation:

  1. We can know statically whether a Variant will be consumed in a subgraph, and so we can avoid propagating Variants in subgraphs that don't consume them (which avoids duplicating Nodes with unused Variant values).
  2. Rather than eagerly computing "default" Variant values at every Select Node (which was removed as part of this ticket), we can compute them only in subgraphs where they might be consumed.
stuhood added a commit that referenced this issue Nov 7, 2016
Applies the review feedback from [r/4270](https://rbcommons.com/s/twitter/r/4270/), and fixes one minor issue with the bootstrap script.

- `exit 1` in `build-support/bootstrap_native` if compilation fails. This allows for iterating on e.g. `./pants --enable-v2-engine list ..` until the native code successfully builds.
- Used `git-ls-files` to determine which files to hash.
- Derive/implement `Default` for Key/Value.
- Whitespace in generic parameter lists.
- Allow for visualizing executions to a directory via a `--native-engine-visualize-to=$dir` option.
- Convert EntryId to a sealed type to avoid accidental random values.
- Improve a few comments.
- Suffix python keywords rather than prefixing them.
- Whitespace fixes.
- Silenced known unused code warning for [4020](#4020).
- Attempt to cache `~/.cargo` in travis.
- Don't nuke `$HOME` in hermetic tests.

Testing Done:
https://travis-ci.org/pantsbuild/pants/builds/173991370

Bugs closed: 4037

Reviewed at https://rbcommons.com/s/twitter/r/4359/
@stuhood stuhood removed the native label Mar 27, 2017
@stuhood
Copy link
Member Author

stuhood commented Jan 11, 2018

cc @baroquebobcat

@stuhood
Copy link
Member Author

stuhood commented May 7, 2018

See #5788, which talks about a challenging/interesting usecase for Variants.

@stuhood
Copy link
Member Author

stuhood commented Jul 25, 2018

Resolving in favor of #5788.

@stuhood stuhood closed this as completed Jul 25, 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.
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

1 participant