Skip to content

fix(epy): reraise exception groups instead of raising TypeError - #824

Open
itzzdev09 wants to merge 1 commit into
google:mainfrom
itzzdev09:fix/reraise-exception-group
Open

itzzdev09 wants to merge 1 commit into
google:mainfrom
itzzdev09:fix/reraise-exception-group

Conversation

@itzzdev09

Copy link
Copy Markdown

Problem

epy.reraise() / epy.maybe_reraise() destroy the exception they are supposed to annotate when it is an ExceptionGroup:

>>> from etils import epy
>>> try:
...   try:
...     raise ExceptionGroup('group msg', [ValueError('a'), TypeError('b')])
...   except Exception as e:
...     epy.reraise(e, prefix='Caught: ')
... except BaseException as out:
...   print(type(out).__name__, out)
TypeError BaseExceptionGroup.__new__() takes exactly 2 arguments (1 given)

wrap_error() creates class WrappedException(type(e)) and calls WrappedException(msg). That inherited __new__ is BaseExceptionGroup.__new__, which requires the sub-exceptions too, so construction raises and the caller sees a TypeError from inside etils instead of the real failure — with the sub-exceptions gone.

This is reachable from ordinary code: asyncio.TaskGroup raises an ExceptionGroup for any failing task, and except* handlers re-raise them, so an epy.maybe_reraise block around async work turns a real error into this TypeError.

Change

  • Rebuild groups through their own constructor (type(e).__new__(cls, msg, e.exceptions) and the matching __init__), so the sub-exceptions survive.
  • Keep the group's own __str__ / __repr__ instead of BaseException's, and build the message from e.message rather than str(e), so the (N sub-exceptions) suffix appears once rather than twice.
  • Initialize through BaseException.__init__ rather than Exception.__init__, so exceptions outside the Exception branch (KeyboardInterrupt,…) can be wrapped too. Previously that raised TypeError: descriptor '__init__' requires a 'Exception' object.

__new__ is only overridden for groups — OSError and friends need their own inherited __new__, which is why it cannot be replaced unconditionally.

Tests

Three tests added: a plain ExceptionGroup, a user subclass of it, and a BaseException. All three fail on main:

FAILED etils/epy/reraise_utils_test.py::test_reraise_exception_group - TypeError...
FAILED etils/epy/reraise_utils_test.py::test_reraise_exception_group_subclass
FAILED etils/epy/reraise_utils_test.py::test_reraise_base_exception - TypeError...
3 failed, 3 passed

and pass with the change (6 passed). The rest of etils/epy is unchanged by it; the 8 failures I see there locally are missing tensorflow_datasets in my environment, in lazy_imports_utils_test.py.

wrap_error() builds a subclass of the original exception and instantiates
it with a single message argument. BaseExceptionGroup.__new__ takes the
sub-exceptions as well, so every epy.reraise()/maybe_reraise() over an
ExceptionGroup replaced the original error with

  TypeError: BaseExceptionGroup.__new__() takes exactly 2 arguments (1 given)

losing the failure it was adding context to. Groups reach this path through
ordinary code: asyncio.TaskGroup raises one for any task failure.

Rebuild groups with their own constructor, keep their __str__ (so the
sub-exception count is not duplicated), and wrap the message from
.message rather than str(e). Also init through BaseException, so
exceptions outside the Exception branch can be wrapped.

@ADITYA-KUMAR-2358 ADITYA-KUMAR-2358 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the changes. The implementation correctly adds support for ExceptionGroup and BaseException handling, and the added tests cover message formatting, exception preservation, subclass handling, and KeyboardInterrupt. Everything looks good from my side.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. The change correctly handles ExceptionGroup construction and formatting while preserving the existing behavior for regular and BaseException types. Using e.message also avoids duplicating the sub-exception count in the wrapped message.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added tests provide good coverage for the new exception handling behavior. They verify correct formatting and preservation of sub-exceptions for ExceptionGroup, preservation of custom ExceptionGroup subclasses, and support for BaseException types such as KeyboardInterrupt. The assertions around the group message and unchanged sub-exceptions are particularly useful for guarding against regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants