curved-television-6568
06/07/2022, 12:36 PMdescription_of_origin
data to various request types, I’m wondering if there’s not a better more generic approach to this? 🧵Get
call, or as an additional argument..bitter-ability-32190
06/07/2022, 1:38 PMdescription_of_origin
gives the user rich context, but for the developer I always find it hard to reason about because it's really provided in the context of some other message.
There has to be a way (and I'm such it's very delicate) where there isn't such tight coupling between the caller and calleecurved-television-6568
06/07/2022, 1:51 PMdescription_of_origin
shows what it is we want to achieve, only I see it as very disruptive and ad-hoc which makes it inhibitly (not a word according to my spell check? 😛 ) difficult pattern to apply generically for more (ideally all) scenarios.hundreds-father-404
06/07/2022, 2:22 PMcurved-television-6568
06/07/2022, 2:23 PM[Eric] I think that we have to get rid offrom https://github.com/pantsbuild/pants/issues/14468#issuecomment-1147977415 Say that at the call site for, and instead addAddresses -> UnexpandedTargets -> Targets
.TargetsRequest
await Get(Targets, Addresses(…))
we could do something like this instead: await Get(Targets, Addresses(…), description_of_origin="…")
that would propagate through all the rules so if you end up in an error handler, you can include that origin description in the error message.hundreds-father-404
06/07/2022, 2:30 PM@rule
, you might have multiple "origins" in play at the same time. For example, ./pants paths
has both --paths-to
and --paths-from
..then while hydrating the dependency graph, we have tons of dependencies
fields in play
So we need to keep the description_of_origin
tightly coupled to each specific caller or Get
Given that, what would be the benefit of Get(Targets, Addresses(…), description_of_origin="…")
vs Get(Targets, TargetsRequest([addresses], description_of_origin="..."))
. Afaict that's only a different typingcurved-television-6568
06/07/2022, 2:31 PM@rule
async def my_rule(request: SomeRequestType, rule_context: RuleExecutionContext) -> ...:
if bad_stuff:
raise InvalidStuff(f"This was no good, because {rule_context.get('reasons', 'unknown')}.")
...
[…] you might have multiple “origins” in play at the same time.yeah, that could prove my idea to be a bad one. (I’ll see if I get the chance to poc something up with this, time permitting, to see if that’s the case)
Afaict that’s only a different typingas I see it, it’s more than different typing, as you don’t need to change the structure of your request types, nor introduce new types.
witty-crayon-22786
06/07/2022, 4:12 PMPerhaps this could be a form of “rule execution context” where any kwargs are preserved, and passed to rules when requested as inputs.this is effectively https://github.com/pantsbuild/pants/issues/7490 i think
await Try
, which could fail and return a value--no-print-stacktrace
.
in particular, https://github.com/pantsbuild/pants/issues/10536 talks about adding EngineAware params verbatim to the end of the description of a @rule
. if we did that, and then even when --no-print-stracktrace
, we rendered the stack of rules with descriptions, i think that it would catch 95% of these.
the primary risk is that anytime we use this data for some “new” (sortof) use case, we complicate what it means to add a description, set the level, etc. but i think that that just means that it is a very powerful mechanismhundreds-father-404
06/08/2022, 7:19 PMdescription_of_origin
though because we need the precision of individual Gets:
• --paths-from
vs --paths-to
, whereas otherwise the most precise we have is "the --paths
goal"
• archive(files=)
vs archive(package=)
, whereas otherwise we just have "resolving dependencies"
• python_distribution(entry_points=)
vs python_distribution(provides=setup_py(...))
• python_test(runtime_package_dependencies)
Get(WrappedTarget, WrappedTargetRequest(address, description_of_origin="the option --paths-to"))
vs Get(WrappedTarget, Address, address)
?
If anything, that's somewhat more predictable. Follows our X and XRequest conventionEngineAware
metadata -- folks may not realize it should be set in a particular way for error messages to render exactly the right way. Whereas description_of_origin
intentionally must be setbitter-ability-32190
06/08/2022, 7:29 PMdescription_of_origin
a bit more clientside-intuitive? I think that's the pain point here.hundreds-father-404
06/08/2022, 7:30 PMbitter-ability-32190
06/08/2022, 7:31 PMI like thatgives the user rich context, but for the developer I always find it hard to reason about because it's really provided in the context of some other message.description_of_origin
There has to be a way (and I'm sure it's very delicate) where there isn't such tight coupling between the caller and callee
hundreds-father-404
06/08/2022, 7:33 PMbitter-ability-32190
06/08/2022, 7:33 PMhundreds-father-404
06/08/2022, 7:37 PMinvalid_address_error_origin
or something, but it's more complicated than that because the same description_of_origin
may be reused across multiple error messages
Let's take the option --paths-to
, which uses Specs
codepath. Possible error locations:
• Specs
rules, e.g. using a file path that does not exist
• AddressInput
rules, that an address literal is malformed like using too many ../
in a file address
• Address
rules, that the address is well-formed but does not exist
We want to use the same description_of_origin="the option --paths-to"
in all those caseswitty-crayon-22786
06/08/2022, 7:42 PMI still strongly advocate forhm, i see what you mean. that would require something closer to @curved-television-6568’sthough because we need the precision of individual Gets:description_of_origin
Get(.., context=..)
syntax.hundreds-father-404
06/08/2022, 7:43 PMdescription_of_origin
😛witty-crayon-22786
06/08/2022, 7:43 PMhundreds-father-404
06/08/2022, 7:44 PMdescription_of_origin: str | None = None
, and have the old error messages + new ones. We could still do that! But my thinking was we always want high quality error messageswitty-crayon-22786
06/08/2022, 7:45 PMYou’d still have to update all callers, no? It’s important that all call sites are using these better error messages imoonly the caller: not the callee
hundreds-father-404
06/08/2022, 7:47 PMAddress -> WrappedTarget
vs WrappedTargetRequest -> WrappedTarget
is worse.
It is for sure disruptive. But after we fix everything, I think it's actually better
1) More consistent with how we usually name things
2) We can add additional enrichment when we need, without changing rule typeswitty-crayon-22786
06/08/2022, 7:52 PM1) More consistent with how we usually name things
2) We can add additional enrichment when we need, without changing rule typesi do think that it is worse in the long run. the Request pattern is not something that we will need in the long run, because https://github.com/pantsbuild/pants/issues/7490 should make many of them unnecessary.
hundreds-father-404
06/08/2022, 7:57 PMi’ll shipit changes for description_of_originCool. First two are ready to land, and unblock next two. I think I can land this all in time for the alpha release
curved-television-6568
06/09/2022, 5:47 AM