Delay scikit-learn import until first use #1061
Conversation
|
Maybe we could use lazy import functions defined in Terra ( |
|
I like that idea. I will try that tomorrow. |
|
This should be okay to review now. A couple of the files have big diffs in GitHub because the line endings got changed from Windows to Unix. I can revert that if that helps. I used terra's LazyImporter but not its HAS_SKLEARN. LazyImporter tries to check for specific elements in the imported module and we use different ones from terra. It might be overkill to check for specific objects any way. I used the LazyImoprter in the tests as well, though they test for sklearn differently -- to decide whether to skip the tests or not. I made them consistent about skipping sklearn tests. |
|
I added a test that the scikit-learn check gets used when scikit-learn can not be imported. I used a subprocess to do this which means that the test takes 1.4 seconds which is not my favorite, but we have a lot of tests that are slower than that at the moment any way. If we wanted to test other optional imports we could add them into that one subprocess. |
wshanks
added
Changelog: None
|
I decided to add a release note since this helps mitigate an issue that users reported (#1050). I moved this text out of the original description above since it doesn't need to go in the commit message: I would not say this PR fully addresses the problem in #1050. For that, we need to ensure that the either the IBM Quantum Lab environment is updated to have a working verison of scikit-learn (we want that any way to support the experiments that require scikit-learn) or to have the newer version of qiskit-experiments with this PR so that all the non-scikit-learn experiments work. One would hope both packages can be updated. |
|
|
||
|
|
||
| class SkLDA(BaseDiscriminator): | ||
| """A wrapper for the SKlearn linear discriminant analysis.""" |
There was a problem hiding this comment.
These classes should probably have a note in the docstring that they require sklearn to run.
There was a problem hiding this comment.
I added notes to the classes that say they require scikit-learn. If you want to get very pedantic, the classes here only require scikit-learn for the from_config methods. The __init__ methods could work with a custom class if it satisfied the Analysis interface of scikit-learn.
There was a problem hiding this comment.
That's true. In the future it would be helpful to have a how-to that explains how to work with discriminators and implement a custom class.
| @@ -86,3 +89,35 @@ def __init__(self, physical_qubits): | |||
| with self.assertWarns(DeprecationWarning): | |||
| instance = OldExperiment(qubit=0) | |||
| self.assertEqual(instance._physical_qubits, (0,)) | |||
|
|
|||
| def test_warn_sklearn(self): | |||
There was a problem hiding this comment.
Hmm, so I'm not sure this test does what we want because I ran it with the current main branch and it only threw the "scikit-learn import guard did not run on scikit-learn usage" error. I would have expected qiskit_experiments to not import at all, which is the error that was reported. When I made a fake dummy sklearn that did nothing except throw an exception, that successfully caused the import of qiskit_experiments to fail, so maybe we have to mock the module instead of setting sys.modules["sklearn"] to None.
There was a problem hiding this comment.
Yes, with the imports moving down into first use and the guard raising an exception on first use, I didn't think it was as important to test against importing scikit-learn raising a surprising error, but I think it is true that raising something different than ImportError would be better just to make sure no code gets added that does try: import sklearn; except ImportError since we prefer to keep that at first use rather than triggered by import qiskit_experiments (an alternative test would be to do what qiskit-terra does and just check the import tree after import qiskit_experiments).
There was a problem hiding this comment.
I added a new commit where I override builtins.__import__ to raise a different exception from ImportError. This gives a test that fails on main but passes in this branch.
test/library/characterization/test_multi_state_discrimination.py
Outdated
Show resolved
Hide resolved
Summary
This commit pushes the import of scikit-learn lower into the code, so that it does not occur until first usage.
Details and comments
Previously, scikit-learn was in a
tryblock which allowed for code that did not use scikit-learn to work fine when it was not installed. However, sometimes scikit-learn can be installed but have errors (like in #1050) which were not caught by thetryblock. Further delaying the import can help in this case. Additionally, scikit-learn is a little bit of a slow import, so not importing it when it is not needed gives a little bit of efficiency (maybe; mostly it imports scipy modules but those might get imported any way by other analysis code).