BUG: Add a lock to assert_equal and other testing functions #8427
Conversation
| # in the case of skimage and probably most testing scenarios and the | ||
| # chance of deadlocks seems very unlikely. | ||
| _assert_comparison_lock = Lock() | ||
| except ImportError: |
There was a problem hiding this comment.
When is threading.Lock not available?
There was a problem hiding this comment.
I thought there are some systems/python without threading support or only dummy threading or so?
There was a problem hiding this comment.
Though, seems like in fft and some other places, we do not guard against it, in docutils there is some guard using dummy_threading.
There was a problem hiding this comment.
if python is built without threading, dummy_threading is still available
There was a problem hiding this comment.
right, that is nicer then the code duplication then.
| if not (desired == actual): | ||
| raise AssertionError(msg) | ||
| return | ||
| with _assert_comparison_lock: |
There was a problem hiding this comment.
you can put two statements into one with, with _assert_comparison_lock, suppress_warnings() as sup:
it looks like the lock it makes more sense inside the warning context manager, but if this is just temporary its probably ok.
There was a problem hiding this comment.
Yes, only that it does not work inside the warning context manager because the context manager could be nested. There may be ways to do it, but it would be pretty complicated I bet. I see this only as a temporary solution, because I am not sure I want to remove the suppressions here within the release cycle.
There was a problem hiding this comment.
OK, changed those two things.
There was a problem hiding this comment.
I suppose the lock fix could also be done in skimage, its a bit tricky. This only band-aids the cases in skimage, might band-aid some other cases, but it is definitely not a real fix....
This lock prevents unsafe warning filter manipulations during testing if downstream packages do parallel testing. Warning filtering is generally not threadsafe in python, this is also true for `catch_warnings` or `suppress_warinings`. In NumPy 1.12 however, `assert_equal` and the array comparison asserts, use this to filter out some comparison warnings. Since removing this filter may also affect downstream projects and skimage (and possibly more) do parallel manual parallel testing using `assert_equal`, a quick fix seems to be to lock the less obvious threading trap. Ideally (in master this is the case), there should simply not be warning filter logic in the assert functions themself. While probably not perfectly safe in principle, it is sufficient in the case of skimage and probably most testing scenarios and the chance of deadlocks seems very unlikely. Closes numpygh-8413
|
@juliantaylor Look good to you? |
This lock prevents unsafe warning filter manipulations during testing
if downstream packages do parallel testing.
Warning filtering is generally not threadsafe in python, this is also
true for
catch_warningsorsuppress_warinings. In NumPy 1.12however,
assert_equaland the array comparison asserts, use thisto filter out some comparison warnings. Since removing this filter
may also affect downstream projects and skimage (and possibly more)
do parallel manual parallel testing using
assert_equal, a quick fixseems to be to lock the less obvious threading trap. Ideally (in
master this is the case), there should simply not be warning filter
logic in the assert functions themself.
While probably not perfectly safe in principle, it is sufficient
in the case of skimage and probably most testing scenarios and the
chance of deadlocks seems very unlikely.
Closes gh-8413