Call for proposals: improved @rule display

[stuhood]

Currently, the Display impl for @rules results in:

(A, [UnionWrapper], [Get(A, UnionA), Get(A, UnionB)], a_union_test()) for UnionWrapper

... which does not obviously represent a python function. We should refactor this so that these are more recognizable (ideally without losing any information). Ideally this would be a one-liner.

---

A few potential candidates:

• function shaped, with parameter-selectors in the signature, and Param types in a "body". The Param types might not always match the parameter selectors (if we can compute a Foo from a Bar to provide a param, you'd have Foo in the signature and Bar in the body).

def a_union_test(UnionWrapper) -> A { for UnionWrapper; Get(A, UnionA), Get(A, UnionB) }

• function shaped, with no parameter selectors. Since python doesn't allow overloading, this should be relatively unambiguous. But since rule names are not globally unique, it could be confusing to identify them.

def a_union_test -> A { for UnionWrapper; Get(A, UnionA), Get(A, UnionB) }

• function shaped, with a funky argument list. This feels like it gets into risky territory where folks could mistake the arguments for literal arguments to their function:

def a_union_test(for UnionWrapper; Get(A, UnionA), Get(A, UnionB)) -> A

• the wildcard. Do away with providing a signature at all, and just refer to the code:

tests/python/pants_test/engine/test_scheduler.py:94: def a_union_test(union_wrapper):

[jsirois]

I definitely think the wildcard is most friendly. But with an eye to only py3.6+ support:

tests/python/pants_test/engine/test_scheduler.py:94: def a_union_test(union_wrapper: UnionWrapper): -> A

Including the inner requests at all seems noisy no matter how this is done. To find the problem rule I probably just want the file / signature no matter the language the rule is implemented in.

[stuhood]

I've also been assuming that we could use the same rendering in the rule graph visualization... I think that dropping the other data might not make that feasible:

... which could be fine. Maybe we have different rendering in:

1. error traces / messages
2. introspection/visualization and rule-graph construction failures

[cosmicexplorer]

#7024 has some / a lot of these types of changes already implemented, I should get back to that or just merge it, oops.

[stuhood]

#7024 has some / a lot of these types of changes already implemented, I should get back to that or just merge it, oops.

Would be good to wait for the discussion here to die out a bit first. Adding newlines in rulegraph rendering might be fine, but would not work for CLI rulegraph construction errors... so at least two cases probably.

But if the rendering for "introspection/visualization and rule-graph construction failures" ends up concise enough, maybe it doesn't need newlines...? unclear.

[benjyw]

I like the wildcard proposal too!

[cosmicexplorer]

Adding newlines in rulegraph rendering might be fine, but would not work for CLI rulegraph construction errors

Yes, however, I don't think we should be using the same display for the rule graph and CLI anyway -- I feel like all that information is necessary to describe why each node has arrows to all its neighbors and that PR focuses a lot on e.g. coloration of node by type, which is a really reasonable and also orthogonal line of investigation.

[illicitonion]

I definitely want the return type to be present. Accordingly, @jsirois's modified wildcard sounds good to me.

[stuhood]

Two things!

1. @jsirois's wildcard is very much looking like the winner given his recent work (Deprecate @rule(...) in favor of @rule. #8338)!
2. I realized that in the rule-graph display, rather than rendering the Gets on the @rule's node, we should be rendering them as edge labels! This simplifies things, because we can use "the function signature" as in the wildcard idea.

[benjyw]

Assigning to @gshuflin who is looking into this.

[benjyw]

Note that this is also relevant in various error messages, such as rule graph errors. This is pretty unreadable:

Exception message: Rules with errors: 5
  (Binary, [BuildFileAddresses, Console, Workspace, _Options, OptionsBootstrapper, BuildRoot], [Get(CreatedBinary, Address), Get(Digest, DirectoriesToMerge)], create_binary()):
    No rule was available to compute Get(CreatedBinary, Address) with parameter types (Address+BuildFileAddress+BuildFileAddresses+Console+Digest+DirectoriesToMerge+DirectoryWithPrefixToAdd+DirectoryWithPrefixToStrip+ExecuteProcessRequest+InputFilesContent+InteractiveRunner+LLVM+MultiPlatformExecuteProcessRequest+NativeToolchain+OptionsBootstrapper+OwnersRequest+PathGlobs+Scope+Specs+ToolchainVariantRequest+UrlToFetch+Workspace)
  (CreatedBinary, [HydratedTarget], [Get(CreatedBinary, PythonBinaryAdaptor)], coordinator_of_binaries()):
    No rule was available to compute Get(CreatedBinary, PythonBinaryAdaptor) with parameter types (Address+BuildFileAddress+BuildFileAddresses+Console+Digest+DirectoriesToMerge+DirectoryWithPrefixToAdd+DirectoryWithPrefixToStrip+ExecuteProcessRequest+InputFilesContent+InteractiveRunner+LLVM+MultiPlatformExecuteProcessRequest+NativeToolchain+OptionsBootstrapper+OwnersRequest+PathGlobs+PythonBinaryAdaptor+Scope+Specs+ToolchainVariantRequest+UrlToFetch+Workspace)
  (CreatedBinary, [PythonBinaryAdaptor], [Get(HydratedTarget, BuildFileAddress), Get(SourceRootStrippedSources, HydratedTarget), Get(Pex, CreatePexFromTargetClosure)], create_python_binary()):
    No rule was available to compute PythonBinaryAdaptor with parameter types (Address+BuildFileAddress+BuildFileAddresses+Console+Digest+DirectoriesToMerge+DirectoryWithPrefixToAdd+DirectoryWithPrefixToStrip+ExecuteProcessRequest+InputFilesContent+InteractiveRunner+LLVM+MultiPlatformExecuteProcessRequest+NativeToolchain+OptionsBootstrapper+OwnersRequest+PathGlobs+Scope+Specs+ToolchainVariantRequest+UrlToFetch+Workspace)
  (Pex, [CreatePexFromTargetClosure, PythonSetup], [Get(TransitiveHydratedTargets, BuildFileAddresses), Get(SourceRootStrippedSources, HydratedTarget), Get(Digest, DirectoriesToMerge), Get(InjectedInitDigest, Digest), Get(Pex, CreatePex)], create_pex_from_target_closure()):
    Was not usable by any other @rule.
  (Run, [Console, Workspace, InteractiveRunner, BuildRoot, BuildFileAddress], [Get(CreatedBinary, Address)], run()):
    No rule was available to compute Get(CreatedBinary, Address) with parameter types (Address+BuildFileAddress+BuildFileAddresses+Console+Digest+DirectoriesToMerge+DirectoryWithPrefixToAdd+DirectoryWithPrefixToStrip+ExecuteProcessRequest+InputFilesContent+InteractiveRunner+LLVM+MultiPlatformExecuteProcessRequest+NativeToolchain+OptionsBootstrapper+OwnersRequest+PathGlobs+Scope+Specs+ToolchainVariantRequest+UrlToFetch+Workspace)

[gshuflin]

Since this issue was created, we've introduced the concept of named rules, with the idea that giving an @rule a name is partially meant to signify it as blessed for display in certain contexts. Does anyone have any thoughts on how explicit rule names should interact with displaying the filename/line and function signature of a rule? @benjyw @stuhood

[benjyw]

Hmm, I think that in error messages whose audience is rule authors (like the one I pasted above) we probably want to use the signature.

I think we can distinguish between the static stringification of a rule function (e.g., as used during rule resolution, when there is no actual invocation yet), and the dynamic stringification of a rule function invocation, which can fall back to the former, but can optionally be nicer, using the named rule thing, plus whatever logic we come up with for displaying specific details of the invocation argument values.

[gshuflin]

wanted to put down my thoughts about a general unified paradigm for displaying
pants rules, including thoughts about some practical details of implementing
them.

Right now the rust engine code requires that an implementer of the node trait
also implement rust stdlib debug and display. the only type in the codebase
(outside of tests) that acutally implements node is nodekey. nodekey has
an explicit implementation of display (in nodes.rs), but this is actually
just printing out the auto-derived debug implementations of its variants.

I've also been working on adding support for named rules, and creating a
concept attached to a node i'm calling "display info". i have a commit i'm
working on right now that adds a method display_info(&self) -> option<string>
to the node trait. this is used to provide the annotated name (if one
exists) for an @rule, but we will also generate this automatically for certain
non-@rule node types. for instance, we'll automatically generate this for an
executeprocessrequest.

The error messages in rule graph errors are coming from prints in
rule_graph/src/rules.rs . these ultimately result from printing the display
representation of the dependencykey type. i agree with @benjyw that this is not
very clear right now.

Here's my proposal for what to change:

-We'll leave all rust auto-implementations of debug alone. this is consistent
with the std library docs about how that trait should be used:
(https://doc.rust-lang.org/std/fmt/trait.debug.html). basically, we want debug
to be a pretty faithful representation of a type as a rust data structure, even
if that looks ugly for other purposes. pants devs working on the engine should
care about the debug trait, most other people shouldn't.

-The display impl for an @rule will be changed to john's proposal; that is, the
file path, line number, and python signature of the @rule.

-The display impl for nodes that are not @rules will be left alone for now
(that is, we'll leave them as basically-identical to the rust
auto-implementation of debug), but we should feel free to change them at any
point in the future.

-We will try to make sure that user-facing error messages always use the
display implementation of a node, not the debug one (i.e. use {} not {:?} when
string-interpolating in rust) or the DependencyKey stringification.

-We will create a new notion of "display info", which we implement by adding a
function to the node trait: display_info(&self) -> option<string>. it's fine
for a node to not have a display info. @rules only have one if a rule author
annotates their rule, and we'll automatically implement one for ExecuteProcessRequests.

-Display info's will be used in two places: naming workunit outputs reported to
zipkin or another external service that collects workunit info; and the v2 ui
action display (the swim lanes with the lightning bolt icons). in the v2 ui we
will fall back to the display implementation if there is no display info, becuase
we have to display something there.

-The rule graph vizulation should use the display output of rules.

[gshuflin]

One more idea - "Display info" is a bad name because it sounds too much like "Display", we should call that concept something different and name the method that provides it something different as well.

[stuhood]

-We'll leave all rust auto-implementations of debug alone. this is consistent
with the std library...

+1

-The display impl for an @rule will be changed to john's proposal; that is, the...

+1

-The display impl for nodes that are not @rules will be left alone for now
(that is...)

I mentioned this briefly here: #8592 (comment) ... IMO, there are a few very cheap, very high value places to add names for intrinsic Nodes (Snapshots and ExecuteProcess nodes, basically): we should probably do that. Whether it is here (on this issue) or elsewhere is a different question though.

-We will try to make sure that user-facing error messages always use the
display implementation of a node, not the debug one (i.e. use {} not {:?} when
string-interpolating in rust) or the DependencyKey stringification.

So, to be explicit (because this very useful comment is on this ticket rather than on #7907): this issue was originally more about the rendering of rules in the RuleGraph, rather than of Nodes: in the first case, we don't know anything about the Params that will be used at runtime: a rule in the RuleGraph is roughly a "template" for a runtime Node that will have been filled out with some specific Params.

-We will create a new notion of "display info", which we implement by adding a
function to the node trait: display_info(&self) -> option...

Yep... makes sense. I thought you had already done that in #8592 though...?

-Display info's will be used in two places: ...

Yep, makes sense.

-The rule graph vizulation should use the display output of rules.

Yep.

[gshuflin]

Yep... makes sense. I thought you had already done that in #8592 though...?

I added a notion of a display info in that commit; what I'm talking about is making that part of the Node trait (so, I'd rewrite the code touched in #8592 to invoke that method on Nodes representing @rules, and also the v2 UI would invoke the same method to figure out what to display in the swim lanes)

[Eric-Arellano]

#8998 implements the majority of this ticket. There are some possible followups I punted on:

• unreachable error should specify the params which might not have been able to be produced. https://github.com/pantsbuild/pants/pull/8998/files#r369598308
• possibly surround all code with backticks https://github.com/pantsbuild/pants/pull/8998/files#r369447741
• filter out unreachable errors when they’re caused by ambiguous rules https://github.com/pantsbuild/pants/pull/8998/files#r369595021
• consider rewording the No rule was available to compute error https://github.com/pantsbuild/pants/pull/8998/files#r369597332

[cosmicexplorer]

from @stuhood in #7509 (comment):

I realized that in the rule-graph display, rather than rendering the Gets on the @rule's node, we should be rendering them as edge labels! This simplifies things, because we can use "the function signature" as in the wildcard idea.

I totally agree -- #7024 has decided to punt on that for now, only because the method used there to display Gets only has the list of Get(Subject, Product) pairs, without any explicit edges to other @rules which may end up satisfying the Get. I don't know if this exact information is already co-located with each Rule, or if we might have to add a utility method to the RuleGraph to get these labelled Get edges.

[stuhood]

from @stuhood in #7509 (comment):

I realized that in the rule-graph display, rather than rendering the Gets on the @rule's node, we should be rendering them as edge labels! This simplifies things, because we can use "the function signature" as in the wildcard idea.

I totally agree -- #7024 has decided to punt on that for now, only because the method used there to display Gets only has the list of Get(Subject, Product) pairs, without any explicit edges to other @rules which may end up satisfying the Get. I don't know if this exact information is already co-located with each Rule, or if we might have to add a utility method to the RuleGraph to get these labelled Get edges.

Here is an old branch that demonstrates doing it (while breaking a bunch of tests, probably): master...twitter:stuhood/move-dependencykey-info-to-edges