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

Bump anyio dependency to latest major version #42

Closed
kwzrd opened this issue Dec 9, 2023 · 8 comments · Fixed by #43
Closed

Bump anyio dependency to latest major version #42

kwzrd opened this issue Dec 9, 2023 · 8 comments · Fixed by #43

Comments

@kwzrd
Copy link

kwzrd commented Dec 9, 2023

Hi, are there any plans to bump the anyio dependency to 4.x.y?

There was a fairly important change in version 4.0.0 where anyio now raises the Python-native ExceptionGroup rather than its own. This means that anyio users can now handle exceptions with except*.

I love using aiometer but being pinned to anyio~=3.2 prevents me from using the new Python-native exception group error handling.

Thanks!

@florimondmanca
Copy link
Owner

Hi,

There's no blocker against this, I would be happy to help get a PR reviewed and merged to add anyio 4 support :)

Let's try to target anyio >= 3.2 if we can?

@kwzrd
Copy link
Author

kwzrd commented Dec 9, 2023

Thanks for the quick reply!

I tried running the testing suite with anyio 4.1.0 and one of the tests fails, specifically this one:

test_amap_task_exception

The test asserts that if one of the tasks started in aiometer.amap fails, the exception is propagated to the caller. Under anyio 4, we now get the exception wrapped in an ExceptionGroup though.

If we retain support for anyio 3, how should we deal with this? The behaviour will differ between the supported anyio versions. We can make the test expect either the exception, or an ExceptionGroup wrapping it, but I'm not sure if that's acceptable for the users.

@florimondmanca
Copy link
Owner

florimondmanca commented Dec 10, 2023

Is the ExceptionGroup raised on all Python versions, or only most recent ones?

Does the ExceptionGroup contain only the exception raised by user code? I assume not. Then what's the other exception?

If there's only the user exception, would it make sense to unwrap the exception group so we keep the current behaviour?

Or should we embrace the new exception group stuff? I must say I haven't really kept on top of that and I'm not sure we can assume users will be comfortable handling exception groups either, so in that case we probably need some extra pointers in the docs.

One thing we can sure of is we don't want any anyio specific APIs to leak through.

@kwzrd
Copy link
Author

kwzrd commented Dec 10, 2023

Is the ExceptionGroup raised on all Python versions, or only most recent ones?

Does the ExceptionGroup contain only the exception raised by user code? I assume not. Then what's the other exception?

I tried on Python 3.8 and 3.11 and the behaviour is the same, the only difference is that on 3.8 we get an ExceptionGroup backport from exceptiongroup and on 3.11 we get the Python-native ExceptionGroup.

It really is just one exception in the group. The anyio release docs mention that after 4.0.0, "any exceptions raising out of a task groups are now nested inside an ExceptionGroup."

In fact, aiometer.amap now raises the following:

- Exception Group with 1 sub-exception
- - Exception Group with 1 sub-exception
- - - Failure (exception defined in the test)

Looking at the implementation, I think this is because there are two nested task groups - one in amap, and one in run_on_each. So because anyio no longer unwraps the exception if only 1 exception occurs, the nesting of exceptions is proportional to the nesting of task groups.

If there's only the user exception, would it make sense to unwrap the exception group so we keep the current behaviour?

I'm not sure, but I'm leaning towards no. I don't think that's something that should happen in aiometer. It's unfortunate that it is a breaking change, but I think anyio went for it because it is consistent with Python-native task groups, where any exception is wrapped in an ExceptionGroup, even if there is only 1. It seems to be a more consistent and predictable API. So my personal opinion would be that aiometer shouldn't diverge from what's become the "standard" behaviour in the standard library.

@florimondmanca
Copy link
Owner

Thanks for the input and bouncing off of ideas.

It's interesting that the anyio migration guide suggests to do this kind of unwrapping "If you need to stay compatible with both AnyIO 3 and 4". I just opened #43 as an attempt of that.

@florimondmanca
Copy link
Owner

florimondmanca commented Dec 10, 2023

@kwzrd #43 is passing. Happy to read your thoughts on this approach?

This would lead to a non-breaking release of aiometer, as 0.5.0.

We could add ExceptionGroup support to aiometer in the future, so that it's possible to really get all exceptions that might happen concurrently as tasks execute.

But I think it's okay treat that as a follow-up enhancement, with its own proper compatibility and upgrade approach, rather than a blocker for anyio 4 support, since we don't have it now anyways.

I understand you were interested in ExceptionGroup support so once we get anyio 4 unblocked, I'd be happy to read your input on how to add support for that in aiometer, and how you'd use it in practice (possibly with some motivating example code?).

@kwzrd
Copy link
Author

kwzrd commented Dec 10, 2023

Looks great! Yeah, this approach makes sense to me.

I understand you were interested in ExceptionGroup support so once we get anyio 4 unblocked, I'd be happy to read your input on how to add support for that in aiometer, and how you'd use it in practice (possibly with some motivating example code?).

I think this solves my use case already. I primarily just want to be able to use except* when calling aiometer in application code. As long as I use Python 3.11 and higher, I should now receive native ExceptionGroups (if multiple errors occur) or unwrapped exceptions. I can handle both with a single except* clause, which is what I needed.

The general problem was that we use Python task groups as well as aiometer for metering, so we were getting different types of ExceptionGroups around the codebase, which made error handling confusing. Now it should be all good!

@florimondmanca
Copy link
Owner

Right, the compat code only unwraps single exception groups. 👍

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 a pull request may close this issue.

2 participants