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

Replace MultiError with BaseExceptionGroup #2213

Merged
merged 104 commits into from
Sep 27, 2022
Merged

Replace MultiError with BaseExceptionGroup #2213

merged 104 commits into from
Sep 27, 2022

Conversation

agronholm
Copy link
Contributor

@agronholm agronholm commented Jan 16, 2022

This PR has two dependencies:

Closes #2211.
Closes #2427.

@codecov
Copy link

codecov bot commented Jan 16, 2022

Codecov Report

Merging #2213 (80889b2) into master (af66bcf) will increase coverage by 0.01%.
The diff coverage is 99.52%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2213      +/-   ##
==========================================
+ Coverage   98.98%   99.00%   +0.01%     
==========================================
  Files         119      117       -2     
  Lines       16165    16119      -46     
  Branches     3101     3122      +21     
==========================================
- Hits        16001    15958      -43     
+ Misses        114      113       -1     
+ Partials       50       48       -2     
Impacted Files Coverage Δ
trio/_core/__init__.py 100.00% <ø> (ø)
trio/tests/test_highlevel_open_tcp_listeners.py 98.25% <75.00%> (-1.75%) ⬇️
trio/__init__.py 100.00% <100.00%> (ø)
trio/_core/_multierror.py 100.00% <100.00%> (ø)
trio/_core/_run.py 100.00% <100.00%> (ø)
trio/_core/tests/test_multierror.py 100.00% <100.00%> (ø)
trio/_core/tests/test_run.py 100.00% <100.00%> (ø)
trio/_highlevel_open_tcp_listeners.py 100.00% <100.00%> (ø)
trio/_highlevel_open_tcp_stream.py 97.53% <100.00%> (+0.19%) ⬆️
trio/tests/test_highlevel_open_tcp_stream.py 100.00% <100.00%> (ø)
... and 3 more

The __cause__ of the OSError will always be an ExceptionGroup going forward.
MultiError's constructor, given a single element exception array, returned just that, but ExceptionGroup does not work the same way.
@agronholm agronholm marked this pull request as ready for review January 26, 2022 00:28
@agronholm
Copy link
Contributor Author

This needs discussion on whether to drop MultiError cold turkey or to have it inherit from BaseExceptionGroup. It's not yet certain if inheriting it is possible in the first place.

@agronholm
Copy link
Contributor Author

Here are my thoughts about deprecating MultiError. If we were to deprecate it and have it inherit from BaseExceptionGroup instead, what would be the migration path? There are two cases how it might be used by downstream users:

  1. Raising them
  2. Catching them (or their sub-exceptions)

To maintain backwards compatibility, both use cases would have to be covered. Trio itself would have to raise MultiError with warning suppression turned on to counter the deprecation warning we would have have its constructor emit. Then we would have to have the extra class methods (catch() and filter()) raise deprecation warnings as well. This only leaves one hole: cases where users catch MultiError directly. We can't emit a deprecation warning from the exceptions property because that would also happen for the users doing the right thing (catching BaseExceptionGroup instead).

Either way, a migration guide would probably be in order.

@smurfix
Copy link
Contributor

smurfix commented Jan 30, 2022

Meh. Frankly I don't think we can achieve 100% backwards compatibility, which is especially bad since the affected code is on code paths that are typically not tested very well.

Thus I'd advise to spend the effort on a good migration guide that covers all the bases, preferably from multiple angles. Code that wants to be able to handle both BaseExceptionGroup and MultiError will just have to duplicate the affected error handlers for a year or two.

@agronholm
Copy link
Contributor Author

Meh. Frankly I don't think we can achieve 100% backwards compatibility, which is especially bad since the affected code is on code paths that are typically not tested very well.

Thus I'd advise to spend the effort on a good migration guide that covers all the bases, preferably from multiple angles. Code that wants to be able to handle both BaseExceptionGroup and MultiError will just have to duplicate the affected error handlers for a year or two.

This seems to be the consensus (on Gitter as well). I will attempt to write a migration guide somewhere in the docs.

@agronholm
Copy link
Contributor Author

I amended the documentation and the news fragment to point users in the right direction. While this is not strictly a "How to migrate from MultiError to BaseExceptionGroup", trio users tend to be more tech savvy than the average Python user. I hope that is enough.

Now it works with BaseExceptionGroup too.
@agronholm
Copy link
Contributor Author

In terms of the ideal long-run state, I agree with you. In terms of the actual experience of our users when this is released, dropping the IPython monkeypatch is a major regression: exception group tracebacks will silently show only some of the frames. I think we need to carry that monkeypatch until IPython actually upstreams proper exception group support.

I've restored the IPython custom error handler, even though I still feel this is the wrong place for it. It also now supports (Base)ExceptionGroup.

docs/source/design.rst Outdated Show resolved Hide resolved
trio/_core/_multierror.py Show resolved Hide resolved
trio/_core/_multierror.py Show resolved Hide resolved
@oremanj
Copy link
Member

oremanj commented Sep 24, 2022

I've restored the IPython custom error handler, even though I still feel this is the wrong place for it.

Thanks. Hopefully IPython will implement proper EG support (including the backport) and we can remove Trio's patching of it soon.

Even though the Ubuntu wiki states that Apport is disabled by default? And what warning would they get?

I don't have easy access to an Ubuntu machine right now, but IIRC ubuntu installs a custom sys.excepthook unconditionally, which then calls the default one unless Apport is enabled. But it still looks like a custom excepthook so Trio won't install its own, so you don't get MultiError tracebacks.

I guess the exceptiongroup backport doesn't print a warning like Trio previously would if it fails to patch sys.excepthook. So users won't get a confusing warning... instead, they'll silently fail to see the "multi" parts of the traceback, which is even worse UX. I think that warning is fairly important in helping people understand what's going on.

@agronholm
Copy link
Contributor Author

I guess the exceptiongroup backport doesn't print a warning like Trio previously would if it fails to patch sys.excepthook. So users won't get a confusing warning... instead, they'll silently fail to see the "multi" parts of the traceback, which is even worse UX. I think that warning is fairly important in helping people understand what's going on.

I'm running Ubuntu on my home server and I can confirm that outside of a virtualenv, the apport hook is indeed present and will prevent exceptiongroup from installing its own hook. So, to not degrade the trio UX, I will fix this by adapting the old trio apport exceptionhook.

@agronholm
Copy link
Contributor Author

Code-wise this PR should be fine now.

trio/_core/_multierror.py Outdated Show resolved Hide resolved
trio/_core/_multierror.py Show resolved Hide resolved
trio/_core/_run.py Outdated Show resolved Hide resolved
trio/_core/_multierror.py Show resolved Hide resolved
@pquentin
Copy link
Member

Thanks for your hard work, @agronholm and @oremanj! We're lucky to have you. I'm really excited!

excited

Will release this tomorrow.

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.

Except hook incompatibility with exceptiongroup backport Replace MultiError with (Base)ExceptionGroup
7 participants