FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-45292: [PEP 654] add the ExceptionGroup and BaseExceptionGroup classes by iritkatriel · Pull Request #28569 · python/cpython · GitHub

/ cpython Public

bpo-45292: [PEP 654] add the ExceptionGroup and BaseExceptionGroup classes - #28569

Merged
iritkatriel merged 40 commits into
python:mainfrom
iritkatriel:bpo-45292-ExceptionGroup
Oct 22, 2021
Merged

bpo-45292: [PEP 654] add the ExceptionGroup and BaseExceptionGroup classes#28569
iritkatriel merged 40 commits into
python:mainfrom
iritkatriel:bpo-45292-ExceptionGroup

Conversation

iritkatriel commented Sep 26, 2021
edited
Loading

Copy link
Copy Markdown
Member

This adds the ExceptionGroup and BaseExceptionGroup classes, but does not yet update the traceback display code to work with them correctly.

https://bugs.python.org/issue45292

iritkatriel requested a review from a team as a code owner September 26, 2021 14:32
iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 26, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit c26ff6b38333ab6c990791628f827c5ab97998c2 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 26, 2021
iritkatriel changed the title bpo-45292: [PEP 654] added ExceptionGroup and BaseExceptionGroup (did… bpo-45292: [PEP 654] add ExceptionGroup and BaseExceptionGroup (did… Sep 26, 2021
iritkatriel changed the title bpo-45292: [PEP 654] add ExceptionGroup and BaseExceptionGroup (did… bpo-45292: [PEP 654] add ExceptionGroup and BaseExceptionGroup Sep 26, 2021
iritkatriel changed the title bpo-45292: [PEP 654] add ExceptionGroup and BaseExceptionGroup bpo-45292: [PEP 654] add the ExceptionGroup and BaseExceptionGroup classes Sep 26, 2021
pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 26, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit c26ff6b38333ab6c990791628f827c5ab97998c2 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 26, 2021
iritkatriel force-pushed the bpo-45292-ExceptionGroup branch from 4907240 to 6c6164a Compare September 26, 2021 18:19

Copy link
Copy Markdown
Member Author

This is part 1 - it rebased very easily so not much has changed in the area since April.

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Make sure you run the buildbot for leak checks.

I'd recommend asking for a review from a core dev who is better than me at catching subtle mistakes in C code (Serhiy?).

Comment thread Lib/test/test_pickle.py Outdated
Comment thread Lib/test/test_pickle.py Outdated
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated

Copy link
Copy Markdown
Member Author

Make sure you run the buildbot for leak checks.

Done.

I'd recommend asking for a review from a core dev who is better than me at catching subtle mistakes in C code (Serhiy?).

Would be great to have a review from @serhiy-storchaka if he's got the time and interest, and also @vstinner and @ncoghlan .

Comment thread Lib/test/test_exception_group.py Outdated
Comment thread Lib/test/test_exception_group.py Outdated
Comment on lines +79 to +81
self.assertIs(
type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This also seems weird to me: I'd expect that if I want MyEG to hold a BaseException, I'd better create a MyBaseEG to do so, ala:

class MyBEG(BaseExceptionGroup):
    @staticmethod
    def create(msg, excs):
        # per creation suggestion above
        if all(isinstance(e, Exception) for e in excs):
            return MyEG(msg, excs)
        return MyBEG(msg, excs)

class MyEG(MyBEG, ExceptionGroup):
    pass

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For user-defined subclasses, we have to punt on the special behavior. And yes, user classes should probably do what you show, but we don't want to enforce it. (That would be making assumptions.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fair enough!

IMO defining custom ExceptionGroup subclasses should be very rare anyway (maybe just Hypothesis and Trio?), and we can certainly deal with whatever complexity that requires.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Eh, why would Hypothesis need to subclass EG? While we were called on arbitrarily forbidding subclassing (as we did in an earlier PEP draft) I don't see why it would be useful. What is your use case? Maybe there's something we can add to the design to avoid the need? (For Trio I could imagine they'd want to provide some kind of backwards compatibility with Trio's current solution.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe a subclass to annotate the nested exceptions? See https://discuss.python.org/t/accepting-pep-654-exception-groups-and-except/10813/9

Zac-HD Oct 17, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd prefer not to change the exception type if we can avoid it, but the more important objection is that it complicates the reporting solely for implementation reasons:

class Explanation(Exception):
    __module__ = "builtins"
    def __str__(self) -> str:
        return f"\n{self.args[0]}"

try:
    why = "Failed!"
    raise AssertionError(why)
except Exception as e:
    msg = "    You can reproduce this error by ...\n    ..."
    raise Explanation(msg) from e

    # Ideally something more like:
    e.__note__ = msg
    raise
$ python example.py
Traceback (most recent call last):
  File "example.py", line 8, in <module>
    raise AssertionError(why)
AssertionError: Failed!
                                                                        # These lines are
The above exception was the direct cause of the following exception:    # confusing for new 
                                                                        # users, and they
Traceback (most recent call last):                                      # only exist due 
  File "example.py", line 10, in <module>                               # to implementation
    raise Explanation(msg) from e                                       # constraints :-(
Explanation:                                                            # 
    You can reproduce this error by ...
    ...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hm, that does not look very principled. Why make it possible to print the __note__ when an exception is printed as the child of an EG but not when it's printed at the toplevel? Maybe Hypotheses could wrap exceptions in some kind of proxy exception?

I'm assuming that the note describes the leaf's membership in the EG rather than the leaf itself. If that's not the case I agree it doesn't make sense.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The note is a description of the leaf, which could equally well be used for a single bare exception (though print() suffices).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sounds like the requirement is to add a feature to BaseException to enrich exceptions in a way that shows up in their __str__.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I've created bpo45607 to track this request from Zac.

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(Buffer flush.)

Comment thread Lib/test/test_pickle.py Outdated
Comment thread Lib/test/test_exception_group.py Outdated
Comment on lines +79 to +81
self.assertIs(
type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Eh, why would Hypothesis need to subclass EG? While we were called on arbitrarily forbidding subclassing (as we did in an earlier PEP draft) I don't see why it would be useful. What is your use case? Maybe there's something we can add to the design to avoid the need? (For Trio I could imagine they'd want to provide some kind of backwards compatibility with Trio's current solution.)

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@iritkatriel, have you managed to convince another core dev to review exceptions.c yet?

Comment thread Lib/test/test_pickle.py Outdated
Comment thread Lib/test/test_exception_group.py Outdated
Comment on lines +79 to +81
self.assertIs(
type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Okay, I'll bite. What would you like to see supported upstream? A separate metadata item for each sub-exception? I'd say if you don't want to store that on the sub-exception in some dunder-ish attribute, you could indeed subclass EG to add that. There's an API so you can replicate this whenever an EG is being filtered. But I don't think we're going to support metadata that automatically gets replicated based on just this example -- in most cases I suspect you can just store an extra attribute on the individual exceptions.

Copy link
Copy Markdown
Member Author

@iritkatriel, have you managed to convince another core dev to review exceptions.c yet?

Not that I know of.

Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated
Comment thread Objects/exceptions.c Outdated

Copy link
Copy Markdown
Member Author

A PR for the traceback display code is here: iritkatriel#31

Once this PR is merged I will make a PR from that branch into main. But in the meantime it can be reviewed there, diffed against this PR.

Comment thread Lib/test/test_exception_group.py Outdated
Comment thread Lib/test/test_exception_group.py Outdated
Comment thread Lib/test/test_exception_group.py Outdated
Comment on lines +79 to +81
self.assertIs(
type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Anyway, if you want to override the way the traceback gets printed, subclassing EG isn't enough -- traceback-printing is (a) built into the C code, and (b) reimplemented in pure Python in traceback.py. To get something extra printed (e.g. err.__note__ if it exists) you will have to write your own traceback-printing code -- though there are various reusable bits and pieces in traceback.py.

Comment thread Lib/test/test_exception_group.py Outdated
Comment on lines +79 to +81
self.assertIs(
type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Anyway, if you want to override the way the traceback gets printed, subclassing EG isn't enough -- traceback-printing is (a) built into the C code, and (b) reimplemented in pure Python in traceback.py. To get something extra printed (e.g. err.__note__ if it exists) you will have to write your own traceback-printing code and invoke it at the right point -- though there are various reusable bits and pieces in traceback.py.

Comment thread Lib/test/test_exception_group.py Outdated
Comment thread Lib/test/test_exception_group.py Outdated
iritkatriel force-pushed the bpo-45292-ExceptionGroup branch from fee53c9 to dbd72d1 Compare October 22, 2021 20:50

Copy link
Copy Markdown
Member Author

(rebased)

iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 22, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit dbd72d1 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 22, 2021
iritkatriel merged commit f30ad65 into python:main Oct 22, 2021

Copy link
Copy Markdown
Member

“And so it begins…”

1st1 commented Oct 23, 2021

Copy link
Copy Markdown
Member

@iritkatriel Congrats Irit. This is very impressive work.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

10 participants


Back | FazBrowse Home | New Git URL