Improved Exception handling#2357
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2357 +/- ##
===========================================
- Coverage 89.92% 89.76% -0.16%
===========================================
Files 175 159 -16
Lines 21912 19954 -1958
Branches 2874 2875 +1
===========================================
- Hits 19704 17912 -1792
+ Misses 1614 1442 -172
- Partials 594 600 +6
Continue to review full report at Codecov.
|
|
This might not work here as the package needs Python2.7 support. You can use six.raise_from though. |
|
Hi @mariocj89 , thanks very much for you comment. My fault not realizing the need to support Py2, I always fall into the trap of never considering Py2, may be that's because I actually never code with it. I've just arrived to the project (as user and as, now, contributor) so I am not yet much deep into the details. Looking into it, I see I will wait for the developers to comment further before submitting any other changes. Best and Thanks! |
|
@joaomcteixeira thank you for contributing! Please use |
|
As a first time contributor, please also add yourself to AUTHORS and add an entry to CHANGELOG. |
There was a problem hiding this comment.
@joaomcteixeira I haven't reviewed all your changes (that's a lot of work that you've put into this already!). I hadn't had time to watch @mariocj89 's talk. Could you please briefly summarize the rationale for showing all preceding exceptions when we re-raise an exception?
The original idea was that we follow Python's "it is easier to ask for forgiveness than for permission" and sometimes we know (or assume) that an exception as it is occurs is meaningless to the user because the real problem is something different (e.g., when we turn an AttributeError into a TypeError when the wrong input object was provided and our duck-typing failed).
But I am open to hear a different argument. And I am sure that there are a number of cases where your approach is definitely providing additional useful information but before going further I'd like to better understand what we're doing and why.
package/MDAnalysis/analysis/align.py
Outdated
| # treat subselection as AtomGroup | ||
| mobile_atoms = subselection.atoms | ||
| except AttributeError: | ||
| except AttributeError as e: |
There was a problem hiding this comment.
We prefer variable names with multiple letters such as err as it is a lot easier to search for.
| except KeyError: | ||
| raise ValueError('Unit ' + str(value) + ' of type ' + str(unit_type) + ' is not recognized.') | ||
| except AttributeError: | ||
| except KeyError as e: |
There was a problem hiding this comment.
We prefer variable names with multiple letters such as err as it is a lot easier to search for. (err is also already being used elsewhere, as you saw.)
(This comment applies throughout.)
package/MDAnalysis/analysis/align.py
Outdated
| except AttributeError as e: | ||
| raise TypeError("subselection must be a selection string, an" | ||
| " AtomGroup or Universe or None") | ||
| " AtomGroup or Universe or None") from e |
There was a problem hiding this comment.
Do you think the user needs to know why we ended up with the TypeError?
|
Hi @orbeckst
In the current implementation (master branch), the user will know about both try:
# do something and a AttributeError raises!
None.atoms()
except AttributeError:
# We catch the exception
# and we convert it to a TypeError
raise TypeError('error to show the user')
# and this is what the user seesThe key problem here is the sentence: In Python, this strictly means that we were not able to handle the Solution 1: try:
# do something and a AttributeError raises!
None.atoms()
except AttributeError as err:
# We catch the exception
# and we convert it to a TypeError
raise TypeError('error to show the user') from err
# and this is what the user seesHence we have: Whatever happened was expected to happen. Solution 2 which outputs only: Comments I hope this explanation was helpful. |
|
Hi, Look forward to your answer so I can complete the task, |
|
@joaomcteixeira thank you for the explanation. You are right that the intention has been
(that's how it worked in Py2, see below) Make the change – or wait for MDA > 1.0The Python 3 code should behave in the way the Python 2 code did, as you identified with I don't know if it easy to make this work in Python 2.7: If it is difficult/ugly to make the code work for Py 2 and Py 3 then I would wait for when we retire Python 2 support. The current plan is still somewhat in flux #2303 but we want to do a (more or less) "final" MDAnalysis 1.0 that still supports Python 2.7 with all the features that users know. We then start on 2.0, which will drop Py 2. (The timeline says "MDA 1.0 by the end of 2019 but this might be a bit ambitious.) Py2 vs Py3 behaviorI had to check how Py2 behaved because I was surprised to read your explanation. MDAnalysis had started around Python 2.3... This is how it used to work in Python 2: Python 2.7.15 | packaged by conda-forge | (default, Jul 2 2019, 00:42:22)
Type "copyright", "credits" or "license" for more information.
IPython 5.8.0 -- An enhanced Interactive Python.
? -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help -> Python's own help system.
object? -> Details about 'object', use 'object??' for extra details.
In [1]: foo = 1
In [2]: try:
...: foo.bar
...: except AttributeError as err:
...: raise TypeError("Don't use foo")
...:
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-2-b01af9547409> in <module>()
2 foo.bar
3 except AttributeError as err:
----> 4 raise TypeError("Don't use foo")
TypeError: Don't use foobut Python 3 change it: Python 3.6.7 | packaged by conda-forge | (default, Feb 28 2019, 02:16:08)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.5.0 -- An enhanced Interactive Python. Type '?' for help.
In [1]: foo = 1
In [2]: try:
...: foo.bar
...: except AttributeError as err:
...: raise TypeError("Don't use foo")
...:
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
<ipython-input-2-2127b0be432b> in <module>
1 try:
----> 2 foo.bar
3 except AttributeError as err:
AttributeError: 'int' object has no attribute 'bar'
During handling of the above exception, another exception occurred:
TypeError Traceback (most recent call last)
<ipython-input-2-2127b0be432b> in <module>
2 foo.bar
3 except AttributeError as err:
----> 4 raise TypeError("Don't use foo")
5
TypeError: Don't use foo |
updates some missing lines from previous commit corrects deleted vars passes tests py37, lookforward towards Travis results
|
Hi @orbeckst , Thanks for you comments. I have made the changes accordingly. As suggested above, I used The last commit passes all tests for py37 in my computer. Travis has one broken build 1297.7, says: I think that is nothing introduced in this PR. Can you help me out, is there something I am missing? Look forward your reply, |
|
Can you just add the Thanks for your massive effort! |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
This looks very good, thank you for the effort.
The coverage seems to have decreased, though. I haven't had the time to look where exactly the decrease occurred. It might be that this is simply due to the fact that you increased the number of lines of code insight except statements that were never tested before. In this case, this is ok – it's not the in the scope of this PR to increase test coverage. However, could you please check the coverage report and see if there is code that was tested before but now is not tested anymore?
Then summarize your findings in a comment here. Thank you!
|
Thanks @orbeckst , |
Coverage might have decreased in previous commits because errmsg were being defined before raise the errors. Error messages are no longer defined beforehand and strings are passed directed to Error calls. I have not tested locally this commit and will rely only on CI online tests.
|
Can you please check why the tests are failing now and fix? (Please excuse my brevity, I’m travelling) |
Solves syntax error from 8b0582a addresses MDAnalysis#2357 (comment) Tests pass for python3.7 on my computer.
|
Hello @orbeckst The latest commits clean the additional variables presented in the first commits of this PR that defined strings that were then passed to the Yet, Coverage reports differ from Appveyor and Travis. Actually, when AppVeyor finishes, coverage reports are higher than previously, while these get reduced during Travis, I do not know if these difference in coverage reports is an acknowledged and expected behavior within the project. By analyzing the coverage reports I cannot understand where more can I operate to increase coverage percentage for the changes I propose apart from actually writing the tests of the cases where Please let me know if there is something more I can help with. EDIT (after Travis build): Looking to the Codecov results, I cannot explain properly the sudden decrease in coverage on the last commit and the differences between coverage of the different commits; many of the highlighted code lines are actually not part of this PR. Currently, here ends my knowledge on Codecov reports. I lookforward any discussions. |
|
@joaomcteixeira thank you for doing all the extra work. I had a quick look and I don't understand what coverage thinks here either. What you did looks fine to me and I'll squash+merge in a minute. |
|
Congratulations @joaomcteixeira to your first merged commit! Thank you for all your work. I look forward to more contributions! (I'll send you an invite to the MDAnalysis contributor group.) |
|
Hello @orbeckst |
3 participants
I did not rise any issue previously to this PR.
Problem
Exceptions raised while handling previous caught exceptions were not raised from the caught exception nor from
None, producing the output:Which in Python means "failed to handle exception properly" or that an uncontrolled exception occurred, which is not the case in MDAnalysis whatsoever.
Solution
By capturing exception as
as eand rising exceptionsfrom ethe correct output yields:Custom exceptions are raised
from None, instead.I did not entered into the detail of fine tuning
from eandfrom Noneaside from usingfrom Nonein custom exceptions because I considered such to be already changing MDAnalysis functionality/interface and I did not want to enter in such detail for this PR.Thanks to @mariocj89 for his talk at PyCon2019 about correct exception handling.
Tests
Tests run without failure (reds):
and pylint,
PR Checklist