Dont use internal synchronicity UserCodeException type #3168

freider · 2025-05-20T08:19:52Z

This turned out to be a bit of a rabbit hole.

The client has abused the fact that synchronicity unwraps synchronicity.UserCodeExceptions and wrapped any exceptions returned by containers
I believe the original intent of this was to piggy back off synchronicity's use of this to exclude certain internal frames in tracebacks
More recently, we've also started using it for client-side retries, since it has been a way for us to disambiguate internal errors from errors in user remote containers (things that should be retried)
Synchronicity only unwraps these exceptions when they are raised, so an unfortunate side effect has been that map(return_exceptions=True) has returned actual synchronicity.UserCodeException objects instead of the actual exception (!)

This PR removes the usage of this internal synchronicity exception type in favor of an explicit modal-specific exception wrapper type, _ContainerException (open to other names). This means we also need to explicitly unwrap these whenever exceptions can be returned to users.

Unfortunately, this is is a breaking change for .map(return_exceptions=True) in that it will no longer wrap the user exceptions... But I believe that has never actually been the intent of the functionality, and it's quite ugly that it has been returning a synchronicity exception type (!).

Closes CLI-419

Compatibility checklist

Check these boxes or delete any item (or this section) if not relevant for this PR.

Client+Server: this change is compatible with old servers
Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.

Release checklist

If you intend for this commit to trigger a full release to PyPI, please ensure that the following steps have been taken:

Version file (modal_version/__init__.py) has been updated with the next logical version
Changelog has been cleaned up and given an appropriate subhead

Changelog

…rapped exception (!)

freider · 2025-05-20T09:17:57Z

modal/_utils/function_utils.py

+        raise exc.unwrap()
+
+
+async def _process_result_wrap_exceptions(result: api_pb2.GenericResult, data_format: int, stub, client=None):


Another option is to use a boolean flag to wrap exceptions, but that feels a bit more smelly to me

freider · 2025-05-20T09:18:47Z

test/function_test.py

@@ -357,6 +356,35 @@ async def test_function_future_async(client, servicer):
        assert future.object_id not in servicer.cleared_function_calls  # keep results around a bit longer for futures


+def test_function_spawn_exception(client, servicer):


Kind of scary that we didn't have any tests for spawn or generator calls with exceptions...

freider · 2025-05-20T09:18:58Z

test/function_test.py

@@ -533,7 +561,7 @@ def test_map_exceptions(client, servicer):

        res = list(custom_function_modal.map(range(6), return_exceptions=True))
        assert res[:4] == [0, 1, 4, 9] and res[5] == 25
-        assert type(res[4]) is UserCodeException and "bad" in str(res[4])
+        assert type(res[4]) is CustomException and "bad" in str(res[4])


Breaking change!

Although this is breaking, I'll say this is more of a bug fix.

freider · 2025-05-20T09:21:04Z

modal/_functions.py

-            or not ctx.sync_client_retries_enabled
-        ):
-            return await self._get_single_output()
+        try:


This is the only place where we actually "need" the exception wrapping (in order to detect retryable errors), so it's explicitly using _get_single_output_wrap_exceptions and then unwrapping it in an outer try/except

freider · 2025-05-20T09:23:04Z

Related thing I noticed - run_generator currently calls run_function, but run_function is unnecessarily complex with retry logic that is never used for generators so maybe we could have it just call _get_single_output instead? wdyt @rculbertson ?

rculbertson · 2025-05-23T20:40:49Z

Nice! Just curious, how did you start down this rabbit hole? :) Was this causing a user issue?

Yea, calling _get_single_output for generators seems good to me.

thomasjpfan · 2025-05-29T01:23:08Z

modal/_functions.py

+        except _ContainerException as exc:
+            raise exc.unwrap()


Nit: In an effort to try to reduce the amount of indenting can the try, except be a decorator? Something like this:

def _unwrap_container_exception(func): @functools.wraps(func) async def wrapper(*args, **kwargs): try: return await func(*args, **kwargs) except _ContainerException as exc: raise exc.unwrap() return wrapper @_unwrap_container_exception async def run_function(self) -> Any: ...

thomasjpfan · 2025-05-29T01:25:42Z

modal/exception.py

@@ -22,6 +22,19 @@ class Error(Exception):
    """


+class _ContainerException(Exception):


I prefer this name a little more:

Suggested change

class _ContainerException(Exception):

class _WrappedException(Exception):

At a glance, I thought "Container" meant "in the runtime container".

thomasjpfan · 2025-05-29T01:26:35Z

test/function_test.py

@@ -533,7 +561,7 @@ def test_map_exceptions(client, servicer):

        res = list(custom_function_modal.map(range(6), return_exceptions=True))
        assert res[:4] == [0, 1, 4, 9] and res[5] == 25
-        assert type(res[4]) is UserCodeException and "bad" in str(res[4])
+        assert type(res[4]) is CustomException and "bad" in str(res[4])


Although this is breaking, I'll say this is more of a bug fix.

freider · 2025-06-16T13:40:39Z

Superseded by #3226

freider added 5 commits May 20, 2025 09:44

Don't use internal synchronicity UserCodeException

d2254e7

Adjust test - return_exceptions should not return UserCodeException-w…

b1105f3

…rapped exception (!)

Fix generators

47a6836

revert print

ec4cfbc

A bit clearer contract of when we wrap exceptions and not

197d283

freider commented May 20, 2025

View reviewed changes

freider requested review from mwaskom and rculbertson May 20, 2025 09:21

rculbertson approved these changes May 23, 2025

View reviewed changes

thomasjpfan reviewed May 29, 2025

View reviewed changes

freider closed this Jun 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Dont use internal synchronicity UserCodeException type #3168

Dont use internal synchronicity UserCodeException type #3168

Uh oh!

freider commented May 20, 2025 •

edited by mwaskom

Loading

Uh oh!

freider May 20, 2025

Uh oh!

freider May 20, 2025

Uh oh!

freider May 20, 2025

Uh oh!

thomasjpfan May 29, 2025

Uh oh!

freider May 20, 2025

Uh oh!

freider commented May 20, 2025

Uh oh!

rculbertson commented May 23, 2025

Uh oh!

thomasjpfan May 29, 2025

Uh oh!

thomasjpfan May 29, 2025

Uh oh!

thomasjpfan May 29, 2025

Uh oh!

freider commented Jun 16, 2025

Uh oh!

Uh oh!

		raise exc.unwrap()


		async def _process_result_wrap_exceptions(result: api_pb2.GenericResult, data_format: int, stub, client=None):

		@@ -357,6 +356,35 @@ async def test_function_future_async(client, servicer):
		assert future.object_id not in servicer.cleared_function_calls # keep results around a bit longer for futures


		def test_function_spawn_exception(client, servicer):

		@@ -22,6 +22,19 @@ class Error(Exception):
		"""


		class _ContainerException(Exception):

	class _ContainerException(Exception):
	class _WrappedException(Exception):

Dont use internal synchronicity UserCodeException type #3168

Dont use internal synchronicity UserCodeException type #3168

Uh oh!

Conversation

freider commented May 20, 2025 • edited by mwaskom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Compatibility checklist

Release checklist

Changelog

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freider commented May 20, 2025

Uh oh!

rculbertson commented May 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freider commented Jun 16, 2025

Uh oh!

Uh oh!

freider commented May 20, 2025 •

edited by mwaskom

Loading