https://pantsbuild.org/ logo
Join Slack
Powered by
> I still suggest that means we need to work ou...
# development
h
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
w
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
providers are effectively async fields… things computed for targets https://docs.bazel.build/versions/master/skylark/lib/Provider.html
and non-computed fields are … something else. lemme see.
(and i say this not because they are better or worse, but because it’s generally good to know the competition)
also, i would say that part of the reason why neither the target API nor the engine itself should be considered “done” at this point is because if we identify something here that we do need from the engine, we should do it. now is the time.
as a 100% strawman example: if support for walking into methods to collect 
Get
s would be helpful, we should think about it. it would allow you to uniformly do 
await target.field
, regardless of whether the field was async or not
👍 1
h
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
w
which, the 
await target.field
thing?
h
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
w
without an affordance to actually collapse it to literally 
await target.field
, yea. i agree.
with it, it seems feasible maybe. but would probably want to make multiget a bit lighter as well (we should anyway)
fields = await (target.sources, target.dependencies)
a
Yeah some kind of super lightweight 
join_all
combinator along the lines of:
Copy code
await [target.field, target.other_field]
w
throwback to the oldschool MultiGet
a
Jinx 😄
w
coke
😃
h
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
w
@hundreds-father-404: no, i was thinking that you could have the field return 
Get[HydratedSources](HydrateSourcesRequest, ..)
so it wouldn’t change the rule graph at all i don’t think… unless we found some other reason to.
or it could be a method returning a 
Get
… 
await target.sources(PythonSources)
property vs method doesn’t matter if it had a declared return type
h
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
w
yep. without the syntax it’s more verbose
with the syntax, it’s less verbose
Copy code
all_hydrated_sources = await MultiGet(
  1.  Get[HydratedSources](HydrateSourcesRequest, tgt.get(Sources).request) for tgt in targets
  2.  Get[HydratedSources](HydrateSourcesRequest(tgt.get(Sources))) for tgt in targets
  3.  tgt.get(Sources)
)
h
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
w
right, but as i said… it could be a method.
(a method which would return a 
Get
)
i think that even without actually inspecting the body of the method/property (just the return type), that allows for this usecase… unless i’m missing something around generics
i realized this morning that this amounts to local type inference. the method/property bit is easy… but you need to know what type the method is being called on. whomp whomp.
unless we wanted to convert the whole phase into an embedded mypy plugin =P
a
I think that could be a very interesting thing to investigate
w
honestly, yea. i don’t have the beginning of a clue what it would look like, but… yea.
e
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:
Copy code
test_addresses = Addresses((field_set.address,))
transitive_targets = await Get[TransitiveTargets](Addresses, test_addresses)
After:
Copy code
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?
(This is all MyPy compliant)
👍 1
h
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?
Any thoughts on what this might look like with multiple params support?
Copy code
get_transitive_targets: GetRule[TransitiveTargets, Params[Addresses, Foo]]
e
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.
It would be GetRule[ProductType, S1, S2, S3, ...]
👍 1
So just more Esses
With the correspoding callable signature taking that many more subjects.
👍 1
h
and then, 
await get_transitive_targets(s1, s2, s3)
What does 
MultiGet
look like?
e
No changes:
Copy code
MultiGet(get_rule1(...), get_rule2(...))
Or:
Copy code
MultiGet(get_rule(x) for x in xs)
Remember, all get_rule(subject) does is return a Get object
👍 1
So no more ast at the cost of explicityly listing your unique Get[X, Y] combos in the form of unique GetRule function parameters.
In other words - its very obvious the only things you get to work with are what the engine provides in the function parameters - that's all you get.
It technically reduces concepts by 1. Now there are just rules (which you already learn are awaitable when you write one as 
async def ...
) and rule parameters. You can ask the engine to find one of these rules for you with a GetRule parameter; ie: link to other peoples rules, just not by name, which is the point of the whole plugin / dependency injection system.
The removed concept is this special Get object.
h
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.
e
The coupling is identical, not exactly following that language.
Before the coupling was ast parsed, now its just a normal parameter with type ascription.
There is more typing for the same coupling though.
h
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.
(I mean coupling from the perspective of the rule author; not from the AST parser)
e
You aren't really, you don't express the type parameters more than once.
w
e
The only thing expressed twice is the name of the rule.
h
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.
e
hat probably means the concept isn’t actually removed?
It is - Get would be a hidden type
w
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
e
All you know is the return of a GetRule is an awaitable object
w
hm. or could it literally be an async function?
e
Not sure, but abit besides the point. Does this shift even sound good to folks is what I'm after.
I can post the draft PR later today to have the reality be easier to see / grok.
w
the most questionable spot will be rules with tons of suspension points
👍 1
e
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.
👍 1
There is no suspension here getrule(subject) returns a Get and then you await that.
w
await
== suspension
(was yield in the past discussion)
e
Stu - I'm pointing out I have changed 0 its still 
await Get
- if that's broken already that's another problem.
w
h
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.
e
K
w
i’m similarly hrm’y. if the tradeoff is longer signature for tighter method body, then… maybe
e
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.
👍 1
w
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
e
The tradeoff is longer signature for same method body and 0 ast parse
👍 1
w
anyway.
e
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.
w
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
e
I think the method body will have slightly more characters on average.
Unless you name your GetRule parameters <= 3 characters each.
w
because of the need to name them?
but the types go away
h
Copy code
await Get[Targets](Addresses([..]))
vs.
Copy code
await get_targets(Addresses([..])
w
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
e
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
w
@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
h
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
e
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.
👍 1
w
thanks: worth investigating at least.
h
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.
e
Its my fault for not pushing on this deeper several years ago.
w
or mine *shrug
i’ll try porting the Black Setup rule
a
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?
h
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
w
the top of this thread has expired out… eep. will start a new one in #engine
Open in Slack
PreviousNext
Powered by