[engine] Introduce static analysis model and replace validator with it
Review Request #4251 — Created Sept. 28, 2016 and submitted
|kwlzn, stuhood, yujiec|
Introduces a Rule Graph that only contains the rules that cannot result in a Noop due to missing selector values. Replaces the existing validator with one backed by this new model. This means that for instance, intrinsics, or product graphs that depend on intrinsics, but are ultimately
Nooped can be eliminated early.
There's still a number of TODOs left in the patch, but it's to a point where I feel like I need feedback before making further changes.
* better error reporting over the previous implementation: The validator will now report transitive failures. Subject type match failures are also reported
BeforeFound 1 rules with errors: (A, (Select(B),), noop) There is no producer of Select(B)
AfterRules with errors: 1 (A, (Select(B),), noop): no matches for Select(B) with subject types: SubA
* cycles: I've got some ideas, but this has gotten pretty big already. Cycles won't cause non-termination because the graph generation algorithm only visits each rule/subject-type entry once.
* noops caused by tasks that can return
* actually generating nodes base on the graphs
Follow on work:
* performance. There are improvements to the model generation that could be done. But, I want to split that work out from this initial cut.
* integration into the scheduler. After this lands, I want to adapt the scheduler to use it for pre-filling dependencies of nodes to execute, and ultimately to simplify or eliminate SelectNodes.
Introducing and running tests for the graph and validator locally. CI passed at https://travis-ci.org/pantsbuild/pants/builds/165056642
This looks great, and I can begin to see how it would replace most of the Node bodies. As you said, there is more to do to figure out how to handle portions that are conditional on runtime values rather than types.
In general, I'd love to see a few more comments. But am shipping now because this is exceedingly well tested, and will certainly be changing further.
At a fundamental level, it's not clear what the "node" and "edge" types in this graph are (Rules and Selectors, I guess? Or is it bipartite between Rule and Selector nodes, with unlabeled dependency edges?), it would be good to expand a docstring out to explain that if possible.
Seems like natively supporting "
subject_type==Nonemeans 'any type'" in Diagnostic would be cleaner, as you could render a more useful error message for that case.
A reminder: happy to temporarily drop support for producing the explicitly-requested Variants class midgraph if that makes this easier. I think sealing the graph as you're planning is significantly more important at the moment, and we can revisit to find the right way to do this in the future.
Changes look awesome. I am still learning the Rule thing. So most of my comments are actually questions.
I am confused here. Why do we want to add constraint here, given that all types in the contraint is already added?
For intrinsic product you filter with subject_types. But for task_product subject_type is not used for filtering.
Is it possible that for a certain subject_type, a task cannot be executed thus the product of that task should not be in the return set?
So the check you did in __repr__ implies that self.value may not be a type instance, in that case, self.value.__name__ may result in AttributeError.
Maybe convert this to a doc string? also the comment is kinda confusing to me. Is it saying, if the input rule is unfulfillable then this RuleEdges becomes unfulfillable?
Should this be moved to line 462?
The line immediately following this comment is about handling the corner case which makes the comment kinda confusing.
also, nit: "If"
- Add marker classes
- Add docstrings
- Use None as Any
- Remove conditional in repr for subject is product
- Maintain selector_path -> dep in edges
- Allow partially fulfilled field_type sets
Revision 2 (+1106 -148)