-
Notifications
You must be signed in to change notification settings - Fork 587
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
Avoid being the first to import large packages #2204
Conversation
7db9257
to
8da50fd
Compare
return tuple(sorted(exceptions, key=str)) | ||
|
||
|
||
@run_once | ||
def failure_exceptions_to_catch(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not wild about running these methods on every exception raised - given that that raising exceptions is the main thing Hypothesis tests do it's pretty hot path, and doing all this conditional querying as to whether stuff is already in sys.modules
is technically fine but looks pretty weird and results in all of these coverage pragmas.
How about instead catching all exceptions and checking whether the current exception is one to reraise based on a string version of its name? e.g. something like:
from hypothesis.internal.compat import qualname
SKIP_EXCEPTIONS = frozenset({
'unittest.SkipTest',
'unittest2.SkipTest',
'nose.SkipTest',
'_pytest.outcomes.Skipped',
})
...
except BaseException as e:
if qualname(e) in SKIP_EXCEPTIONS or (
not isinstance(e, Exception) and qualname(e) != '_pytest.outcomes.Failed'
):
raise
# normal handling logic continues here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That really doesn't look better to me! The sys.modules checks are unusual, but the whole function is fast - we have four dict lookups, and up to five attribute accesses. The slowest part is sorting the types by str!
And we already had those coverage pragmas, spread out across more lines of code.
else: | ||
if not CAN_PACK_HALF_FLOAT: # pragma: no cover | ||
try: | ||
import numpy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not care about non-lazily importing numpy in this case? I guess it only matters until January.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for Python 3.5, so until September!
But basically no, I don't care about this case - pypistats show very few people still using 3.5. A lazy import here would be pretty awful code, impact the floats() check too, and we'd be taking it out in 10 months anyway. I'll add it if someone asks, though.
393899c
to
7418432
Compare
Via astropy/astropy#9532 (comment), our attempted import of
nose
(to get itsSkipTest
exception) can cause problems in some setups.So I've adapted this trick to ensure that we only import test runner packages (and numpy) if someone else did so first. If it's never imported, we don't need to integrate with it!
Caveats:
st.from_type()
still does optimistic imports, and if you're on Python <= 3.5numpy
will be imported to support 16-bit floats. Otherwise we should be a bit lighter than before 😄