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

Fix exception chaining #3679

Closed
akihironitta opened this issue Sep 27, 2020 · 5 comments · Fixed by #3750
Closed

Fix exception chaining #3679

akihironitta opened this issue Sep 27, 2020 · 5 comments · Fixed by #3750
Assignees
Labels
feature Is an improvement or enhancement help wanted Open to be worked on

Comments

@akihironitta
Copy link
Contributor

I recently went over PyTorch and Detectron2, suggesting a fix in the way that Python 3's exception chaining is used.

As described in detail in this article, exception chaining (PEP 3134) can be used to make exceptions more user-friendly, and in that case, the syntax raise new_exception from old_exception needs to be used.

When raise .. from .. is used, there will be a line saying The above exception was the direct cause of the following exception between tracebacks. However, when implicitly chaining exceptions (meaning when raise .. from .. is not used), the message will be During handling of the above exception, another exception occurred which can confuse users.

Specifically, the following should be used in order to chain exceptions:

try:
    something_which_raises_OldError
except OldError as e:
    raise NewError("A more user-friendly exception message.") from e 

instead of:

try:
    something_which_raises_OldError
except OldError:
    raise NewError("A more user-friendly exception message.")

One example which needs to be fixed is:
https://github.com/PyTorchLightning/pytorch-lightning/blob/3d76f604bdb97d66dd1248540ea088fb18c06a03/pytorch_lightning/utilities/parsing.py#L159-L162

If this suggestion sounds good and reasonable, I'd be happy to create a PR! Let me know your thoughts on this!

@akihironitta akihironitta added bug Something isn't working help wanted Open to be worked on labels Sep 27, 2020
@github-actions
Copy link
Contributor

Hi! thanks for your contribution!, great first issue!

@awaelchli
Copy link
Contributor

oh! I just learned something new about Python exceptions. Awesome!
So if I understand correctly, this change will make the exceptions more user friendly, pointing to where it originated from.
Do we keep the bugfix label? It sounds more like an enhancement

Related: #2266

@edenlightning edenlightning added feature Is an improvement or enhancement Hacktoberfest and removed bug Something isn't working labels Sep 28, 2020
@akihironitta
Copy link
Contributor Author

Thanks @edenlightning for relabelling the issue! I didn't know Hacktoberfest at all!

@awaelchli Thank you for your quick response!
Yes, you're right. Since this issue is not critical, can I work on this in October as a PR for Hacktoberfest?

@rohitgr7
Copy link
Contributor

@akihironitta assigned you to this issue. Yeah, you can do this in October.

@akihironitta
Copy link
Contributor Author

@awaelchli @rohitgr7 I just made #3750 ready for review! Can you have a look at the changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement help wanted Open to be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants