ENH: Fix exception causes in _iotools.py #15731

cool-RR · 2020-03-09T18:13:46Z

I recently went over Matplotlib and Pandas, fixing a small mistake in the way that Python 3's exception chaining is used. If you're interested, I can do it here too. I've done it on just one file right now.

The mistake is this: In some parts of the code, an exception is being caught and replaced with a more user-friendly error. In these cases the syntax raise new_error from old_error needs to be used.

Python 3's exception chaining means it shows not only the traceback of the current exception, but that of the original exception (and possibly more.) This is regardless of raise from. The usage of raise from tells Python to put a more accurate message between the tracebacks. Instead of this:

During handling of the above exception, another exception occurred:

You'll get this:

The above exception was the direct cause of the following exception:

The first is inaccurate, because it signifies a bug in the exception-handling code itself, which is a separate situation than wrapping an exception.

Let me know what you think!

seberg · 2020-03-10T22:58:15Z

Better error messages are a good project/improvement, although I am wondering if most of these should not rather use from None. The old exception is often just noise and thus very confusing, in my opinion.
I am not sure if all of these error messages are actually in tested code paths (or can even be created), although that is specifric to the _iotools.py file.

cool-RR · 2020-03-11T08:35:18Z

I'm strongly against using from None. When I'm debugging, I'm like a man who got lost in the desert and is about to die of thirst. Any possible insight into what happened is like an oasis, even if there are just a few drops of water there.

Also, some tools like Django and Sentry show you all the local variables for your stacktraces, which is a godsend. These often have important information that sheds light on what went wrong, and if you remove the traceback they'll be gone.

mattip · 2020-03-25T18:14:33Z

In a community discussion we would prefer to go through these one at a time and add from e as default, but some might want to use from None if the deeper error adds no more information.

WarrenWeckesser

Thanks for the pull request, @cool-RR. I added comments in-line. Based on my attempts to actually trigger these exceptions, I suggested that we not use from e in one case. In another case, I have a question about how to actually trigger the exception. It would be nice to be able to exercise these changes before committing them. For the remaining changes, using from e is probably OK.

WarrenWeckesser · 2020-03-31T07:12:48Z

numpy/lib/_iotools.py

                # dtype_or_func must be a function, then
                if not hasattr(dtype_or_func, '__call__'):
                    errmsg = ("The input argument `dtype` is neither a"
                              " function nor a dtype (got '%s' instead)")
-                    raise TypeError(errmsg % type(dtype_or_func))
+                    raise TypeError(errmsg % type(dtype_or_func)) from e


We shouldn't use from e here. The original exception doesn't provide useful information. For example,

In [29]: conv = StringConverter("foo") --------------------------------------------------------------------------- TypeError Traceback (most recent call last) ~/mc37np/lib/python3.7/site-packages/numpy-1.19.0.dev0+116a021-py3.7-macosx-10.9-x86_64.egg/numpy/lib/_iotools.py in __init__(self, dtype_or_func, default, missing_values, locked) 606 self.func = None --> 607 dtype = np.dtype(dtype_or_func) 608 except TypeError as e: TypeError: data type 'foo' not understood The above exception was the direct cause of the following exception: TypeError Traceback (most recent call last) <ipython-input-29-f4dc02a5945d> in <module> ----> 1 conv = StringConverter("foo") ~/mc37np/lib/python3.7/site-packages/numpy-1.19.0.dev0+116a021-py3.7-macosx-10.9-x86_64.egg/numpy/lib/_iotools.py in __init__(self, dtype_or_func, default, missing_values, locked) 611 errmsg = ("The input argument `dtype` is neither a" 612 " function nor a dtype (got '%s' instead)") --> 613 r 10000 aise TypeError(errmsg % type(dtype_or_func)) from e 614 # Set the function 615 self.func = dtype_or_func TypeError: The input argument `dtype` is neither a function nor a dtype (got '<class 'str'>' instead)

The first exception, TypeError: data type 'foo' not understood doesn't provide any information that is not also in the final exception message (TypeError: The input argument dtype is neither a function nor a dtype (got '<class 'str'>' instead), so it is noise. We should use from None here and not expose the first exception.

WarrenWeckesser · 2020-03-31T07:37:46Z

numpy/lib/_iotools.py

-                except OverflowError:
-                    raise ValueError
+                except OverflowError as e:
+                    raise ValueError from e


Do you have an example that triggers this exception? If this exception is raised, it is then caught a few lines down and raised again, so the end result of the use of from e in all these try-except statements is the user being given three chained exceptions. That seems pretty noisy, and probably not useful.

WarrenWeckesser · 2020-03-31T07:42:21Z

numpy/lib/_iotools.py

            if value.strip() in self.missing_values:
                if not self._status:
                    self._checked = False
                return self.default
-            raise ValueError("Cannot convert string '%s'" % value)
+            raise ValueError("Cannot convert string '%s'" % value) from e


It looks like the previous exception in the will give include the details of what went wrong. This one just says, in effect "fail!". So I guess this use of from e is OK.

(I'm starting to think that the way this code uses exceptions is awkward, and could use a redesign, but that will have to wait for another time.)

WarrenWeckesser · 2020-03-31T07:59:21Z

numpy/lib/_iotools.py

            # Raise an exception if we locked the converter...
            if self._locked:
                errmsg = "Converter is locked and cannot be upgraded"
-                raise ConverterLockError(errmsg)
+                raise ConverterLockError(errmsg) from e


The exception chain is a bit noisy, but it looks like the early exceptions have useful information, so I guess this use of from e is OK:

In [121]: conv = StringConverter(int, locked=True) In [122]: conv.upgrade('0.') --------------------------------------------------------------------------- ValueError Traceback (most recent call last) ~/mc37np/lib/python3.7/site-packages/numpy-1.19.0.dev0+116a021-py3.7-macosx-10.9-x86_64.egg/numpy/lib/_iotools.py in _strict_call(self, value) 687 # We check if we can convert the value using the current function --> 688 new_value = self.func(value) 689 ValueError: invalid literal for int() with base 10: '0.' The above exception was the direct cause of the following exception: ValueError Traceback (most recent call last) ~/mc37np/lib/python3.7/site-packages/numpy-1.19.0.dev0+116a021-py3.7-macosx-10.9-x86_64.egg/numpy/lib/_iotools.py in upgrade(self, value) 736 try: --> 737 return self._strict_call(value) 738 except ValueError as e: ~/mc37np/lib/python3.7/site-packages/numpy-1.19.0.dev0+116a021-py3.7-macosx-10.9-x86_64.egg/numpy/lib/_iotools.py in _strict_call(self, value) 706 return self.default --> 707 raise ValueError("Cannot convert string '%s'" % value) from e 708 # ValueError: Cannot convert string '0.' The above exception was the direct cause of the following exception: ConverterLockError Traceback (most recent call last) <ipython-input-122-b82e0b39004c> in <module> ----> 1 conv.upgrade('0.') ~/mc37np/lib/python3.7/site-packages/numpy-1.19.0.dev0+116a021-py3.7-macosx-10.9-x86_64.egg/numpy/lib/_iotools.py in upgrade(self, value) 740 if self._locked: 741 errmsg = "Converter is locked and cannot be upgraded" --> 742 raise ConverterLockError(errmsg) from e 743 _statusmax = len(self._mapper) 744 # Complains if we try to upgrade by the maximum ConverterLockError: Converter is locked and cannot be upgraded

I don't think this is correct.

This makes the error message The above exception was the direct cause of the following exception:.

However, that's not what's happening here. What's happening here is that something else went wrong while we tried to recover (by upgrading the data type). Today, the message is

During handling of the above exception, another exception occurred:

This is a more accurate message. So this line was better unchanged.

WarrenWeckesser · 2020-03-31T08:12:52Z

numpy/lib/_iotools.py

            _statusmax = len(self._mapper)
            # Complains if we try to upgrade by the maximum
            _status = self._status
            if _status == _statusmax:
                errmsg = "Could not find a valid conversion function"
-                raise ConverterError(errmsg)
+                raise ConverterError(errmsg) from e


This looks like it is in the same situation as the previous one: it makes a noisy exception, but there might be useful info. in there, so OK.

WarrenWeckesser · 2020-03-31T08:13:37Z

numpy/lib/_iotools.py

            # Raise an exception if we locked the converter...
            if self._locked:
                errmsg = "Converter is locked and cannot be upgraded"
-                raise ConverterLockError(errmsg)
+                raise ConverterLockError(errmsg) from e


Ditto, so OK.

WarrenWeckesser · 2020-03-31T08:13:51Z

numpy/lib/_iotools.py

            _statusmax = len(self._mapper)
            # Complains if we try to upgrade by the maximum
            _status = self._status
            if _status == _statusmax:
                raise ConverterError(
                    "Could not find a valid conversion function"
-                    )
+                    ) from e


Ditto, so OK.

cool-RR · 2020-03-31T18:24:57Z

@WarrenWeckesser Thanks for your review. That was very thorough.

I disagree with the decision to use from None anywhere, and I don't want to be the reason that a developer didn't get a traceback. So here's what I did: I amended my commit to only include the 5 cases we agree about. The other cases could be done in a separate PR by whoever's interested.

eric-wieser

I don't think any of the ConversionError cases make sense to chain exc 685C eption __cause__s, these look like __context__ chains to me, which is what we already had.

eric-wieser · 2020-03-31T18:29:56Z

numpy/lib/_iotools.py

            # Raise an exception if we locked the converter...
            if self._locked:
                errmsg = "Converter is locked and cannot be upgraded"
-                raise ConverterLockError(errmsg)
+                raise ConverterLockError(errmsg) from e


I don't think this is correct.

This makes the error message The above exception was the direct cause of the following exception:.

However, that's not what's happening here. What's happening here is that something else went wrong while we tried to recover (by upgrading the data type). Today, the message is

During handling of the above exception, another exception occurred:

This is a more accurate message. So this line was better unchanged.

eric-wieser · 2020-03-31T18:31:02Z

numpy/lib/_iotools.py

            _statusmax = len(self._mapper)
            # Complains if we try to upgrade by the maximum
            _status = self._status
            if _status == _statusmax:
                errmsg = "Could not find a valid conversion function"
-                raise ConverterError(errmsg)
+                raise ConverterError(errmsg) from e


eric-wieser · 2020-03-31T18:31:08Z

numpy/lib/_iotools.py

            # Raise an exception if we locked the converter...
            if self._locked:
                errmsg = "Converter is locked and cannot be upgraded"
-                raise ConverterLockError(errmsg)
+                raise ConverterLockError(errmsg) from e


eric-wieser · 2020-03-31T18:31:12Z

numpy/lib/_iotools.py

            _statusmax = len(self._mapper)
            # Complains if we try to upgrade by the maximum
            _status = self._status
            if _status == _statusmax:
                raise ConverterError(
                    "Could not find a valid conversion function"
-                    )
+                    ) from e


eric-wieser · 2020-04-03T09:19:23Z

Apologies for the merge conflicts. Note that to merge keeping the semantics of this patch (that I'm arguing against above) you'd need to write:

        except ValueError as e:
            try:
                self._do_upgrade()
            except ConversionError as e2:
                raise e2 from e1  # claim that e2 was _caused_ by e1

Again though, I'd recommend you not do this.

eric-wieser · 2020-04-15T14:04:31Z

@cool-RR, I've found a bunch of places elsewhere in numpy where changing to use raise from would definitely be valuable. I'd recommend you restrict yourself to clear-cut cases like:

except TypeError:
        raise ValueError(...)

etc

cool-RR · 2020-04-15T14:23:57Z

@eric-wieser Hmm, that's not enjoyable enough for me to do, so I'll leave that to whoever's interested.

eric-wieser · 2020-04-16T08:21:42Z

Opened #15986 to track that, thanks for bringing it to our attention @cool-RR even though this PR didn't get merged.

cool-RR marked this pull request as ready for review March 9, 2020 18:21

seberg changed the title ~~MAINT: Fix exception causes in _iotools.py~~ ENH: Fix exception causes in _iotools.py Mar 10, 2020

seberg added 01 - Enhancement component: numpy.lib labels Mar 10, 2020

mattip added the triage review Issue/PR to be discussed at the next triage meeting label Mar 18, 2020

mattip added triaged Issue/PR that was discussed in a triage meeting and removed triage review Issue/PR to be discussed at the next triage meeting labels Mar 25, 2020

WarrenWeckesser requested changes Mar 31, 2020

View reviewed changes

MAINT: Fix exception causes in _iotools.py

536afeb

cool-RR force-pushed the 2020-03-09-raise-from branch from 116a021 to 536afeb Compare March 31, 2020 18:21

eric-wieser requested changes Mar 31, 2020

View reviewed changes

eric-wieser mentioned this pull request Mar 31, 2020

MAINT: Remove duplicated code in iotools.py #15883

Merged

cool-RR closed this Apr 15, 2020

eric-wieser mentioned this pull request Apr 15, 2020

Chain exceptions where appropriate #15986

Closed

rossbar mentioned this pull request May 13, 2020

MAINT: Chain exceptions in npyio.py #16218

Closed

cool-RR mentioned this pull request Feb 20, 2021

Fix exception cause in video.py google-deepmind/acme#99

Merged

cool-RR mentioned this pull request Mar 11, 2021

Use exception chaining when re-raising numpy exceptions within BoundedArray.__init__ google-deepmind/dm_env#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

ENH: Fix exception causes in _iotools.py #15731

ENH: Fix exception causes in _iotools.py #15731

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ENH: Fix exception causes in _iotools.py #15731

ENH: Fix exception causes in _iotools.py #15731

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!