Skip to content

/ numpy

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

Jump to bottom

Chain exceptions where appropriate #15986

Open
eric-wieser opened this issue Apr 15, 2020 · 16 comments
Open

Chain exceptions where appropriate #15986

eric-wieser opened this issue Apr 15, 2020 · 16 comments
Labels
good first issue

Comments

@eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Apr 15, 2020 •
edited by rossbar

Picking up from #15731, there are many places in numpy where we do something like:

try:
    something_which_throws_typeError
except TypeError:
    raise ValueError("a clearer explanation of what went wrong")

It would produce a marginally clearer error if we could change these to use traceback chaining. Our two options are either:

  1. The inner error provides valuable information that is not present in the outer error. We should use from e. 
    try:
    something_which_throws_typeError
except TypeError as e:
    raise ValueError("a clearer explanation of what went wrong") from e
  2. The inner error provides no valuable information that is not present in the outer error. We should use from None: 
    try:
    some_dict[name]
except KeyError:
    raise ValueError("The name %s is not supported") from None

An example of such a change would be this line of this otherwise-discarded patch.

For now, I would recommend we only apply this to cases where the exception is thrown unconditionally, as other cases can be more nuanced.

Please see this list for potential places to contribute. Special thanks to @nkleinbaer for compiling the list!
@eric-wieser eric-wieser mentioned this issue Apr 16, 2020
Closed
@seberg
Copy link
Member

@seberg seberg commented Apr 17, 2020

Sure @JanukanS, thanks. We don't usually assign issues. So you can search for these type of calls and simply open a PR with suggested changes linking this issue.
@keremh
Copy link
Contributor

@keremh keremh commented Apr 18, 2020

Hi @seberg
I determined the part to be changed, can i work on this issue too?
This was referenced Apr 22, 2020
Merged
Merged
This was referenced Apr 24, 2020
Closed
Merged
@keremh keremh mentioned this issue Apr 24, 2020
Merged
@ChrisAHolland ChrisAHolland mentioned this issue Apr 24, 2020
Merged
@keremh keremh mentioned this issue Apr 30, 2020
Merged
@bartosz-grabowski bartosz-grabowski mentioned this issue May 12, 2020
Merged
@smitgajjar smitgajjar mentioned this issue May 12, 2020
Closed
seberg pushed a commit that referenced this issue May 14, 2020
@bartosz-grabowski @eric-wieser @rossbar
MAINT: Chain exceptions and use NameError in np.bmat (#16215)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a813a5f 
This solution is related to the issue #15986. I also made a change to the newer string formatting.
Uses NameError, which prints nicer, and is actually the more correct error type.

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
@lkdmttg7 lkdmttg7 mentioned this issue May 15, 2020
Merged
@zaak71 zaak71 mentioned this issue May 28, 2020
Merged
anirudh2290 added a commit to anirudh2290/numpy that referenced this issue May 28, 2020
@bartosz-grabowski @eric-wieser
@rossbar @anirudh2290
MAINT: Chain exceptions and use NameError in np.bmat (numpy#16215)
5959c53 
This solution is related to the issue numpy#15986. I also made a change to the newer string formatting.
Uses NameError, which prints nicer, and is actually the more correct error type.

Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
This was referenced Jun 23, 2020
Merged
Merged
@ayao451
Copy link

@ayao451 ayao451 commented Jun 30, 2020

Can I work on this issue? It is my first issue and I want to give it a try!
seberg pushed a commit that referenced this issue Jun 30, 2020
@keremh @rossbar
MAINT: Chain exceptions in npyio.py (gh16121)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f38073b 
this solution is related to the following issue #15986


Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
@rossbar
Copy link
Contributor

@rossbar rossbar commented Jul 1, 2020

@ayao451 please feel free. A good place to start might be by checking the PRs linked in this issue: exception chaining has been added in quite a few places, and a few other instances have been identified as better off unchanged (e.g. nested exceptions in the test suite).
@abhinav-thimma abhinav-thimma mentioned this issue Jul 4, 2020
Merged
@sarthakchaudhary13 sarthakchaudhary13 mentioned this issue Jul 6, 2020
Merged
@rossbar
Copy link
Contributor

@rossbar rossbar commented Jul 12, 2020

A lot of these instances have been fixed (thanks to all the contributors for this!) It would be nice to have a "definition of done" for this issue if anyone wants to do a little forensics to see what's already been tackled, what's been reviewed and determined better left as-is, and what still remains to be done.
@nkleinbaer
Copy link

@nkleinbaer nkleinbaer commented Jul 17, 2020 •
edited

@rossbar I think I can help the 'forensics' you describe above. My thought was to create a regex expression to search for instances of nested exceptions in the codebase (excluding tests). Something like (\s*)except .*\n(\1\s+.*\n)*\1\s+raise to capture the indentation level of the original except statement, then searching for raise within that indent block.

Then I could cross-reference that list with those instances that have already been addressed in the PRs above, and make a table of what's been addressed and what still needs to be reviewed.

Does that seem like a reasonable approach? I'm a complete beginner, found this issue through the 'good first issue' tag, so just want to ensure I'm not going down a rabbit hole or creating additional confusion.
@rossbar
Copy link
Contributor

@rossbar rossbar commented Jul 18, 2020

@nkleinbaer that approach sounds reasonable, and I think tabulating the results in this issue would be very useful. Even if the cross-referencing proves to be difficult to automate, having a list of all of the potential instances for exception chaining would help finish off this issue. If you are interested in taking this on I think that'd be great! Feel free ping with any questions/comments.
@nkleinbaer
Copy link

@nkleinbaer nkleinbaer commented Jul 18, 2020 •
edited

OK, to start off here is a table of the files that have been touched in reference to this issue, the number of instances reviewed, the action taken, and associated PRs. I've used the nomenclature "Implemented exception chaining" to indicate something of the form except SomeError as e: raise SomeOtherError from e and "Explicitly silenced exception chaining" to indicate the form except SomeError as e: raise SomeOtherError from None. Please correct if the this can be phrased better.

In some cases my regex found additional instances of 'nested' exceptions in these files that I do not see included in the PRs. It may be the case that they were intentionally left as is, but because I cannot find any documented 'review' of them I have included them for completeness.

File Action Taken Associated PR(s) Comments
ctypeslib.py 1 instance each: explicitly silenced exception chaining and implemented exception chaining #16418 1 additional instances may need review

numpy/numpy/ctypeslib.py

Line 151 in 6ef5ec3

except OSError:
core/_dtype.py 1 instance(s): explicitly silenced exception chaining #16418
core/_internal.py 2 instance(s): explicitly silenced exception chaining #16747
core/fromnumeric.py 1 instance(s): implemented exception chaining #16418
core/memap.py 1 instance(s): explicitly silenced exception chaining #16067
core/records.py 2 instance(s): implemented exception chaining #16418 1 additional instance may need review

numpy/numpy/core/records.py

Line 495 in 6ef5ec3

except Exception:
core/code_generators/generate_umath.py 1 instance(s): implemented exception chaining #16254
distutils/command/build_clib.py 1 instance(s): implemented exception chaining #16032
distutils/command/build_ext.py 1 instance(s): implemented exception chaining #16042
lib/_datasource.py 1 instance(s): Completely removed nested exception -> left inner exception by itself #16761
lib/hisograms.py 1 instance(s): implemented exception chaining #16064 1 additional instance added in #16129 - seems to implement exception chaining correctly but may need review
lib/index_tricks.py 1 instance(s): implemented exception chaining #16064 1 additional instance needs review

numpy/numpy/lib/index_tricks.py

Line 378 in 6ef5ec3

except (ValueError, TypeError):
lib/npyio.py 2 instance(s): implemented exception chaining #16121 A bit confused on what happened here. My regex finds 8 instances total - I think all have been reviewed and some were determined to be better left as is in #16218, may be worth a second look
lib/shape_base.py 1 instance(s): explicitly silenced exception chaining #16064 2 additional instances may need review

numpy/numpy/lib/shape_base.py

Line 774 in 6ef5ec3

except TypeError:

numpy/numpy/lib/shape_base.py

Line 868 in 6ef5ec3

except TypeError:
lib/ufunclike.py 2 instance(s): implemented exception chaining #16064
linalg/linalg.py 2 instance(s): implemented exception chaining #16061
ma/core.py 1 instance(s): explicitly silenced exception chaining #16067 3 additional instances may need review

numpy/numpy/ma/core.py

Line 446 in 6ef5ec3

except ValueError:

numpy/numpy/ma/core.py

Line 463 in 6ef5ec3

except (OverflowError, ValueError):

numpy/numpy/ma/core.py

Line 3794 in 6ef5ec3

except IndexError:
matrixlib/defmatrix.py 1 instance(s): explicitly silenced exception chaining #16215
polynomial/polyutils.py 2 instance(s): implemented exception chaining #16061
@nkleinbaer
Copy link

@nkleinbaer nkleinbaer commented Jul 18, 2020

And here is a table of additional files/instances found by my regex that have NOT yet been reviewed in a PR related to this issue. Some of these look like they are already handling exception chaining properly (example einsumfunc.py), but again included here for completeness because I see no documentation of an explicit review.

File Instances found by Regex Instances
init.py 2

numpy/numpy/__init__.py

Line 127 in 6ef5ec3

except ImportError:

numpy/numpy/__init__.py

Line 292 in 6ef5ec3

except AssertionError:
compat/py3k.py 1

numpy/numpy/compat/py3k.py

Line 173 in 6ef5ec3

except AttributeError:
core/_type_aliases.py 1

numpy/numpy/core/_type_aliases.py

Line 47 in 6ef5ec3

except StopIteration:
core/einsumfunc.py 2

numpy/numpy/core/einsumfunc.py

Line 583 in 6ef5ec3

except TypeError as e:

numpy/numpy/core/einsumfunc.py

Line 598 in 6ef5ec3

except TypeError as e:
core/code_generators/genapi.py 1

numpy/numpy/core/code_generators/genapi.py

Line 283 in 6ef5ec3

except Exception:
distutils/conv_template.py 4

numpy/numpy/distutils/conv_template.py

Line 219 in 6ef5ec3

except KeyError:

numpy/numpy/distutils/conv_template.py

Line 239 in 6ef5ec3

except ValueError as e:

numpy/numpy/distutils/conv_template.py

Line 287 in 6ef5ec3

except ValueError as e:

numpy/numpy/distutils/conv_template.py

Line 324 in 6ef5ec3

except ValueError as e:
distutils/mingw32ccompiler.py 1

numpy/numpy/distutils/mingw32ccompiler.py

Line 563 in 6ef5ec3

except KeyError:
distutils/unixccompiler.py 2

numpy/numpy/distutils/unixccompiler.py

Line 52 in 6ef5ec3

except DistutilsExecError as e:

numpy/numpy/distutils/unixccompiler.py

Line 124 in 6ef5ec3

except DistutilsExecError as e:
distutils/command/config.py 3

numpy/numpy/distutils/command/config.py

Line 53 in 6ef5ec3

except IOError as e:

numpy/numpy/distutils/command/config.py

Line 96 in 6ef5ec3

except (DistutilsExecError, CompileError) as e:

numpy/numpy/distutils/command/config.py

Line 456 in 6ef5ec3

except Exception:
distutils/fcompiler/init.py 2

numpy/numpy/distutils/fcompiler/__init__.py

Line 613 in 6ef5ec3

except DistutilsExecError as e:

numpy/numpy/distutils/fcompiler/__init__.py

Line 681 in 6ef5ec3

except DistutilsExecError as e:
distutils/fcompiler/compaq.py 3

numpy/numpy/distutils/fcompiler/compaq.py

Line 82 in 6ef5ec3

except AttributeError as e:

numpy/numpy/distutils/fcompiler/compaq.py

Line 87 in 6ef5ec3

except IOError as e:

numpy/numpy/distutils/fcompiler/compaq.py

Line 91 in 6ef5ec3

except ValueError as e:
distutils/fcompiler/environment.py 1

numpy/numpy/distutils/fcompiler/environment.py

Line 35 in 6ef5ec3

except KeyError:
fft/_pocketfft.py 1

numpy/numpy/fft/_pocketfft.py

Line 113 in 6ef5ec3

except KeyError:
lib/arraypad.py 1

numpy/numpy/lib/arraypad.py

Line 785 in 6ef5ec3

raise ValueError("unsupported keyword arguments for mode '{}': {}"
lib/arraysetops.py 2

numpy/numpy/lib/arraysetops.py

Line 279 in 6ef5ec3

except np.AxisError:

numpy/numpy/lib/arraysetops.py

Line 303 in 6ef5ec3

except TypeError:
lib/format.py 4

numpy/numpy/lib/format.py

Line 379 in 6ef5ec3

except struct.error:

numpy/numpy/lib/format.py

Line 596 in 6ef5ec3

except SyntaxError as e:

numpy/numpy/lib/format.py

Line 617 in 6ef5ec3

except TypeError:

numpy/numpy/lib/format.py

Line 745 in 6ef5ec3

except UnicodeError as err:
linalg/lapack_lite/make_lite.py 1

numpy/numpy/linalg/lapack_lite/make_lite.py

Line 264 in 6ef5ec3

except subprocess.CalledProcessError:
ma/mrecords.py 3

numpy/numpy/ma/mrecords.py

Line 201 in 6ef5ec3

except (TypeError, KeyError):

numpy/numpy/ma/mrecords.py

Line 254 in 6ef5ec3

except Exception:

numpy/numpy/ma/mrecords.py

Line 276 in 6ef5ec3

except (TypeError, KeyError):
ma/timer_comparison.py 1

numpy/numpy/ma/timer_comparison.py

Line 103 in 6ef5ec3

except ValueError:
polynomial/_polybase.py 2

numpy/numpy/polynomial/_polybase.py

Line 550 in 6ef5ec3

except ZeroDivisionError as e:

numpy/numpy/polynomial/_polybase.py

Line 608 in 6ef5ec3

except ZeroDivisionError as e:
@Ashutosh619-sudo Ashutosh619-sudo mentioned this issue Jul 18, 2020
Merged
@rossbar
Copy link
Contributor

@rossbar rossbar commented Jul 19, 2020

Thanks for taking the time to put this report together @nkleinbaer , this is a really nice review of the current state of things!

I think the most effective way to present the information to help close this issue would be if we could take all of the instances that you found in your regex search, and list them using a list of checkboxes, e.g.

 - [x] https://github.com/numpy/numpy/blob/6ef5ec39cdfaf77aa4600ec2e3bf9f679a4fd527/numpy/ctypeslib.py#L151
 - [ ] https://github.com/numpy/numpy/blob/6ef5ec39cdfaf77aa4600ec2e3bf9f679a4fd527/numpy/__init__.py#L127
 ...

where instances that have been changed or reviewed get a check, and instances that haven't been addressed yet are left un-checked. That way, we have a running list of the instances where a decision has been made (whether to chain exceptions, use from None, or leave as-is) so that contributors can focus on the remaining ones.

This should give us a list like:

which I believe GitHub will summarize in a progress bar for the issue.

If it is easy for you to re-organize your results in this way and you don't mind the extra effort, it'd be great if you could add a list formatted as described in a comment below, then I will take the list and move it up to the top comment so it is more visible. You've already done plenty though, so if you don't have the bandwidth that's totally fine - just let me know and I'll organize the list using your results. Thanks for all you've done so far!
@nkleinbaer
Copy link

@nkleinbaer nkleinbaer commented Jul 20, 2020

Sure, I can do that! I actually wanted to have check boxes in the tables above, but couldn't figure out how to do the formatting (not sure if possible at all). Mega-checklist to follow.
@nkleinbaer
Copy link

@nkleinbaer nkleinbaer commented Jul 20, 2020 •
edited by mattip

@rossbar
Copy link
Contributor

@rossbar rossbar commented Jul 21, 2020

@nkleinbaer I wasn't able to add the full list to the top comment (trying to do so gave github fits), so I've linked to it instead. Many thanks for compiling it!
@nomanarshad94 nomanarshad94 mentioned this issue Jul 27, 2020
Merged
@nomanarshad94
Copy link
Contributor

@nomanarshad94 nomanarshad94 commented Jul 27, 2020

Hi, picked this file numpy/numpy/lib/arraypad.py to update exceptions and created a pull request.
@eric-wieser eric-wieser reopened this Jul 28, 2020
@nomanarshad94 nomanarshad94 mentioned this issue Jul 28, 2020
Merged
@rossbar rossbar reopened this Aug 4, 2020
@applemonkey496 applemonkey496 mentioned this issue Aug 19, 2020
Open
@applemonkey496
Copy link

@applemonkey496 applemonkey496 commented Aug 19, 2020

I found more: #17108.
@raghavkhanna18 raghavkhanna18 mentioned this issue Aug 21, 2020
Merged
@karan-dhir karan-dhir mentioned this issue Aug 21, 2020
Merged
@EthanCJ-git EthanCJ-git mentioned this issue Sep 3, 2020
Merged
seberg pushed a commit that referenced this issue Sep 3, 2020
@EthanCJ-git
MAINT: added exception chaining in shape_base.py (gh-17240)
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
9127903 
This edit is relation to issue gh-15986.
Chained exception in shape_base.py
This was referenced Sep 16, 2020
Closed
Merged
@eric-wieser eric-wieser reopened this Sep 16, 2020
This was referenced Oct 1, 2020
Closed
Merged
@Ashutosh619-sudo Ashutosh619-sudo mentioned this issue Oct 3, 2020
Merged
This was referenced Oct 3, 2020
Closed
Merged
@valdaarhun valdaarhun mentioned this issue Oct 4, 2020
Merged
@zer0link zer0link mentioned this issue Oct 6, 2020
Merged
@mattip
Copy link
Member

@mattip mattip commented Oct 7, 2020

In the recent developer meeting we discussed this issue, which is pretty low on the benefit/cost ratio. We would like to suggest that contributors show the output from the error. This will require you think about the error: is the code path actually hit? Does it make sense to chain the exception?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

update numpy/lib/arraypad.py with appropriate chain exception
MAINT: Chain exception in ``distutils/fcompiler/environment.py``.
MAINT: Chaining exceptions in npyio.py
MAINT: Fix chain exception in npyio.py
9 participants
@seberg @eric-wieser @mattip @rossbar @keremh @ayao451 @nomanarshad94 @nkleinbaer @applemonkey496
You can’t perform that action at this time.