-
Notifications
You must be signed in to change notification settings - Fork 53
Add return_exceptions
argument to modal.FunctionCall.gather
.
#3014
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -496,6 +496,24 @@ def test_function_exception(client, servicer): | |
assert "foo!" in str(excinfo.value) | ||
|
||
|
||
def test_function_exception_gather(client, servicer): | ||
app = App() | ||
|
||
servicer.function_body(failure) | ||
failure_modal = app.function()(failure) | ||
with app.run(client=client): | ||
with pytest.raises(CustomException) as excinfo: | ||
FunctionCall.gather(failure_modal.spawn(), failure_modal.spawn()) | ||
assert "foo!" in str(excinfo.value) | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be nice to assert that you can have a mixture of exceptions with expected successful results. |
||
assert isinstance(result, UserCodeException) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ( 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to get a 😵💫 option for GitHub reactions 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohhhh no, Now I'm not sure what to do - will have to discuss in client team... |
||
assert isinstance(result.exc, CustomException) | ||
assert "foo!" in str(result) | ||
|
||
|
||
@pytest.mark.asyncio | ||
async def test_function_exception_async(client, servicer): | ||
app = App() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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