Skip to content

Chain exceptions where appropriate #15986

Closed
Closed
Chain exceptions where appropriate#15986
#17334 (+2)

Description

@eric-wieser
eric-wieser
opened on Apr 15, 2020

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" % name) from None
    In that example all the information from the KeyError is already in the message of the new error, so there is no point chaining

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!

Activity

  1. eric-wieser
    added
    good first issue
    on Apr 15, 2020

  2. seberg commented on Apr 17, 2020

    @seberg
    seberg
    on Apr 17, 2020
    Member

    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.

  3. keremh commented on Apr 18, 2020

    @keremh
    keremh
    on Apr 18, 2020
    Contributor

    Hi @seberg
    I determined the part to be changed, can i work on this issue too?

  4. seberg
    added a commit that references this issue on May 14, 2020

    MAINT: Chain exceptions and use NameError in np.bmat (#16215)

    a813a5f

  5. ayao451 commented on Jun 30, 2020

    @ayao451
    ayao451
    on Jun 30, 2020

    Can I work on this issue? It is my first issue and I want to give it a try!

  6. seberg
    added a commit that references this issue on Jun 30, 2020

    MAINT: Chain exceptions in npyio.py (gh16121)

    f38073b

  7. rossbar commented on Jul 1, 2020

    @rossbar
    rossbar
    on Jul 1, 2020
    Contributor

    @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).

  8. rossbar commented on Jul 12, 2020

    @rossbar
    rossbar
    on Jul 12, 2020
    Contributor

    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.

  9. nkleinbaer commented on Jul 17, 2020

    @nkleinbaer
    nkleinbaer
    on Jul 17, 2020

    @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.

  10. rossbar commented on Jul 18, 2020

    @rossbar
    rossbar
    on Jul 18, 2020
    Contributor

    @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.

  11. nkleinbaer commented on Jul 18, 2020

    @nkleinbaer
    nkleinbaer
    on Jul 18, 2020

    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

  12. nkleinbaer commented on Jul 18, 2020

    @nkleinbaer
    nkleinbaer
    on 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:

  13. rossbar commented on Jul 19, 2020

    @rossbar
    rossbar
    on Jul 19, 2020
    Contributor

    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!

  14. nkleinbaer commented on Jul 20, 2020

    @nkleinbaer
    nkleinbaer
    on 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.

  15. 17 remaining items

  16. labbunny
    added a commit that references this issue on Apr 21, 2021

    MAINT: Chain exceptions in ctypeslib.py

    b602df1

  17. labbunny commented on Apr 21, 2021

    @labbunny
    labbunny
    on Apr 21, 2021

    Hello everyone. I'm new to open-source and I want to start doing some work.
    Can I jump in and contribute to the solution of this issue? As you see I already tried to commit some code but it failed on few tests. Can somebody explain what went wrong? @eric-wieser perhaps?

  19. mattip commented on Jun 9, 2021

    @mattip
    mattip
    on Jun 9, 2021
    Member

    ... with that please realize that this is a low priority issue, so PRs may take a while to get reviewed and merged. And as I commented above:

    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?

  20. adithya-1 commented on Aug 17, 2021

    @adithya-1
    adithya-1
    on Aug 17, 2021

    Hey @eric-wieser, I am new to open source and would like to contribute to this project. Can I work on this issue?

  21. adithya-1
    added 2 commits that reference this issue on Aug 17, 2021

    Fixed issue numpy#15986 in runtests.py line 250 in db1a343

    56c333c

    Fixed issue numpy#15986 in benchmarks/benchmarls/bench_ufunc.py in li…

    66f182c
  22. vincenzosantopietro
    added 2 commits that reference this issue on Aug 18, 2021

    refactor exceptions handling due to issue numpy#15986

    006003b

    refactor exceptions handling due to issue numpy#15986

    82dcb81
  23. debarun1234
    added a commit that references this issue on Aug 19, 2021

    Import directory debugged and readdressed

    b0ccfe7

  24. mattip commented on Aug 19, 2021

    @mattip
    mattip
    on Aug 19, 2021
    Member

    I think we have gotten about as much mileage as possible from this issue and would like to close it. @eric-wieser what do you think?

  25. eric-wieser commented on Aug 19, 2021

    @eric-wieser
    eric-wieser
    on Aug 19, 2021
    MemberAuthor

    I think I agree @mattip, feel free to close. We can still review any remaining open PRs.

    I'm curious if any contributors who contributed for the first time to numpy thanks to this issue ended up making further contributions later, or if they've all just been drive-by one-off contributions.

  26. mattip
    closed this as completedon Aug 19, 2021
  27. mattip
    removed
    good first issue
    on Aug 19, 2021

  28. mattip commented on Aug 19, 2021

    @mattip
    mattip
    on Aug 19, 2021
    Member

    Maybe they are now contributing elsewhere.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions