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

Use __notes__ for exceptions when reporting keys during IO #868

Closed
ivirshup opened this issue Dec 5, 2022 · 3 comments · Fixed by #1055
Closed

Use __notes__ for exceptions when reporting keys during IO #868

ivirshup opened this issue Dec 5, 2022 · 3 comments · Fixed by #1055

Comments

@ivirshup
Copy link
Member

ivirshup commented Dec 5, 2022

Background

Currently, anndata reports which element from a particular store led to an error by wrapping the exception in an AnnDataReadError or modifying the text of the exception for read errors.

These are both kinda weird, but PEP-678 has the solution: notes for exceptions. Our use case is specifically described under the alternative raise Wrapper(explanation) from err:

An alternative pattern is to use exception chaining: by raising a ‘wrapper’ exception containing the context or explanation from the current exception, we avoid the separation challenges from print(). However, this has two key problems.

First, it changes the type of the exception, which is often a breaking change for downstream code. We consider always raising a Wrapper exception unacceptably inelegant ...

Second, exception chaining reports several lines of additional detail, which are distracting for experienced users and can be very confusing for beginners ...

BaseException now gets a __notes__ attribute and an add_note() method. __notes__ is a list of notes which will be reported when the exception gets formatted.

Idea

So, instead of using a hacky solution for read errors and an unacceptably inelegant one for write errors lets try to use this built in solution. E.g. we add information about the key to each error as a note.

Implementation

While this was only added in python 3.11, it is available in older versions through the exceptiongroup backport package. For example (using the helper function suggested at agronholm/exceptiongroup#31):

import exceptiongroup
import sys

def add_note(err: BaseException, msg: str) -> None:
    if sys.version_info < (3, 11):
        err.__notes__ = getattr(err, "__notes__", []) + [msg]
    else:
        err.add_note(msg)

def foo():
    raise AssertionError("foo is a bad function")

try:
    foo()
except AssertionError as e:
    add_note(e, "yeah, foo is awful")
    raise e
Traceback (most recent call last):
  File "<stdin>", line 5, in <module>
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 2, in foo
AssertionError: foo is a bad function
yeah, foo is awful

However, this currently does not work in ipython:

So these notes currently wouldn't be visible to most of our users. That means we probably can't use this solution until the IPython problem is solved.

Plan

  • Wait until this is fixed in IPython
  • Deprecate AnnDataReadError
  • Next release use notes instead

Alternative AnnDataWriteError, AnnDataReadError

The alternative would be to define our own error types where this information is included. Really the goal here is just to add a little information, and these add a ton of traceback along with that little bit of information. The new notes mechanism is very similar to how we handle writing errors currently.

@ivirshup
Copy link
Member Author

ivirshup commented Jun 21, 2023

I believe the upstream IPython error was fixed in

as I can now run the above example.

Implementation question, can we express a version restriction on ipython>=8.14 without making it a dependency?

@ivirshup ivirshup added this to the 0.10.0 milestone Jun 21, 2023
@ivirshup ivirshup removed the upstream label Jun 21, 2023
@flying-sheep
Copy link
Member

can we express a version restriction on ipython>=8.14 without making it a dependency?

I don’t think so, and I think supporting that would make dependency resolution turing complete.

@Zac-HD
Copy link

Zac-HD commented Aug 17, 2023

Implementation question, can we express a version restriction on ipython>=8.14 without making it a dependency?

I don't think you can express this at install-time, but you can have a runtime warning if sys.modules["ipython"]. version_info[:2] < (8, 14) 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants