Add InvocationStrategy for sending inputs to control plane#31
Add InvocationStrategy for sending inputs to control plane#31rculbertson merged 7 commits intomainfrom
Conversation
rculbertson
force-pushed
the
ryan/control-plane-strategy
branch
2 times, most recently
from
a64b00c to
291fad9
Compare
rculbertson
force-pushed
the
ryan/control-plane-strategy
branch
from
291fad9 to
de7c480
Compare
| } | ||
| } | ||
|
|
||
| function timeNowSeconds() { |
There was a problem hiding this comment.
everything from here to the end of the file is moved from function.ts, but not changed at all.
| functionCallId: string, | ||
| timeout?: number, // in milliseconds | ||
| ): Promise<any> { | ||
| const startTime = Date.now(); |
There was a problem hiding this comment.
everything below here was moved to invocation_strategy.ts
ekzhang
left a comment
ekzhang
left a comment
There was a problem hiding this comment.
Thanks, yeah it's great that we're splitting this up, will take some time to review.
| /** | ||
| * This abstraction exists so that we can easily send inputs to either the control plane or the input plane. | ||
| * For the control plane, we call the FunctionMap, FunctionRetryInputs, and FunctionGetOutputs RPCs. | ||
| * For the input plane, we call the AttemptStart, AttemptRetry, and AttemptAwait RPCs. | ||
| * For now, we support just the control plane, and will add support for the input plane soon. | ||
| */ | ||
| export interface InvocationStrategy { | ||
| /** | ||
| * Executes the function call remotely and waits for the output. | ||
| * @returns A promise that resolves to the function output item. | ||
| */ | ||
| remote(input: FunctionPutInputsItem): Promise<FunctionGetOutputsItem>; | ||
|
|
||
| /** | ||
| * Spawns the function call asynchronously. | ||
| * @returns A promise that resolves to the function call ID. | ||
| */ | ||
| spawn(input: FunctionPutInputsItem): Promise<string>; | ||
| } |
There was a problem hiding this comment.
Hm, isn't this a leaky abstraction? I thought FunctionPutInputsItem and string are handled differently between the two implementations of an invocation, and with different tokens.
Also, I think the scope of this object isn't correct. In #32, you're storing attemptToken on the InvocationStrategy itself. But that means that if you call .remote() twice on the same function concurrently, their attempt tokens will overwrite each other.
There was a problem hiding this comment.
Oh yes, you're totally right about the object scope - my mistake. I pushed another commit with the scoping changed, so it should be fixed now.
Not sure if follow about the leaky abstraction? If you're referring to the original implementation we were looking at yesterday, I simplified things so it may be different now. The ControlPlaneStrategy no longer keeps a token in state. And InputPlaneStrategy doesn't implement spawn at all - that operation is not supported by the input plane.
rculbertson
force-pushed
the
ryan/control-plane-strategy
branch
2 times, most recently
from
cacbb31 to
23a98f7
Compare
rculbertson
force-pushed
the
ryan/control-plane-strategy
branch
3 times, most recently
from
eb9fcc6 to
a999ab3
Compare
rculbertson
force-pushed
the
ryan/control-plane-strategy
branch
from
a999ab3 to
99f9f77
Compare
rculbertson
force-pushed
the
ryan/control-plane-strategy
branch
from
a38d9c2 to
90b362b
Compare
ekzhang
left a comment
ekzhang
left a comment
There was a problem hiding this comment.
Looks good to me! Nice, I think the only thing I noticed is that some of the Go functions and structs are capitalized, which means they’ll be exported as public APIs - you just need to make the first letter lowercase and should be all set.
|
@ekzhang Thanks for reviewing! Ah ok, thanks for catching the public/private thing - will go through it and fix. |
…s#31) * Add InvocationStrategy for sending inputs to control plane * Create ControlPlaneStrategy once per invocation, not per function * Add test and fixes * Use lower level invocation model * Rename await to awaitOutput * Add go implementation * Make functions private
2 participants
This is the first of a few PRs to add support to for the input plane to libmodal.
Users can use the new input plane by specifying
input_plane_regionin their function definition like this:When
input_plane_regionis specified, the server will return the input plane URL to the client. The client should then connect to this URL and send all inputs to it.This PR begins to add support by introducing a new interface,
InvocationStrategy, which handles sending of inputs. For now, there is just one implementation,ControlPlaneStrategy, which sends inputs to the control plane. This does not alter the current functionality, it's just a restructuring, setting things up for subsequent PRs.The next PR builds on this one and adds support for
InputPlaneStrategy.Currently the input plane supports only
.remote- not.mapor.spawn.For now this includes only the typescript implementation - if this approach looks good, we will add the go implementation.