https://pantsbuild.org/ logo
Join Slack
Powered by
<@U02TSJJT9DJ> I forget if/where we landed on this...
# development
h
@wide-midnight-78598 I forget if/where we landed on this, but how do we do MultiGet in the call-by-name world?
w
concurrently
is just an alias to MultiGet - but I haven't QA'd the call-by-name migrations, as it's waiting on the intrinsics piece
That is/will be the purpose of this repo: https://github.com/sureshjoshi/pantsanity try to enable every backend as I migrate through them
👍 1
h
So it occurred to me yesterday that it should be called 
gather
, a la 
asyncio.gather
.
?
w
I am fine with basically any naming that makes any sense. MultiGet was probably the only name that no longer made sense. You mentioned in a comment that we could use 
asyncio.gather
even? But I wasn't sure if that was the case, having never looked under the hood
h
I doubt we can without monkeypatching, but it would have been nice
But AFAICT this is exactly the same as 
asyncio.gather
- wait for all the awaitables concurrently, and return when they are all done
So that name makes sense
(I originally suggested 
concurrently()
but I have since done a whole bunch of non-Pants-related python asyncio work, and so am used to 
gather()
)
w
This is all I've done as of today: https://github.com/pantsbuild/pants/blob/aeefb6b53297e6865555f4014d975b2409b27ff4/src/python/pants/engine/internals/selectors.py#L599 The only "thing" I've got in my mind about 
gather
is shadowing the actual gather. BUT, if we're not using Python's asyncio in any of our files, then it's a nonissue
h
Note that 
gather()
is itself an awaitable so you can compose them. Not sure if that is true of MultiGet.
Well, it's idiomatic to call 
asyncio.gather()
, so they wouldn't collide even if we did have python asyncio
w
Yeah, good point - I always call asyncio.whatever - just a note though, as people are gonna people
h
truth
Still, if you have regular asyncio code in your Pants plugin, you're already covered in blood
w
Ha! Yep, fair point. Can't argue that
oooh, that is a good idea to add to my plugin linter though. Make sure 
asyncio
isn't imported
h
Background is I'm prototyping some New Python Backend ideas and using call-by-name
Hmm, I'm getting errors like these when using `concurrently`:
Copy code
TypeError: get_optional_source_root() got multiple values for argument 'source_root_request'
Ring any bells?
The code is something like:
Copy code
source_roots = await concurrently(
        [get_optional_source_root(SourceRootRequest(Path("zzz")), **implicitly()),
         get_optional_source_root(SourceRootRequest(Path("yyy")), **implicitly())]
    )
But this happens even with a single underlying get, and the error is Python's error when you provide the same kwarg multiple times
(and removing the implicitly call doesn't help)
w
I ran into something like this, but stopped debugging until the intrinsics was handled - as I have a bunch of QA to run on it first.
But that was months ago
Is https://github.com/pantsbuild/pants/pull/20874 something you think you could push over the finish line, and then I can hop back on the migrations and QA effort
h
Yeah, I can take a look at that
🎉 1
@witty-crayon-22786 what is the status on https://github.com/pantsbuild/pants/pull/20874 ? Should I take it over?
w
It should be very, very close but I just had a few busy weekends. If you have time to land it that would be awesome!
h
Is it just at "Get a few stubborn tests to pass" stage?
w
That's right
h
On it
w
In fact, it might just be missing pyi entries
I went ahead and pushed something that should get it green. Sorry for the delay! Review appreciated.
❤️ 1
@wide-midnight-78598: after that lands, your rewrite will pick up the intrinsics without any changes, since they will be normal 
@rules
. Woot.
🎉 1
w
Yayyyy
I've got twitchy fingers to start getting some of those migrations running and figuring out gaps in the migration/migration script. There were just so many intrinsics when compared to the others that it didn't make sense 🤷
h
Sorry, I got sidetracked seeing various tests fail, then realized many of them also failed on main on my machine, so am working on getting main green on a non-linux laptop (obvi it passes on macos CI, so there are evidently some places where our tests are sensitive to local conditions)
w
What system are you on? macos intel or arm?
h
arm
In some cases this is known issues such as depending on an externally installed thrift compiler, which we expect to be of a specific version
So to "fix" that I just installed the right thrift compiler
but other cases are more nuanced
w
One of my older issues was not having rosetta installed
I think most of those are solved - the Go backend is another one, I thought it was auto downloaded, but seems not
I think we can build a portable thrift compiler per OS - maybe it's worth having a vendored version to auto-download and run?
Wouldn't be the most updated, but 🤷
h
We used to do exactly that, but it costs us bandwidth
Well, we still host those binaries, for any Pants v1 users, but that is negligible
Some of these may be bitrot because we don't run all tests on all platforms
w
@happy-kitchen-89482 Starting back on the migration this week - you still running into the 
multiple values
issue?
I ran into it, when using a certain style of call - we map the 4 GET forms into 4 call-by-name forms, but it looks like Pants doesnt' like one of them. Unrelated to 
concurrently
This gives me a multiple values error (I think because 
implicitly
is passing the 
request
object, even though that's the first arg in line 1. (
map_short_form_get_to_new_syntax
in migrate_call_by_name.py)
Copy code
explicitly_provided_deps = await determine_explicitly_provided_dependencies(DependenciesRequest(request.field_set.dependencies), **implicitly())
    hydrated_sources = await hydrate_sources(HydrateSourcesRequest(request.field_set.sources), **implicitly())
This version of call-by-name works. (
map_long_form_get_to_new_syntax
in migrate_call_by_name.py)
Copy code
explicitly_provided_deps = await determine_explicitly_provided_dependencies(**implicitly(DependenciesRequest(request.field_set.dependencies)))
    hydrated_sources = await hydrate_sources(**implicitly(HydrateSourcesRequest(request.field_set.sources)))
h
I may have been using 
implicitly()
incorrectly. I'm not actually sure what its arguments should be?
w
I don't think you necessarily were - there are a bunch of ways to call it - but this is a new error. https://github.com/pantsbuild/pants/blob/cc5577ed867400cb003da75aa1a9d44812ecf940/src/python/pants/goal/migrate_call_by_name.py#L388 There are the 4 mappings we used in that function, 1 per GET style.
Original proposal with syntax proposals at the bottom: https://github.com/pantsbuild/pants/discussions/18905
It also looks like Huon is using callbyname in an upcoming feature too: https://github.com/pantsbuild/pants/pull/20988/files#diff-01d55b62882286165f80d38bb3123af1a461ddbaac9863cd93a8822f03059e79R671 Note: As far as I know, you don't "need" 
implicitly
if everything can be marked by positional args. The migration tool doesn't read out function types, so it indiscriminately has 
implicitly
in every migrated call
Though, that would be a pretty neat linter - to help with the obvious future cargo culting that'll happen
@witty-crayon-22786 @happy-kitchen-89482 https://github.com/pantsbuild/pants/blob/99badc093b24ef5389299b0d14589aba7e966bcd/src/rust/engine/src/nodes/task.rs#L283-L289 I think there's a bug here? tasks.args zipped with deps seems to give an off-by-1 kwargs error in my testing, where I get a:
Copy code
async def determine_explicitly_provided_dependencies(
    request: ExplicitlyProvidedDependenciesRequest,
    union_membership: UnionMembership,
    registered_target_types: RegisteredTargetTypes,
    subproject_roots: SubprojectRoots,

...

foo = await determine_explicitly_provided_dependencies(DependenciesRequest(request.field_set.dependencies), **implicitly())
In the rust code 
func.call(args, Some(kwargs))
- I'm seeing a kwargs of:
Copy code
"request": UnionMembership<>,
"union_membership": RegisteredTargetTypes<>,
"registered_target_types": SubprojectRoots
which is causing the 
multiple values of "request"
error. I think we're missing a 
.skip(explicit_args_arity)
kinda thing on the 
self.tasks.args.iter()
?
w
Hm. Yea, I think you're right: sorry about that!
w
No worries - I can try a fix, but are there tests in Rust for this section of code? Or should this be tested at the python level?
h
Verified that fixing this gets rid of the issue I was having
w
Approved, but wasn't sure if there was a unit test to run somewhere, or if something in python would catch it. I'm guessing it would be captured once more call-by-name features are used, via the integration tests
7 Views
Open in Slack
PreviousNext
Powered by