-
-
Notifications
You must be signed in to change notification settings - Fork 646
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
Proposal: Introduce rule-name-centric calling convention, and deprecate return-type-centric Get
#18895
Comments
So just to be clear, we still aren't calling the rule. We're just shifting the convention and the magic to look closer to normal code? |
Oh and how does this work with providing an arg that isn't the request type, but is convertible through the engine to the request type? |
I like the syntax as it feels and looks more concise. The described examples are a bit vague to me still though. Summarizing my understanding of it yields: # Anything after ellipsis is equiv. with regular `Get`
await the_rule_to_call(..., *args)
await Get(*args)
# Anything before ellipsis will be complemented with in scope params as needed
await the_rule_to_call(*args, ...)
await Get(*args) # adds params from scope =>
Get(ReturnType, {*args, Param1: param1, Param2: param2})
# Not using ellipsis must provide all required params for the call as the params in scope will not be consulted; this would be the preferred recommended way to call rules in order to fully specify the requirements?
await the_rule_to_call(*args)
await Get(*args) # w/o added params from scope Was this somewhat accurate? IIUC this merely directs the rule graph building to cut down the number of alternative rules for a dependency key, reducing the exponential explosion of variants going into monomorphization, at the cost of not being able to swap out rules by replacing backends, leaving I'm in favour of adding support for this call rule-by-name convention, but I'm vary of deprecating the |
I imagine the same thing would hold as today for this case, i.e. the engine would need to go out and find the chain of rules to use to get the required type from what you provided. |
It is only in order to be able to deprecate call rule-by-return-type that an alternative for As there is no single rule to call by name for a @union
class Car:
pass
class Volvo:
pass
Union(Car, Volvo)
# Old style:
await Get(Car, Volvo(...))
# New style, using the union type as the rule to call:
await Car(Volvo(...)) With the added benefit that it will be very clear at the call site that this is no regular rule we're calling but a union type. This has some implementation implications, but I think they are manageable. |
Hey @stuhood can we convert this to a discussion so we can reply in threads? |
Also out of curiosity, would this open the door for not using type-based returns in the engine for solving rule graph queries? We could endow the engine's representation of the return type with the name of the rule for differentiation. E.g. if we know the name of the rule, the following definitions are unambiguous in which one to use:
The engine creates a distinct type for each using the rule name(space) as a delineator, and rule parsing knows which should be used because now we're using the rule name. |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
tl;dr: This issue proposes introducing a syntax to call
@rule
s by name, and deprecating theGet
-by-return-type syntax in almost all cases (@union
s TBD).Motivation
Currently, rather than actually referring to the
@rule
that will be called,Get
s use a return-type-centric syntax (await Get(SomeReturnType, SomeArgument(..))
). But as our set of plugins has expanded and our understanding of@union
s has matured, it's becoming clearer that one of the motivations for that syntax is probably no longer valid."
@rule
graph solving" (documented here) refers to the process of:@rule
implementations to use for:@rule
@rule
'sGet
sParam
s which must be part of the runtime graphNode
for a@rule
, because they will be used (transitively) to compute its dependenciesThat second aspect of
@rule
graph solving (determining the set ofParam
s that a@rule
needs in order to run) is something which cannot practically be done by hand, and so it continues to be an essential component of composing@rule
s. But the first aspect (deciding which@rule
s will be called) can definitely be done by hand, since at a fundamental level, it can be reduced to the same sort of function calling convention that is used in most programming languages: calling a function by name.There is no hardship involved in calling functions by name, but when the v2 engine was originally created, particular focus was paid to pluggability and extensibility. Avoiding calling functions by name meant that the implementations of your dependencies could be swapped out without mocking, and that "abstract"
@rule
s (currently represented as@union
s) would be on equal footing with other calls.But it's become clear in the intervening time that explicitly introducing and tracking pluggability upstream via a set of declared
@union
s is more maintainable and easier to reason about than attempting to swap out@rule
s. And the cost/complexity of rule graph solving means that:@rule
graph error messages remain incomprehensiblepantsd
is noticeable (with enough@rule
s installed, even in--release
mode)@rule
graph solving (e.g. Re-enable concurrent runs for pantsd in v2 #7654 (comment))To simplify and remove roughly half of the magic involved in rule graph solving, we should deprecate the
Get
-by-return-type syntax in favor of calling@rule
s by name (@union
s TBD).Syntax
One syntax for accomplishing this would be to adjust the
@rule
decorator to convert the function it is applied to into aCallable
with adapted arguments, which returns aGet
-shaped type. For a@rule
like:Callsites could allow positional arguments before a literal
...
(ellipses). After the ellipses, they would use the existing relevant portion ofGet
syntax to provide any (transitive) parameters for the@rule
call:The text was updated successfully, but these errors were encountered: