Proposal: Introduce rule-name-centric calling convention, and deprecate return-type-centric Get

[stuhood]

tl;dr: This issue proposes introducing a syntax to call @rules by name, and deprecating the Get-by-return-type syntax in almost all cases (@unions TBD).

Motivation

Currently, rather than actually referring to the @rule that will be called, Gets use a return-type-centric syntax (await Get(SomeReturnType, SomeArgument(..))). But as our set of plugins has expanded and our understanding of @unions 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:

1. choosing the @rule implementations to use for:
   1. each positional argument to a @rule
   2. each of a @rule's Gets
2. determining the set of Params which must be part of the runtime graph Node for a @rule, because they will be used (transitively) to compute its dependencies

That second aspect of @rule graph solving (determining the set of Params 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 @rules. But the first aspect (deciding which @rules 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" @rules (currently represented as @unions) 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 @unions is more maintainable and easier to reason about than attempting to swap out @rules. And the cost/complexity of rule graph solving means that:

1. @rule graph error messages remain incomprehensible
2. startup time for pantsd is noticeable (with enough @rules installed, even in --release mode)
3. feature development is inhibited by the inability to make fundamental changes to @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 @rules by name (@unions TBD).

Syntax

One syntax for accomplishing this would be to adjust the @rule decorator to convert the function it is applied to into a Callable with adapted arguments, which returns a Get-shaped type. For a @rule like:

@rule
async def my_rule(arg1: Arg1, arg2: Arg2) -> ReturnType:
    return ReturnType()

Callsites could allow positional arguments and keyword arguments, which would be typed. To use the existing relevant portion of Get syntax to provide any (transitive) parameters for the @rule call, they would use a new **implicitly() function/syntax (which "reads" similar to what it does: fill in the remaining arguments to the @rule):

# Equivalent to `Get(ReturnType)` (i.e. no arguments to `Get`). All arguments would be
# computed from Params which were already in scope:
await the_rule_to_call(**implicitly())

# Using a positional arg. Roughly equivalent to `Get(ReturnType, Arg1())`: the remaining arguments would be
# computed from Params which were already in scope:
await the_rule_to_call(Arg1(), **implicitly())

# Two positional args. Roughly equivalent to `Get(ReturnType, {Arg1(): Arg1, Arg2(): Arg2)`. Note that
# `@rule` graph solving is still necessary for this case, because the called `@rule` may have `Get`s
# which have additional dependencies.
await the_rule_to_call(Arg1(), Arg2())

# Equivalent to `Get(ReturnType, Arg1())`:
await the_rule_to_call(**implicitly(Arg1()))

# Equivalent to `Get(ReturnType, {Arg1(): Arg1, Arg2})`:
await the_rule_to_call(**implicitly({Arg1(): Arg1, Arg2(): Arg2}))

[thejcannon]

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?

[thejcannon]

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?

[stuhood]

If the argument type did not match positionally and needed to be converted, it would need to be placed after the ellipses, using roughly the existing Get syntax. i.e. this example from the description:

await the_rule_to_call(..., Arg1())

[thejcannon]

I think for clarity that should be Arg3 then? And state somewhere there's a rule for Arg3 -> Arg1?

[kaos]

#18905 (comment)

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.

[kaos]

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 @unions to be the only pluggable alternative.

I'm in favour of adding support for this call rule-by-name convention, but I'm vary of deprecating the Get-style call rule-by-return-type at the same time.

[stuhood]

Those examples are mostly accurate, except that your Gets would need the return type, which in the proposal is inferred from the return type of the named rule:

# Anything after ellipsis is equiv. with regular `Get`, but the return type is not mentioned:
await the_rule_to_call(..., *args)
await Get(ReturnType, *args)

[kaos]

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 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.

(edit: this is now #18905 (reply in thread) )

[kaos]

It is only in order to be able to deprecate call rule-by-return-type that an alternative for @unions is needed to be able to use the call rule-by-name, so for that I have this idea:

As there is no single rule to call by name for a @union, why not go with what we have, the union type:

@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.

[stuhood]

Possibly... usually @union callsites also mention the expected return type (because the @union is an input type), which you wouldn't have in this example:

await Car(Volvo(...))

[thejcannon]

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:

async foo(input: int) -> FrozenDict[...]: pass
async bar(input: int) -> FrozenDict[...]: pass

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.

[stuhood]

Correct.

[thejcannon]

Oh wow. Yeah OK, uhhh let's figure this thing out 😄

[cognifloyd]

I've only seen the ellipses ... used in type stubs. Is using this when calling a function valid python syntax?

How does this interact with type-aware tooling? Will we have to add # type: ignore all over the place? Or will this somehow make mypy or IDE auto complete somehow more helpful?

[thejcannon]

Yeah type-checkers will be the issue here. Nothing us clever folks can't solve though 😉

[stuhood]

I believe that it is possible for it all to be type safe via proper typing of the @rule decorator: https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators

[stuhood]

...certainly the trailing ellipses and Get syntax can be. Whether the optional positional args can be will be trickier.

[stuhood]

But honestly, if positional args are not supported for by-name @rule invokes, that probably wouldn't be too much of a hardship.

[thejcannon]

In hindisght, this should've been obvious...

[benjyw]

This is great!

Re "There is no hardship involved in calling functions by name" - I think that is overly bullish. It does require knowing those names, whereas previously you only had to know about types (which you still need to know about). So it makes the surface area of our API dramatically larger. Previously all you had to know was that Type A can be converted to Type B, now you have to go look up the name of the rule that does so.

I think this is still worth it for the huge benefits, but let's not gloss over the cost.

Also, it means we can no longer change names at will, although that rarely happened anyway so truly is no hardship.

We do have a one-time opportunity to change rule names, so we should probably come up with a naming convention for them that makes them easier to find/grok?

[cognifloyd]

Maybe we can extend the interface so that we can call rules directly AND use Get.

Whenever calling rules within the same backend, the rules should just call each other.
If using core rules, or rules from other backends, Get is a nicer interface so that you can continue treating the rules as a black box.

[stuhood]

Maybe we can extend the interface so that we can call rules directly AND use Get.

Whenever calling rules within the same backend, the rules should just call each other. If using core rules, or rules from other backends, Get is a nicer interface so that you can continue treating the rules as a black box.

Get comes at a high cost though: that's the key reason for this proposal. Rule graph solving has become untenably slow/complicated. We are additionally paying the cost of constructing/defining disambiguating wrapper types: many @rules would be completely fine directly returning, say, FrozenDict, rather than wrapping in other types.

But to your point: the only difference in opaqueness in the proposal is that you go from needing to memorize a return type name (await Get(ReturnType, ...)) to needing to memorize/learn the implementing @rule name (await rule_name(...)). So I would argue that it is similarly opaque.

[benjyw]

For consistency, it would be good if MultiGet were now spelled concurrently() or something like that:

await concurrently(
     rule1(...),
     rule2(Arg1(), ...),
)

[stuhood]

Looping back on this: for now, I have not changed the name of MultiGet, but #20011 added support for using MultiGet with by name calls. We should probably still rename it (possibly even as part of #20572), but I haven't gotten to it.

[benjyw]

Also, in this proposal are the pre-ellipsis args truly positional?

Today in await Get(ReturnType, Arg1()) Arg1 needn't be the first arg of the rule, params can fill in any other args around it.

[thejcannon]

We could get more clever.

Something like:

• make every argument keyword only (and change the engine for it)
• the request argument uses a new generic: ConvertibleTo[RequestType] the generic is just RequestType (so it can be type-checked as such) but it's a hint to callers that can provide either a RequestType or something convertible to it

[stuhood]

To revise my answer slightly: more than just "familiarity", I would say that symmetry with how you call "rule helpers" (i.e., plain async functions which happen to yield Gets currently) is good.

If what we are able to do is reduce the magic involved in calling a @rule to "the remaining arguments are filled in automatically, and the result is memoized", then it will be much easier to toggle between @rules and using plain async calls... which we should likely be using a lot more of to reduce memoization and object creation overheads.

[benjyw]

So I'm not sure I understand the distinction between await the_rule_to_call(Arg1(), ...) and await the_rule_to_call(..., Arg1()) ...

[stuhood]

If we chose to allow positional args, as originally proposed at the top of this discussion, the behavior would be as described at the top of this discussion: await the_rule_to_call(Arg1(), ...) and await the_rule_to_call(..., Arg1()) would be equivalent, because Arg1 is the first argument of the rule being called. The idea would be that all of the arguments before the ellipses must be valid positional args to the @rule, while the syntax after the ellipses would effectively be approximately "type-driven and possibly-converted kwargs" (the same as the current Get syntax).

I'm not sure that we can make pre-ellipses positional args mypy checkable though, so it might not actually be worth doing: unclear.

[benjyw]

Yeah, I wonder if the pre-ellipsis is worth it, in terms of extra confusion. And in that case do we in fact need the ellipsis at all?

[thejcannon]

I'll also point out this eliminates a class of dev mistakes where you copy/paste some code and forget to change the rule names

[kaos]

didn't think this was an issue no more. In different files it's no issue (right?) and in the same file it will now throw an error at load time.

[thejcannon]

The traces come back misleading

[stuhood]

One other potential syntax to throw into the mix would be something like:

await the_rule_to_call(**implicit())

... where implicit() (or some other name: bikeshed TBD) would take optional arguments similar to Get's arguments. Might be slightly less magical than the literal ellipses idea.

[thejcannon]

I think you can get away without the ellipsis if we did the keyword thing, no?

[benjyw]

Or even without the keyword thing if we don't bother with positional args and always do:

await the_rule_to_call(Arg_x(), Arg_y())

Where any missing args are filled in, and so the order of Arg_x() and Arg_y() don't actually matter?

[stuhood]

FWIW, this can be made type-safe with positional args if the magic implicit() keyword/function doesn't have a return type:

def implicit():
    pass

def my_rule(arg1: str, arg2: int) -> str:
    return "yep"

my_rule(**implicit())
my_rule("one", **implicit())
my_rule("one", 2, **implicit())
my_rule(2, "one", **implicit())

example.py:29:17: error: Argument 1 to "my_rule" has incompatible type "int"; expected "str"  [arg-type]
            my_rule(2, "one", **implicit())
                    ^
example.py:29:20: error: Argument 2 to "my_rule" has incompatible type "str"; expected "int"  [arg-type]
            my_rule(2, "one", **implicit())
                       ^
Found 2 errors in 1 file (checked 3 source files)

[stuhood]

Or even without the keyword thing if we don't bother with positional args and always do:

No positional args means no useful type checking, which is a bummer I think.

[thejcannon]

I think this might be a better path forward.

I want to make sure the typing is completely sound and editors can join in.

I also want to see if we can eliminate request types this way as well

[thejcannon]

I'm guessing this also doesn't jive well with all the "inner rule generation" we do for fmt/fix/lint/python lockfiles?

[thejcannon]

Although I think every example so far is unions...

[thejcannon]

OK so throwing my idea in. Two changes:

• All rules declare they take by-keyword-only parameters. Calls are made by-keyword. (Or something equivalent to make calls "look" normal, and to eliminate the awkward dictionary call syntax)
  • So maybe we allow/enforce if there is a parameter not computable from the root (the request type, essentially), it comes first. I'm ok with punting on this and just continuing (and maybe strengthening the convention)
• We change the rule scraper to deduce the type of the argument using the following criteria:
  • If the argument is a type constructor, it is assumed to be of that type
  • If the argument is a called-callable, it is assumed to be the type that the callable is annotated as returning
  • Otherwise, the argument must be a variable (cannot be any other form of expression) and the variable MUST have been annotated with a PEP 526 annotation. The annotation type is the type.

With those in place, I think we can achieve a syntax that looks like (copying your example):

await the_rule_to_call()
await the_rule_to_call(request=Arg1())
await the_rule_to_call(arg1=Arg1(), arg2=Arg2())  # This was previously the dictionary calling syntax

(I think we ought to set a semi-strict convention on request parameter names, but that relates to #18905 (comment) in that we have a one-time chance to rename things)

[kaos]

This makes me think of one minor downside to this, and that is that naming things is close to the top on the list of the most difficult things in computer science. So exposing more names in the API will be a cause for headaches at some point. Not saying this is reason to not do this, just worth considering I think.

[stuhood]

Where does the request= name come from in this example?

await the_rule_to_call(request=Arg1())

What would it look like to pass an argument which was going to be converted from the actual passed argument type? That still wouldn't be type safe, I imagine...

[thejcannon]

From the definition of the_rule_to_call. And yes all type safety is/was already off the table because of conversion semantics.

Unfortunately I don't think there's a way in the type system to model this at the call-site. Ideally we keep the names and map the types to Any, but...

[benjyw]

Another thing to remember here is that callers of rules must now import them, so moving them around breaks the plugin API where it did not before. Worth it, but not "no hardship".

Also, does this potentially mean not needing to register rules in register.py, if all rules are explicitly imported anyway?

[thejcannon]

Good point. Rule collection might look different as well. That's probably a good thing?

[kaos]

I kind of liked the disconnect between a type and the rule implementation.

[thejcannon]

I've thought about that some. I like this way better, and I'd argue it's "more correct".

• Human correctness: The code reader is able to reason about the flow of code better, and more importantly tools can assist (e.g. "jump to definition" in your IDE)
• Graph Correctness: Currently if the request type and rule are in two different files, --changed values, including transitive, don't do the right thing if you edit a rule. Even with this the graph will still be wrong, but is a closer approximation to the truth

[kaos]

I don't get the correctness arguments.

• Human correctness: The code reader is able to reason about the flow of code better, and more importantly tools can assist (e.g. "jump to definition" in your IDE)

I don't want to reason about the flow of code (but about the flow of data, which is about the types involved) I want to focus on an isolated part of a bigger question trusting that the engine will provide the rest.

Either way, I don't see these contrasts as more or less correct than the other.

Graph Correctness: Currently if the request type and rule are in two different files, --changed values, including transitive, don't do the right thing if you edit a rule. Even with this the graph will still be wrong, but is a closer approximation to the truth

This would be during Pants/plugin development only, I guess? What do you mean with that the graph will be wrong?
If I edit a rule, and I want to run tests for all changed rules, tests that depend on that rule will be included with --changed, right?

[stuhood]

As referenced on #19053, we should be making a lot more memoization decisions than we currently are.

In general, we should (be able to) make a lot more memoization decisions than we currently are (where all https://github.com/rule invokes are memoized, and only "rule helpers" are not). That way, if something is likely to only be used once, we avoid the cost of memoization, and only pay the cost of the dependency injection. I'll open a separate ticket about that.

That's lightly relevant to this issue, in that this issue changes memoized-by-default call sites to look a lot more like "rule helper" callsites (which are not). Another potential connection is that in many cases, we wrap in a return type for the purposes of call-by-type callsites: removing them by using call-by-name instead would reduce memory overhead.

It's probably still the case that memoization should be disabled by the callee rather than the caller though.

[stuhood]

#20011 made rule helper and by-name calls identical, including support in MultiGet.

[stuhood]

Thanks folks: have opened #19730 to cover implementation.

[stuhood]

Reopening, because we might also need a new MultiGet syntax.

While:

await MultiGet(
  one_rule(2, "one", **implicit()),
  two_rule("two", **implicit()),
)

... might be made to typecheck, MultiGet was already a bit awkwardly named, let alone once there is no more "Get" in view.

[Eric-Arellano]

await ConcurrentRules()?

[thejcannon]

asyncio calls this gather: https://docs.python.org/3/library/asyncio-task.html#asyncio.gather

Not sure it makes much sense here though.

All or Concurrently wrapper classes would be easy to read.

[stuhood]

Yea, I like All since it's short (and because we had also discussed introducing a FirstOf type for speculation).

[stuhood]

#20011 added support for using the call-by-name syntax with MultiGet, but did not yet change the name: choosing a new name will be a blocker for any large-scale rewrite to the new syntax though.

[stuhood]

#19797 now contains two or three example usages of the **implicitly(..) syntax (which was adopted from Scala as a placeholder), and we should give some more thought to what to call it.

The name of this function needs to be:

1. short
2. self-explanatory OR easily searchable (to find an explanation of what it does)

Scala 3 has renamed "implicit parameters" to "contextual parameters" (i.e. "parameters that come from the context"), which is definitely an improvement. But Scala declares contextual parameters in the callee rather than at the caller, so the equivalent might be:
**contextually(..)... which is probably too long.

[benjyw]

**context(...) ?

[benjyw]

It is somewhat descriptive

[benjyw]

I've realized I don't know how this affects union registration and use. Currently we register a union of concrete request types and iterate over them via UnionMembership:

results = await MultiGet(
        Get(LintResults, LintTargetsRequest, concrete_request)
        for concrete_request in concrete_requests
    )

What is the call-by-name-equivalent?

[stuhood]

TBD! It might still be type driven.

[sureshjoshi]

@stuhood Can we close this discussion as resolved as we're in the implementation phase?

[grihabor]

Just to clear things out: with this new design it won't be possible to hook into the existing backend rule anymore, right?

Here is what I mean. Imagine you have a backend, let's say docker backend, and you want some custom logic in one of the rules that docker backend provides. Right now what I can do is I can create a new plugin, import all rules from the docker backend except for the one I want to customize, and declare my own new rule with the same signature. This way I can reuse all docker backend logic and customize a single rule. But with call-by-name I won't be able to do it, right?

[kaos]

I think you're right. And things like this is why I'm not 100% happy with the call-by-name move as I rather liked the loose coupling we get with the typed rule system. But I realize it all comes with trade offs, and I'm not in a position where I can invest a lot of time to figure out a better approach here.. ;)