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
Document rule graph construction and open issues #10690
Document rule graph construction and open issues #10690
Conversation
…ding here: maybe on the docsite. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
a2299d0
to
fd55ba3
Compare
A `Rule` is a function or coroutine with all of its inputs declared as part of its type signature. The end user type signature is made up of: | ||
1. the return type of the `Rule` | ||
2. the positional arguments to the `Rule` | ||
3. a set of `Get`s which declare the runtime requirements of a coroutine, of the form `Get(output_type, input_type)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reader may not intuit what a Get
is. It's worth elaborating that a Rule
can request further input at runtime, by yielding a Get
object back to the engine, so these also represent part of the input signature, along with the positional arguments, yadda yadda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that that is self explanatory by virtue of being part of the signature... not sure how to clarify it.
src/rust/engine/rule_graph/README.md
Outdated
2. the positional arguments to the `Rule` | ||
3. a set of `Get`s which declare the runtime requirements of a coroutine, of the form `Get(output_type, input_type)` | ||
|
||
In the `RuleGraph`, these are encoded in a [Rule](https://github.com/pantsbuild/pants/blob/3a188a1e06d8c27ff86d8c311ff1b2bdea0d39ff/src/rust/engine/rule_graph/src/rules.rs#L76-L95) trait, with a [DependencyKey](https://github.com/pantsbuild/pants/blob/3a188a1e06d8c27ff86d8c311ff1b2bdea0d39ff/src/rust/engine/rule_graph/src/rules.rs#L21-L41) trait representing both the positional arguments (which have no provided `Param`) and the `Get`s (which provide their input type). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You use the term Param
here before defining it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really great, and the level of detail is just right, I think.
What I think would enhance this further, especially to readers not steeped in the problem space as we are, is:
A) Short, precise definitions: Rule, Query, Param, dependency etc. Basically an intro to what the vertices and edges in a rule graph represent.
B) Consistent subsequent use of terminology and definitions. E.g., the term "source" isn't clear.
C) An example.
D) Pseudocode.
[ci skip-build-wheels]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Depending on how much the reader already knows about our domain, we might have to abstract this further/provide more background, but we can deal with that as needed.
A `Rule` is a function or coroutine with all of its inputs declared as part of its type signature. The end user type signature is made up of: | ||
1. the return type of the `Rule` | ||
2. the positional arguments to the `Rule` | ||
3. a set of `Get`s which declare the runtime requirements of a coroutine, of the form `Get(output_type, input_type)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️
Co-authored-by: Benjy Weinberger <benjy@toolchain.com>
Problem
RuleGraph
construction is a very complicated bit of code, and could stand being more widely understood.Solution
Document
RuleGraph
construction, and its open issues. This will also likely be copy-pasted to the docsite in a tucked-away architecture page somewhere.[ci skip-build-wheels]