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

Reinstate "collapse exception group" in TaskGroup exception handling #641

Closed
1 task done
rutger-clearblue opened this issue Nov 24, 2023 · 8 comments
Closed
1 task done
Labels
enhancement New feature or request

Comments

@rutger-clearblue
Copy link

Things to check first

  • I have searched the existing issues and didn't find my feature already requested there

Feature description

First of all, many thanks for this excellent library.

The TaskGroup for the asyncio backend used a nice collapse_exception_group() function that keeps exception messages and stacktraces as clear as possible when only a single exception happens in a task group.
It was removed in PR #586 and I wonder if it is possible to get that functionality back?

https://github.com/agronholm/anyio/pull/586/files#diff-202c3fc4551e5a287df488a9bd96a25dcc1405ec135d5c7aceb6ef4c02755341L678

Use case

The difference in readability of stacktraces between a single exception and an exception group is quite big. In Starlette/FastAPI for example the main exception messages have become "unhandled errors in a TaskGroup".

@rutger-clearblue rutger-clearblue added the enhancement New feature or request label Nov 24, 2023
@agronholm
Copy link
Owner

First of all, asyncio.TaskGroup doesn't collapse these, and Trio is also moving in that same direction, so it doesn't make sense for AnyIO to suddenly reverse course.

Also, FastAPI doesn't even support AnyIO 4 yet, so how are you seeing this in FastAPI apps?

@rutger-clearblue
Copy link
Author

Starlette supports AnyIO 4 and FastAPI seems to have no issues with it either.

It makes sense for anyio to go in the same direction as the frameworks it builds upon and there are probably some good reasons for not collapsing exception groups. However, for the developing and monitoring (e.g. Sentry) experience it is such a step back that I resolved to patching the TaskGroup class. I guess the tooling still needs to grow around this.

@agronholm
Copy link
Owner

FastAPI pins Starlette to a version that doesn't have proper support for exception groups. I know this because I'm the one who patched Starlette to support AnyIO 4. Starlette 0.28 introduces a breaking change which FastAPI has not been able to adapt to yet, and as such, it has pinned AnyIO to < 4.

@agronholm
Copy link
Owner

I agree that the lack of tooling thus far to support exception groups is a bit frustrating. But then again, the very concept of exception groups is pretty novel in the entire field of programming AFAIK, and no other language has them yet.

@rutger-clearblue
Copy link
Author

Ok, interesting. FYI this is the combination we run, and seems to work:

fastapi            0.95.0
starlette          0.26.1
anyio              4.0.0

fastapi==0.95.0
└── starlette<0.27.0,>=0.26.1
    └── anyio<5,>=3.4.0

@agronholm
Copy link
Owner

Well, just consider yourself warned that this is not a supported combination.

At my day job we're probably switching to Litestar, as the relationship between FastAPI and Starlette is a constant source of trouble.

@agronholm
Copy link
Owner

Incidentally, I just saw this: python-trio/trio#2886
Looks like Trio is moving to make this the default behavior on their end now.

@A5rocks
Copy link

A5rocks commented Nov 25, 2023

It makes sense for anyio to go in the same direction as the frameworks it builds upon and there are probably some good reasons for not collapsing exception groups. However, for the developing and monitoring (e.g. Sentry) experience it is such a step back that I resolved to patching the TaskGroup class. I guess the tooling still needs to grow around this.

Saw the pingback to trio and just wanted to comment the reasoning as I understand it!

Basically, this comes down to the fact that except ExceptionName will not catch an exception group containing ExceptionName. [i had more explanation here but TBH the link explains it much better :D]

See https://discuss.python.org/t/flat-exception-groups-alternative-to-pep-654/10433#pep-654-3 for a more elaborate explanation of why we need to wrap everything in ExceptionGroup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants