Add return_exceptions argument to modal.FunctionCall.gather.
#3014
Conversation
danielshaar
force-pushed
the
dshaar/gather-return-exceptions
branch
from
776baf1 to
055e9d5
Compare
| with app.run(client=client): | ||
| results = FunctionCall.gather(failure_modal.spawn(), failure_modal.spawn(), return_exceptions=True) | ||
| for result in results: | ||
| assert isinstance(result, UserCodeException) |
There was a problem hiding this comment.
What is this UserCodeException type?
There was a problem hiding this comment.
This is a synchronicity-internal type ("User" in this case is user of synchronicity, i.e. modal sdk devs - not user of Modal) that isn't intended to leak into user code.
Internally in synchronicity it's used to wrap exceptions when they first happen and then unwrap them when they are raised to users. This is used a a shortcut to remove as much synchronicity context leak as possible in tracebacks etc. since those parts are usually not relevant to users.
However in this case, since we never raise the exceptions from the synchronized function (Function.gather(return_exceptions=True) in this case) it never triggers the unwrapping code in synchronicity and the internal type "leaks", since it's a return value instead.
I'm not sure what would be the cleanest fix tbh... It does feel like this is synchronicity's responsibility and the fix should happen on that side - I'll try something.
I'm personally not convinced the exception wrapping/unwrapping does us much good, and it's something I'd like to get rid of in synchronicity 2.0 if we can somehow get the traces relatively simple there through other means.
There was a problem hiding this comment.
After some more investigation, the problem might be worse than I thought, apparently the client has some explicit wrapping using UserCodeException too in _process_result, which seems like the more likely culprit here 🤯
There was a problem hiding this comment.
We need to get a 😵💫 option for GitHub reactions 😁
There was a problem hiding this comment.
I'm inclined to believe this isn't actually a synchronicity issue but a Modal hack introducing explicit UserCodeException wrapping on the modal client side. This was introduced in 0.0.35 :D
There was a problem hiding this comment.
Ohhhh no, synchronicity.UserCodeException is apparently being returned by .map(return_exceptions=True) as well :(
Now I'm not sure what to do - will have to discuss in client team...
|
|
||
| with app.run(client=client): | ||
| results = FunctionCall.gather(failure_modal.spawn(), failure_modal.spawn(), return_exceptions=True) | ||
| for result in results: |
There was a problem hiding this comment.
Might be nice to assert that you can have a mixture of exceptions with expected successful results.
| @@ -1760,13 +1760,15 @@ async def from_id(function_call_id: str, client: Optional[_Client] = None) -> "_ | |||
| return fc | |||
|
|
|||
| @staticmethod | |||
| async def gather(*function_calls: "_FunctionCall[T]") -> typing.Sequence[T]: | |||
There was a problem hiding this comment.
This is going to mean that the return type is "wrong" in the case of an exception right? Any idea how asyncio.gather handles this? Might need to @overload
There was a problem hiding this comment.
+1 - add an @overload which differentiates the return type based on a Literal[True] value for the return_exceptions argument, and otherwise only returns Sequence[T] - that should fix the existing type assertion
Describe your changes
This is possible in the async context by using asyncio.gather directly, but we want to expose this in a sync way for users as well.
Changelog
return_exceptionsargument tomodal.FunctionCall.gatherto allow gathering failed and successful results from a collection ofFunctionCallobjects.