Skip to content
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

[question] Proper way to implement ExceptionGroup.derive method with the existing overloads? #9922

Open
kkirsche opened this issue Mar 22, 2023 · 8 comments

Comments

@kkirsche
Copy link
Contributor

Good afternoon,

I was working to implement some custom exception groups in an application that I work on and have had a really hard time getting the types for the derive function to work correctly. Based on reading the documentation linked below, derive is a required method when overriding an exception group to return an instance of it's own class.

Reference

https://docs.python.org/3/library/exceptions.html#BaseExceptionGroup.derive

Reproduction / Example

To reproduce this error, I'll use the example shows in the BaseExceptionGroup page, but with type hints:

from __future__ import annotations

from collections.abc import Sequence
from typing import TypeVar

_ExceptionT_co = TypeVar("_ExceptionT_co", bound=Exception, covariant=True)
_ExceptionT = TypeVar("_ExceptionT", bound=Exception)


class MyGroup(ExceptionGroup[_ExceptionT_co]):
    def derive(self, excs: Sequence[_ExceptionT]) -> MyGroup[_ExceptionT]:
        return MyGroup(self.message, excs)

Mypy output:

❯ mypy ~/Desktop/example.py
/Users/kkirsche/Desktop/example.py:11: error: Signature of "derive" incompatible with supertype "BaseExceptionGroup"  [override]
/Users/kkirsche/Desktop/example.py:11: note:      Superclass:
/Users/kkirsche/Desktop/example.py:11: note:          @overload
/Users/kkirsche/Desktop/example.py:11: note:          def [_ExceptionT <: Exception] derive(self, Sequence[_ExceptionT], /) -> ExceptionGroup[_ExceptionT]
/Users/kkirsche/Desktop/example.py:11: note:          @overload
/Users/kkirsche/Desktop/example.py:11: note:          def [_BaseExceptionT <: BaseException] derive(self, Sequence[_BaseExceptionT], /) -> BaseExceptionGroup[_BaseExceptionT]
/Users/kkirsche/Desktop/example.py:11: note:      Subclass:
/Users/kkirsche/Desktop/example.py:11: note:          def [_ExceptionT <: Exception] derive(self, excs: Sequence[_ExceptionT]) -> MyGroup[_ExceptionT]
Found 1 error in 1 file (checked 1 source file)

Given that this is working off an exception group rather than a base exception group, I'm unclear on what the proper way to fix this would be and was wondering if the team here was able to provide an example that satisfies mypy / typeshed's overrides for BaseExceptionGroup's derive function.

Thoughts

Would it potentially make sense to add a derive method to ExceptionGroup which returns Self rather than a hardcoded ExceptionGroup? This seems to align with how the documentation describes using this behavior, but I'm not confident I understand fully to submit a pull request with recommended change(s).

Thanks for your time and help, hope you all are having a great day.

@AlexWaygood
Copy link
Member

Hi Kevin, hope you're well!

I'm paging @sobolevn on this one, who authored our current stack of overloads and test cases for ExceptionGroup, and I think understands this stuff much better than I do 😄

@sobolevn
Copy link
Member

I am sorry, my example was not correct. You can not have exception types there. Here are correct ones:

>>> BaseExceptionGroup('a', [SystemExit()]).derive([ValueError()])
ExceptionGroup('a', [ValueError()])

>>> ExceptionGroup('a', [ValueError()]).derive([SystemExit()])
BaseExceptionGroup('a', [SystemExit()])

This is why we have two overloads. And Self is not actually correct.

@kkirsche
Copy link
Contributor Author

kkirsche commented Mar 26, 2023

But isn't that specifically the base classes not what derive is meant to do when implemented by the user, as outlined in the linked example?

A subclass needs to override it in order to make subgroup() and split() return instances of the subclass rather than ExceptionGroup.

https://docs.python.org/3/library/exceptions.html#BaseExceptionGroup.derive

Just trying to understand how the base classes returning the exception group should be overloaded by the end user given the base exception group overload, specifically.

@sobolevn
Copy link
Member

Yes, this is just the base classes. Custom types can return whatever they like.
Do you have any suggestions on how to fix this?

@kkirsche
Copy link
Contributor Author

kkirsche commented Mar 26, 2023

Yes, this is just the base classes. Custom types can return whatever they like.

Do you have any suggestions on how to fix this?

I'm sorry, I was still trying to understand fully what the situation was. Based on your feedback, my understanding is that the example cannot currently be typed given the current overloads, as the separation of BaseException from Exception requires BaseExceptions to return BaseExceptionGroup which isn't a requirement of a custom exception group. Is that a correct understanding of your feedback?

Happy to help brainstorm solutions, I want to make sure I understand how it's supposed to work in a custom error group to understand it if there is an opportunity to change type hints or if instead it's more of a documentation change where the example should call out that handling BaseExceptions should be / may be necessary separate of the Exceptions (at least for type uses).

Thanks for your patience

@sobolevn
Copy link
Member

as the separation of BaseException from Exception requires BaseExceptions to return BaseExceptionGroup which isn't a requirement of a custom exception group. Is that a correct understanding of your feedback?

Yes.

Happy to help brainstorm solutions, I want to make sure I understand how it's supposed to work in a custom error group

I can think of several solutions.

  1. Simplify current annotations to return Self and ignore this corner case. How bad it can be for our users? I don't really think that users will use .derive to pass completely unrelated BaseException instances
  2. Require users to have # type: ignore comment on the overriden methods because this is how implementation works. Basically, users can return instance of their own class for any input

@kkirsche
Copy link
Contributor Author

I think I lean towards option 1 of those two solutions, if for no other reason than it aligns with the example use case outlined in the documentation.

An alternative I was wondering about is whether it would make sense to return a Protocol representing an ExceptionGroup or a BaseExceptionGroup instead of a type. That may be completely off base, please let me know if it is, as protocols aren't an area I'm very familiar with in Python.

Looking at the types of ExceptionGroup and BaseExceptionGroup, they seem to be largely compatible:

I haven't attempted to implement it yet, so there may be a gotcha I'm not aware of, but was wondering if this would make sense or not. What is your opinion on this idea?

@PIG208
Copy link
Contributor

PIG208 commented Aug 4, 2023

I ran into this issue as well when writing a custom ExceptionGroup class. The overloads are implementation details, but once we have a workaround, it would be great to document the proper use of type annotations of the example use case.

@AlexWaygood AlexWaygood changed the title [question] Proper way to implement ExceptionGroup.derive method with the existing overrides? [question] Proper way to implement ExceptionGroup.derive method with the existing overloads? Aug 4, 2023
@srittau srittau removed the question label Mar 23, 2024
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

No branches or pull requests

5 participants