remove multierror #2891

jakkdl · 2023-11-27T15:56:31Z

The companion to #2886, see #2785

Clean up comments and test names to not refer to multierror [in present tense]
update docs
add newsfragment
and probably some more tests in test_multierror that could be retired.
And files/directories referencing "multierror" to be renamed.

codecov · 2023-11-27T15:58:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d4ce2f9) 99.49% compared to head (3e485dc) 99.65%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2891      +/-   ##
==========================================
+ Coverage   99.49%   99.65%   +0.16%     
==========================================
  Files         115      115              
  Lines       17704    17310     -394     
  Branches     3158     3110      -48     
==========================================
- Hits        17615    17251     -364     
+ Misses         70       40      -30     
  Partials       19       19

Files	Coverage Δ
src/trio/__init__.py	`100.00% <100.00%> (ø)`
src/trio/_core/_concat_tb.py	`100.00% <100.00%> (ø)`
src/trio/_core/_run.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_exceptiongroup_gc.py	`100.00% <100.00%> (ø)`
src/trio/_core/_tests/test_run.py	`100.00% <100.00%> (ø)`
src/trio/_highlevel_open_tcp_stream.py	`100.00% <100.00%> (ø)`
src/trio/_tests/test_exports.py	`99.60% <ø> (ø)`

... and 3 files with indirect coverage changes

…nner check not being run

…n some cases

…at_tb.py

jakkdl · 2023-11-28T15:32:33Z

I experimented with removing the code that's supposedly there to fix test_MultiError_catch_doesnt_create_cyclic_garbage and it didn't seem to break it - but test_locals_destroyed_promptly_on_cancel did start failing.
@A5rocks are those workarounds still needed? Can the cyclic garbage test be retired? Should I change those comments to refer to the destroyed_promptly test instead?

jakkdl · 2023-11-28T15:46:47Z

do we want a newsfragment for now relying on exceptiongroup's apport excepthook monkeypatching?

…taining backwards-compatibility from raising multierror's

richardsheridan · 2023-11-28T21:21:37Z

I experimented with removing the code that's supposedly there to fix test_MultiError_catch_doesnt_create_cyclic_garbage and it didn't seem to break it - but test_locals_destroyed_promptly_on_cancel did start failing. @A5rocks are those workarounds still needed? Can the cyclic garbage test be retired? Should I change those comments to refer to the destroyed_promptly test instead?

Ya these cyclic gc tests have somewhat overlapping behavior, since we made them by tracking down cycles basically one-by-one in various scenarios. I'd say it's not trio's job to check if exception groups create garbage (anymore). The question is whether a similar test exists in the suite for exception groups in cpython or the backport.

richardsheridan · 2023-11-28T21:22:40Z

Should I change those comments to refer to the destroyed_promptly test instead?

oh and yes please on this

CoolCat467 · 2023-11-29T00:50:47Z

I totally agree with using python's built in ExceptionGroups, but what about python versions pre-3.11? My worry is for one of my own projects, I support 3.9 and up, but ExceptionGroup doesn't exist pre-3.11.

My particular use case of MultiError:
Shim code:
https://github.com/CoolCat467/Scanner-Server/blob/0a64a32fee92a4b9e3d80ad46d9c9b7fcb595f65/src/sanescansrv/server.py#L68-L71
Main usage:
https://github.com/CoolCat467/Scanner-Server/blob/0a64a32fee92a4b9e3d80ad46d9c9b7fcb595f65/src/sanescansrv/server.py#L645-L657

If I were using 3.11 + I would just use except * but that's not a thing in older python versions.

CoolCat467 · 2023-11-29T01:03:25Z

I totally agree with using python's built in ExceptionGroups, but what about python versions pre-3.11? My worry is for one of my own projects, I support 3.9 and up, but ExceptionGroup doesn't exist pre-3.11.

My particular use case of MultiError: Shim code: https://github.com/CoolCat467/Scanner-Server/blob/0a64a32fee92a4b9e3d80ad46d9c9b7fcb595f65/src/sanescansrv/server.py#L68-L71 Main usage: https://github.com/CoolCat467/Scanner-Server/blob/0a64a32fee92a4b9e3d80ad46d9c9b7fcb595f65/src/sanescansrv/server.py#L645-L657

If I were using 3.11 + I would just use except * but that's not a thing in older python versions.

Oh I see, we can just use external exceptiongroup module. Sorry about that, I didn't know that was a thing.

CoolCat467

Looks good other than a few small things I noticed.

src/trio/_core/_concat_tb.py

CoolCat467 · 2023-11-29T00:57:27Z

src/trio/_core/_run.py

+                if not self._strict_exception_groups and len(self._pending_excs) == 1:
+                    return self._pending_excs[0]
+                return BaseExceptionGroup(
+                    "collapsible" if not self._strict_exception_groups else "TODO",


What needs to be done in TODO here?

The exception requires specifying a message, so we probably want some informative message on the cause of the exception or something.

Made it a somewhat generic "Exceptions from Trio nursery". Am very open to suggestions for what else could be added.

CoolCat467 · 2023-11-29T01:12:44Z

Oh I see, we can just use external exceptiongroup module. Sorry about that, I didn't know that was a thing.

Fixed as of CoolCat467/Scanner-Server@9ad2774

jakkdl · 2023-11-29T11:54:36Z

Oh I see, we can just use external exceptiongroup module. Sorry about that, I didn't know that was a thing.

Fixed as of CoolCat467/Scanner-Server@9ad2774

trio also has an exceptiongroup dependency on <3.11

jakkdl · 2023-11-29T12:05:48Z

I experimented with removing the code that's supposedly there to fix test_MultiError_catch_doesnt_create_cyclic_garbage and it didn't seem to break it - but test_locals_destroyed_promptly_on_cancel did start failing. @A5rocks are those workarounds still needed? Can the cyclic garbage test be retired? Should I change those comments to refer to the destroyed_promptly test instead?

Ya these cyclic gc tests have somewhat overlapping behavior, since we made them by tracking down cycles basically one-by-one in various scenarios. I'd say it's not trio's job to check if exception groups create garbage (anymore). The question is whether a similar test exists in the suite for exception groups in cpython or the backport.

I'm not finding anything with a quick search in the backport @agronholm

I'm not familiar with the inner workings of gc and the implementation of multierror/exceptiongroup, but wouldn't the need for such tests depend on the manner of implementation? If the garbage was caused by hacky stuff that multierror needed to do in order to work, and cpython manages to sidestep that completely by implementing stuff """properly""", then there's no direct need for the tests on their side and/or they might have to look quite different.

agronholm · 2023-11-29T12:08:13Z

I experimented with removing the code that's supposedly there to fix test_MultiError_catch_doesnt_create_cyclic_garbage and it didn't seem to break it - but test_locals_destroyed_promptly_on_cancel did start failing. @A5rocks are those workarounds still needed? Can the cyclic garbage test be retired? Should I change those comments to refer to the destroyed_promptly test instead?

Ya these cyclic gc tests have somewhat overlapping behavior, since we made them by tracking down cycles basically one-by-one in various scenarios. I'd say it's not trio's job to check if exception groups create garbage (anymore). The question is whether a similar test exists in the suite for exception groups in cpython or the backport.

I'm not finding anything with a quick search in the backport @agronholm

I'm not familiar with the inner workings of gc and the implementation of multierror/exceptiongroup, but wouldn't the need for such tests depend on the manner of implementation? If the garbage was caused by hacky stuff that multierror needed to do in order to work, and cpython manages to sidestep that completely by implementing stuff """properly""", then there's no direct need for the tests on their side and/or they might have to look quite different.

I'd add such tests to the backport only as regression tests if such a problem is detected there.

jakkdl · 2023-11-29T12:21:38Z

oh actually, I am getting cyclic garbage test fails in some cases. Will push a commit with updated comments.
EDIT: Ah, it's specifically test_cancel_scope_exit_doesnt_create_cyclic_garbage that can fail, haven't managed to trigger any of the others.

…uses what

jakkdl · 2023-11-29T12:42:54Z

but test_locals_destroyed_promptly_on_cancel did start failing.

Strangely enough I'm not managing to make this test fail now

richardsheridan · 2023-11-29T12:54:30Z

Just to be completely clear, every spooky del in the codebase removes a cycle or shortens the lifetime of a strong reference in a way that can be statically reasoned about. Some of these we were clever enough to also create regression tests. I did my best at the time to cross reference these, by going through each del of the PR and commenting out the fix and seeing which test failed, or putting a TODO where nothing failed.

…update comments, add test_nursery_loose_exception_groups, add string for the exceptiongroup

richardsheridan

I think all the cyclic garbage changes are making sense. Hanging on to test_simple_cancel_scope_usage_doesnt_create_cyclic_garbage even though it maybe can't fail currently is fine IMO since we want to cover outcome and protect future refactoring.

src/trio/_core/_run.py

Zac-HD

Two trivial nitpicks, but overall I think this is ready to merge.

I'd also be keen to ship a new version with this change ASAP, so that we can see it working in the wild before shipping #2886. That should reduce the risk of complications for everyone...

src/trio/_core/_tests/test_multierror.py

src/trio/_core/_tests/test_run.py

src/trio/_core/_run.py

A5rocks

Sorry about the wait; essentially just nitpicks (even about code you just copied over from elsewhere instead of writing :-) and I paid less attention to the tests.

A5rocks · 2023-12-19T07:00:43Z

src/trio/__init__.py

-    ),
-    "NonBaseMultiError": _deprecate.DeprecatedAttribute(
-        value=_NonBaseMultiError,
-        version="0.22.0",


So I guess any release with these should be 0.24.0? Release preparation page says "two months or two releases, whichever is longer") and I assume it means minor releases.

makes sense to do major (or whatever 0.X releases are called) release on both this and on #2886 since they can break stuff.

I still don't get why Trio doesn't have a 1.0.0 release yet if compatibility guarantees are this stringent anyway.

I'd be inclined to go 1.0 when we've got this and strict-by-default in; I agree but it seems odd to make that release right before some substantial changes.

src/trio/_core/_concat_tb.py

src/trio/_core/_run.py

A5rocks · 2023-12-19T07:20:42Z

src/trio/_core/_tests/test_exceptiongroup_gc.py

+# Unclear if this can still fail, removing the `del` from _concat_tb.copy_tb does not seem
+# to trigger it (on a platform where the `del` is executed)


Huh, it might be a good idea to remove this test then and add a TODO to that del that it isn't tested for.

It seems like a good idea to try to remove unnecessary dels if things have improved!

see #2891 (comment) on removing unnecessary dels.
But copy_tb is possibly an exception since it no longer uses MultiError.

A5rocks · 2023-12-19T07:23:04Z

src/trio/_core/_tests/test_run.py

+# I don't know if this one can fail anymore, the `del` next to the comment that used to
+# refer to this only seems to break test_cancel_scope_exit_doesnt_create_cyclic_garbage


Same opinion here as for my other comment. Maybe others have a differing opinion, but rationally I think removing tests is good (though emotionally, not so much :'( ).

@richardsheridan said in #2891 (review)

Hanging on to test_simple_cancel_scope_usage_doesnt_create_cyclic_garbage even though it maybe can't fail currently is fine IMO since we want to cover outcome and protect future refactoring.

I'd probably weakly be in favor of removing the test, but if not should probably update the comment to include sheridan's reasoning.

though concat_tb is only used for collapse_exception_group, which is only used for strict_exception_groups=False, which we might deprecate soonish. So not sure there's much future refactoring to consider.

Yeah updating the comment would be nice

Ah oops, I mixed up the tests - the latter half of my previous comment pertains to test_ExceptionGroup_catch_doesnt_create_cyclic_garbage.
Updated the comment though.

Co-authored-by: EXPLOSION <git@helvetica.moe>

remove multierror

5ff3464

jakkdl added 5 commits November 27, 2023 17:36

remove more unused code, fix test_cancel_scope_multierror_filtering i…

9c23ab0

…nner check not being run

fix remaining failing tests, instantly collapse groups in nurseries i…

4cb329f

…n some cases

bump exceptiongroup version to one that has the apport excepthook

3822dc1

remove multierror_scripts, rename _core/_multierror.py to _core/_conc…

f52ea57

…at_tb.py

edit some comments, remove test_ExceptionGroupNotHashable

4d600bc

jakkdl marked this pull request as ready for review November 28, 2023 15:28

Merge branch 'master' into multierror_removal

091e962

jakkdl added 3 commits November 28, 2023 17:41

docs: update some references to MultiError, remove section about main…

8246daf

…taining backwards-compatibility from raising multierror's

add newsfragment

a7a5a71

Merge remote-tracking branch 'origin/master' into multierror_removal

bfca5a5

CoolCat467 approved these changes Nov 29, 2023

View reviewed changes

update gc comments to my current state of understanding as to what ca…

5dd4b95

…uses what

This was referenced Dec 6, 2023

Add RaisesGroup, a helper for catching ExceptionGroups in tests #2898

Merged

change strict_exception_groups default to True #2886

Merged

Merge remote-tracking branch 'origin/master' into multierror_removal

47ef84a

jakkdl added 3 commits December 16, 2023 12:54

Merge remote-tracking branch 'origin/master' into multierror_removal

95252ac

move 'collapsible' to __notes__, clean up some stray unused-ignores, …

9c1400c

…update comments, add test_nursery_loose_exception_groups, add string for the exceptiongroup

update exceptiongroup message, test for it with a match

907a2c5

jakkdl requested a review from richardsheridan December 16, 2023 12:34

fix expected message

3319a01

jakkdl requested review from Zac-HD and A5rocks December 16, 2023 12:34

fix mypy (this should be checked by pre-commit...)

9941762

richardsheridan approved these changes Dec 16, 2023

View reviewed changes

CoolCat467 reviewed Dec 16, 2023

View reviewed changes

src/trio/_core/_run.py Outdated Show resolved Hide resolved

Zac-HD approved these changes Dec 17, 2023

View reviewed changes

src/trio/_core/_tests/test_multierror.py Outdated Show resolved Hide resolved

src/trio/_core/_tests/test_run.py Show resolved Hide resolved

src/trio/_core/_run.py Outdated Show resolved Hide resolved

jakkdl added 2 commits December 18, 2023 13:28

add test for loose exceptiongroup collapsing, rename test_multierror

6977353

fix test

bd72a32

A5rocks reviewed Dec 19, 2023

View reviewed changes

jakkdl and others added 5 commits December 19, 2023 11:51

Apply suggestions from code review

0439e66

Co-authored-by: EXPLOSION <git@helvetica.moe>

update comments after review from a5rocks

e19f341

make the note more verbose

b0eb6b4

Merge branch 'master' into multierror_removal

d4463eb

fix the test

f433fdb

jakkdl requested a review from A5rocks December 20, 2023 15:44

A5rocks approved these changes Dec 21, 2023

View reviewed changes

update comment

3e485dc

jakkdl merged commit 9fbcb38 into python-trio:master Dec 21, 2023
29 checks passed

jakkdl deleted the multierror_removal branch December 21, 2023 21:19

TeamSpen210 mentioned this pull request Dec 24, 2023

Change defer_to_* to be based on [Base]ExceptionGroup groove-x/trio-util#23

Closed

nicoddemus mentioned this pull request Jan 10, 2024

Initial implementation of RaisesGroup pytest-dev/pytest#11671

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove multierror #2891

remove multierror #2891

jakkdl commented Nov 27, 2023 •

edited

Loading

codecov bot commented Nov 27, 2023 •

edited

Loading

jakkdl commented Nov 28, 2023

jakkdl commented Nov 28, 2023

richardsheridan commented Nov 28, 2023

richardsheridan commented Nov 28, 2023

CoolCat467 commented Nov 29, 2023

CoolCat467 commented Nov 29, 2023

CoolCat467 left a comment

CoolCat467 Nov 29, 2023

jakkdl Nov 29, 2023

jakkdl Dec 16, 2023

CoolCat467 commented Nov 29, 2023

jakkdl commented Nov 29, 2023

jakkdl commented Nov 29, 2023

agronholm commented Nov 29, 2023

jakkdl commented Nov 29, 2023 •

edited

Loading

jakkdl commented Nov 29, 2023

richardsheridan commented Nov 29, 2023

richardsheridan left a comment

Zac-HD left a comment

A5rocks left a comment

A5rocks Dec 19, 2023

jakkdl Dec 19, 2023

agronholm Dec 19, 2023

Zac-HD Dec 19, 2023

A5rocks Dec 19, 2023

jakkdl Dec 19, 2023

A5rocks Dec 19, 2023

jakkdl Dec 19, 2023

A5rocks Dec 21, 2023

jakkdl Dec 21, 2023

		# Unclear if this can still fail, removing the `del` from _concat_tb.copy_tb does not seem
		# to trigger it (on a platform where the `del` is executed)

		# I don't know if this one can fail anymore, the `del` next to the comment that used to
		# refer to this only seems to break test_cancel_scope_exit_doesnt_create_cyclic_garbage

remove multierror #2891

remove multierror #2891

Conversation

jakkdl commented Nov 27, 2023 • edited Loading

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

jakkdl commented Nov 28, 2023

jakkdl commented Nov 28, 2023

richardsheridan commented Nov 28, 2023

richardsheridan commented Nov 28, 2023

CoolCat467 commented Nov 29, 2023

CoolCat467 commented Nov 29, 2023

CoolCat467 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoolCat467 commented Nov 29, 2023

jakkdl commented Nov 29, 2023

jakkdl commented Nov 29, 2023

agronholm commented Nov 29, 2023

jakkdl commented Nov 29, 2023 • edited Loading

jakkdl commented Nov 29, 2023

richardsheridan commented Nov 29, 2023

richardsheridan left a comment

Choose a reason for hiding this comment

Zac-HD left a comment

Choose a reason for hiding this comment

A5rocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Nov 27, 2023 •

edited

Loading

codecov bot commented Nov 27, 2023 •

edited

Loading

jakkdl commented Nov 29, 2023 •

edited

Loading