I still suggest that means we need to work out how to improve our syntax

Generally agreed with this sentiment. But we’re also running up against natural constraints of the engine that we are unlikely to want to change. For example, needing a new request type and a result type will likely always be the case, afaict. The engine is exceptionally magical, in a good way. But sometimes plain and simple Python is more appropriate. —

We should work out how to do that kind of transition in the future without breaking the world

Agreed

i think that @average-vr-56795 has probably been studiously avoiding saying so, but: it’s worth taking a look at Bazel’s API for this

Fwit, that was the original syntax I was going to propose until realizing 98% of fields are nothing more than a boolean toggle or a string value and to keep it simple

which, the

await target.field

thing?

Yes. Until Tansy and I came up with the idea of a PrimitiveField, I was horrified at the verbosity of the original implementation where every field was async

without an affordance to actually collapse it to literally

await target.field

, yea. i agree.

Yeah some kind of super lightweight

join_all

combinator along the lines of:

await [target.field, target.other_field]

throwback to the oldschool MultiGet

Jinx 😄

This would require writing a rule for every single field definition, presumably, right?

await target[Field]

means the call site is acceptable, but we need to also keep boilerplate minimal for plugin authors Also, nb that

HydrateSources

takes parameters, like

enable_codegen

.

await tgt[Sources]

would lose that ability, unless we had some special case

@hundreds-father-404: no, i was thinking that you could have the field return

Get[HydratedSources](HydrateSourcesRequest, ..)

So, that was similar-ish to an original mis-optimization I had on `AsyncField`s with the

.request

property. See https://github.com/pantsbuild/pants/pull/9632

yep. without the syntax it’s more verbose

No, that’s not my point. My point is that you need to be able to have the request type take any arbitrary values. Originally, I thought “it only needs an instance of the underlying field.” But I realized that doesn’t generalize What this prevents is having a

@property

on every

AsyncField

that generates the request for you, becaause

@property

doesn’t allow parameters. You could do it for most

AsyncFields

, but not all, and it’s not obvious when you can vs. cannot

right, but as i said… it could be a method.

I think that could be a very interesting thing to investigate

honestly, yea. i don’t have the beginning of a clue what it would look like, but… yea.

So, I proposed a long time ago not doing (... = yield ...) for Gets, but instead injecting functions. I finally took the time to do this and it seems pertinent to this discussion - it eliminates any ast parsing. Before:

test_addresses = Addresses((field_set.address,))
transitive_targets = await Get[TransitiveTargets](Addresses, test_addresses)

After:

async def setup_pytest_for_target(
    ...,
    get_transitive_targets: GetRule[TransitiveTargets, Addresses]
) -> TestTargetSetup:
    test_addresses = Addresses((field_set.address,))
    transitive_targets = await get_transitive_targets(test_addresses)

The idea is probably self explanatory given signatures, but GetRule[X, Y] produces a unique singleton type for the pair X, Y and associated singleton rule producing a singleton instance of that type. GetRule is a callable that takes a subject and produces a Get(X, Y, subject). Nay, Hrm or Yay?

Initial reaction is hrm, but possibly that’s from becoming used to the old style. What are your goals with this new syntax? Avoid ast parsing, right?

Yes - that's the only goal. The side effect pertinent to this discussion is the idea of syntax getting in the way. It proves out the idea of having fonctions that return a get for use in await.

and then,

await get_transitive_targets(s1, s2, s3)

No changes:

MultiGet(get_rule1(...), get_rule2(...))

Or:

MultiGet(get_rule(x) for x in xs)

Interesting, that MultiGet looks good. My first major reaction was not liking the coupling of the call sites with the function signature. `await Get`s are no longer as decentralized. The main example I’m thinking of is how we often use conditionals with Gets, such as

list_targets.py

using either

Get[Targets]

or

Get[TransitiveTargets]

. I like that that is encapsulated in the call site and doesn’t leak. I think I like this because I view it as reducing the surface area of what I need to consider. But, rules are not normal. Here, the coupling might not be a bad thing. There’s an argument that it centralizes all the magic of the engine into one place. So, I’m trying to keep an open mind.

The coupling is identical, not exactly following that language.

Currently, you only put information on your Gets at the call site. Now, you need to pre-declare it in the rule signature, then still use at the original call site. You’re expressing information about the Get in two places now.

You aren't really, you don't express the type parameters more than once.

https://pantsbuild.slack.com/archives/C0D7TNJHL/p1590770408460600?thread_ts=1590092441.380300&cid=C0D7TNJHL that probably means the concept isn’t actually removed?

The only thing expressed twice is the name of the rule.

You aren’t really, you don’t express the type parameters more than once.

Yes, but this no longer gets expressed in the same location as the call site. “Principle of Proximity” is more apt than coupling for my concern.

hat probably means the concept isn’t actually removed?

It is - Get would be a hidden type

the first time this was proposed, it was as callables that actually return the value, iirc… which would have had multiple magic removal benefits, but runs afoul of the fact that we need to suspend

All you know is the return of a GetRule is an awaitable object

hm. or could it literally be an async function?

Not sure, but abit besides the point. Does this shift even sound good to folks is what I'm after.

the most questionable spot will be rules with tons of suspension points

https://pantsbuild.slack.com/archives/C0D7TNJHL/p1590771152464000?thread_ts=1590092441.380300&cid=C0D7TNJHL The 1st time it was proposed IIRC the objection that halted me was something like that - yeah. ~"due to technicalities we can't actually be concurrent here - would require maybe trampoliing" ... but its clear now with the awaitable interface we don't need that.

await

== suspension

Stu - I'm pointing out I have changed 0 its still

await Get

- if that's broken already that's another problem.

understood. i’m clarifying what i was saying in https://pantsbuild.slack.com/archives/C0D7TNJHL/p1590771152464000?thread_ts=1590092441.380300&cid=C0D7TNJHL which you quoted

I’m a hrm vote. I think it’s a good idea to centralize all the engine’s magic in one place. Where I’m conflicted is the above concerns about principle of proximity. It’d help to see some 2-3 real world examples, I think.

pytest_runner.py

would be a great example.

K

i’m similarly hrm’y. if the tradeoff is longer signature for tighter method body, then… maybe

understood. i’m clarifying what i was saying in

K - I think though, since the switch to await did not change Python <-> rust concurrency dynamics or code at all, that the old point was wrong in the large and maybe right about me using poor language. IE we could have always done this.

the first time this was proposed, it was as callables that actually return the value

^ this is the thing that runs afoul of suspension… that’s all i’m saying

The tradeoff is longer signature for same method body and 0 ast parse

anyway.

The proposal was an idea on how to shape the API. If it was possible and I was using the wrong language, that's better feedback.

the method body is shorter, no? no more Gets. in the context of this thread, thinking about how it applies to field accesses would be interesting

I think the method body will have slightly more characters on average.

because of the need to name them?

await Get[Targets](Addresses([..]))

vs.

await get_targets(Addresses([..])

anywho, i do think it is interesting. i can try porting something and seeing what it looks like, although we probably all shouldn’t choose the same thing to try with

Let me post the PR and I'll port a small rule and a pytest_runner which is about as large as they come right now to see the extremes of 1 get type vs ~7

@hundreds-father-404: although it opens the door to

await get_targets(addresses)

… no need for the type declaration to be on the same line anymore

Hm, yeah, that snippet keeps me at a hrm. Currently I can know exactly what the product I’m getting will be. It’s all there. With the change, I need to refer back to my rule signature to know that

get_targets()

will give me back

Targets

. Arguably, the parameter name makes that obvious. But the call site now de-emphasizes types. I like the emphasis on types because this is a type-driven API

OK, well I'll post the PR. One of the things that drove me here in part is our types are neutered by the ast parsing. You must artifically say Get[X](Y()) and cannot say y = Y(); Get[X](y) just to appease the restricted info environment of the ast parse context.

thanks: worth investigating at least.

Agreed, I appreciate the investigation. I could definitely see us wanting to do this. And removing the AST restriction could end up resulting in some really neat future development we can’t anticipate.

Its my fault for not pushing on this deeper several years ago.

or mine *shrug

With the change, I need to refer back to my rule signature to know that 

get_targets()

 will give me back 

Targets

.

Why should this be different from the rest of the language?

Why should this be different from the rest of the language?

Because the rest of the language doesn’t place a huge weight on types, even with MyPy. In contrast, you must use types for the engine. And you must also use them in a particular way, e.g. not being able to request a subclass of a registered type

the top of this thread has expired out… eep. will start a new one in #engine