@rules as coroutines

[stuhood]

Problem

The "complex" Selector types used to select @rule inputs for subjects other than the current subject of an @rule have proven to be entirely too complicated. Additionally, their lack of flexibility meant that we needed to introduce many intermediate types to provide links between @rules in cases where imperative logic was needed for filtering or conditionals.

The only remaining motivating factor for these complex selectors was that they allow for closed-world compilation of the @rule graph, which avoids the need for error checking in rule bodies and provides for easier composition of sets of @rules.

Solution

After experimenting with using coroutines (ie: functions containing yield statements that receive a result on their left hand side), it became clear both that:

1. Coroutines were universally easier to understand than the Selector DSL
2. The dependency requests made by coroutines could still easily be statically checked via a small amount of runtime introspection.

And so, this change removes all usage of SelectProjection and most usage of SelectDependencies in favor of @rule coroutines. It does not yet remove the implementation of those Selectors (although doing so will shed a massive amount of complexity from the codebase).

Result

@rules are still statically checked, but are now much easier to write. Performance is unaffected.

[cosmicexplorer]

I was just looking at this exact rule today to try to understand how to use SelectProjection and this change is RIDICULOUSLY cool imo

[cosmicexplorer]

If no Dir -> AddressFamily path exists in the rule graph, does it error at this line? Would the previous use of SelectProjection have errored out earlier, during the rule graph construction?

[stuhood]

Yep, exactly. And that's the primary downside of this change as it stands: we would lose the validation that all rules are satisfiable.

Have been thinking more about this though, and I think that with some very, very basic parsing (essentially, "capture the RHS of yield statements"), we could get back the static checking... which might be justified.

[stuhood]

...and also, as it stands the exception would be created in the rust code rather than literally on this line. It looks like it is easy to send an exception to the generator though with generator.throw().

EDIT: Hmm, but actually: the downside of this would be that it would introduce the possibility of people accidentally/intentionally implementing error checking. And frankly, I'd rather get the static checks fixed and discourage all error handling in @rules.

[cosmicexplorer]

I think "capture the RHS of yield statements" is a highly reasonable goal, but I don't know if it needs to be done by parsing -- that being said I remember python's ast module is supposed to be not awful? I can't see what the exact code would look like right now, but since we know we can get the desired info we want before invoking the python rule definition (with the previous example using SelectProjection), I think there's hopefully a middle ground where we can specify rule graphs without too much ceremony (probably exactly the same in spirit as parsing for yield statements). I would love to parse some python for yield statements if it gets us there, but I think there might be a neater way -- will think on it.

[stuhood]

This is now resolved: all Get statements in the body of an @rule are statically checked at RuleGraph construction time.

[stuhood]

Parsing yield statements from @rules appears to be straightforward: have added one more commit on top that demonstrates it in a single file.

So I think that with one relatively minor restriction (which technically already existed) we can preserve the static checks; namely: the request/Get/yield would need to include both the product and subject type explicitly. An example syntax might be:

# verbose form: Get(product_type, subject_type, subject)
classpath = yield Get(Classpath, ScalaSources, sources)
# potential shorthand: Get(product_type, subject_type(subject))
classpath = yield Get(Classpath, ScalaSources(snapshot))

EDIT: And after thinking about it for a few minutes more: we'd probably want to parse the calls to the Get/"whatever" constructor rather than the yield statements themselves... that would more naturally allow for (for example) building up a mutable list of requests and then yielding them. And what we need are the arguments to the constructor anyway.
EDIT2: Did that. Looks good.

[illicitonion]

I like it, much cleaner than SelectProjection imo :)

[stuhood]

I've implemented the static checks for Get calls, so this is almost ready to graduate from experiment to recommended API.

The next order of business: bikeshedding. What should Get's final name be?

[illicitonion]

Very cool :) Though I'm not sure I entirely follow the fine details of everything going on.

I'm happy to keep the name Get - we can always change it in the future if we need to (I'm assuming this will be an "unstable" API for a bit :))

My next request would be a few Python unit tests which should this actually in use

[illicitonion]

Seems a little strange for entry to be both Arc'd and clone'd here. My instinct (which may be wrong) is that either it's a cheap to clone type, and we shouldn't be Arcing it, or it's an expensive to clone type, and we should always be Arcing it...

[stuhood]

Yea, I have a separate review that attempts to Arc up the rule-graph, since it is full of tiny Vecs. On the other hand, it does not appear to impact performance in a significant way... so perhaps this would not either. It's very surprising to me that all of the allocation doesn't have a noticeable impact.

[stuhood]

I haven't written any new unit tests, but I updated the documentation in the topmost commit and added validation to the Get constructor, so I believe that this is now reviewable.

[stuhood]

A link to the rendered README: https://github.com/twitter/pants/blob/56dd7bdd4df03cc8e6a7864bf1ab8231a2500656/src/python/pants/engine/README.md

[jsirois]

I like the direction, but I'm not in love with the new syntax / concept / mixed-point dependency injection. Perhaps best to explain with a summary example. From the current change we now have:

class Get(datatype('Get', ['product_type', 'subject'])):
  pass

@rule(UnhydratedStruct, [Select(AddressMapper), Select(Address)])
def resolve_unhydrated_struct(address_mapper, address):
  address_family = yield Get(AddressFamily, Dir(address.spec_path)
  ...

So we have dependency injection through function arguments and, now, co-routine results.

This could alternatively be modeled with just dependency injection through function arguments if we allow for both terminal values and rules (functions) to be injected:

class Rule(datatype('Rule', ['product_type', 'subject_type'])):
  def __call__(subject):
    pass  # IE: Get
  
  def map(subjects):
    pass  # IE: GetMulti

@rule(UnhydratedStruct, [Select(AddressMapper), Select(Address), Rule(AddressFamily, Dir)])
def resolve_unhydrated_struct(address_mapper, address, address_family_rule):
  address_family = address_family_rule(address.spec_path)
  ...

IOW, I'm a Rule author who knows how to operate on certain selected values but I need to enlist the help of other rules imperatively. The proposed syntax change says this more directly I think and allows eliminating AST gymnastics.

Taken further (but importantly ignoring variant selects), this just means @rule becomes nothing but type annotations for a python function; ie: things eventuially could just boil down to:

@rule(UnhydratedStruct, [AddressMapper, Address, Rule(AddressFamily, Dir)])
def resolve_unhydrated_struct(address_mapper, address, address_family_rule):

[stuhood]

Thanks @jsirois , that's definitely an interesting suggestion!

One downside I see is that it removes the trampolining that happens when we yield as a coroutine. @rules actually synchronously calling one another would put us back in a python threadstack ~all of the time. Whether this results in a net advantage in terms of time spent outside of the GIL is an open question, because if python functions were to synchronously call into other rules, I believe that the GIL would be released for the length of the call.

Taken further (but importantly ignoring variant selects), this just means @rule becomes nothing but type annotations for a python function; ie: things eventuially could just boil down to:

I believe that it's possible that the @rule declaration can actually be simplified with either of the proposals, but I haven't looked into how variants will affect it either. I'd need to look at more callsites, but it's possible that SelectVariant would become a flavor of Get, which would mean that @rule arguments could be simplified similarly.

EDIT: Although, of course, they would still be coroutines, and thus harder to invoke.

[stuhood]

Another advantage that @illicitonion might point out is that avoiding coroutines would mean that we could more easily use Skylark at some point (which doesn't support them, afaik).

EDIT: Then again, skylark probably doesn't support decorators either.

[jsirois]

I may be missing something here, but couldn't the implementation of the call into the engine be masked, deferring the optimizations for behind-the-scenes work?:

>>> def coroutine(value):
...   result = yield rust(value)
...   yield result
... 
>>> def rust(value):
...   return value * 2
... 
>>> def using_coroutine(value):
...   c = coroutine(value)
...   for r in c:
...     if r:
...       return r
... 
>>> using_coroutine(3)
6

Where using_coroutine is what is passed into the example above as the Rule implementation stub.

[stuhood]

I may be missing something here, but couldn't the implementation of the call into the engine be masked, deferring the optimizations for behind-the-scenes work?:

result = yield rust(value) is still a "python calls rust" situation... so the coroutine would not actually be doing anything useful with the yield. In this PR the @rule yields to the rust code which sends back the value. Again, really not certain that it buys us less time under the GIL. But it's recursion safe at least?

[jsirois]

Finished some discussion about coroutines vs rule stubs injected as values like all other values in the function arguments in slack #engine. I'd still prefer the latter on behalf of the average rule writer, but my instincts may be wrong and I don't strongly object.

[stuhood]

After dancing around the realization a bit, it occurred to us this morning that if we have synchronous calls back and forth between python and rust, we can't have async io. While fully switching to synchronous io via rayon would be a possibility, it would be a pretty massive refactor... and there is a lot of good stuff coming down the pipe for async in rust.

[stuhood]

The top commit now adds a test utility function that makes it easier to test @rules that make Get requests.

[baroquebobcat]

I really like the direction this is going.

The yield syntax in rules is a lot more understandable than the projecting selects.

I'd like to think a bit more about how we can validate that rules construct the Gets in expected ways going forward, but I don't think that blocks getting this in.

I do have a few minor requests about the doc updates, but I don't think they're blocking. I think doing a couple copy edit passes on the docs might be worthwhile as a separate task.

[baroquebobcat]

nit: second @rule needs back ticks

[baroquebobcat]

There's a couple more @rule references here without backticks. Also s/depenencies/dependencies/

[baroquebobcat]

I think it'd make sense to add a paragraph here explaining the FormattedInt, and maybe providing an example of what a subject would be that would cause it to be evaluated. I think discussing subjects here would help the next section do its job of explaining how to change subjects.

[baroquebobcat]

nit: s/instatiated/instantiated/

[baroquebobcat]

This feels like it could use a little more explanation in it. Maybe something like

This `@rule` declares that: "for any Subject for which we can compute `Files`, we can also compute
`ConcattedFiles`". In order to do that, it needs the `FileContent` for each of the files in the selected 
`Files`. It gets those contents by yielding `Get` requests that ask for `FileContent`s for with each 
file as a Subject. Once those have been collected, it concatenates the contents and yields the result.

[baroquebobcat]

This is pretty slick. After we nail down the API further, or as part of doing so, I'd like to see a bunch of tests that cover different ways that Gets are defined and the error cases defined here.

[baroquebobcat]

Perhaps these should raise ValueError?