Adopt lazy imports from Terra by manoelmarques · Pull Request #535 · qiskit-community/qiskit-nature

manoelmarques · 2022-02-04T00:09:54Z

Summary

Adopt the API implemented in Qiskit/qiskit#7525

Details and comments

coveralls · 2022-02-04T01:17:55Z

Pull Request Test Coverage Report for Build 1799728890

65 of 69 (94.2%) changed or added relevant lines in 11 files are covered.
9 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.07%) to 82.661%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit_nature/drivers/second_quantization/psi4d/psi4driver.py	4	5	80.0%
qiskit_nature/drivers/second_quantization/pyquanted/pyquantedriver.py	10	11	90.91%
qiskit_nature/drivers/second_quantization/gaussiand/gaussian_utils.py	1	3	33.33%

Files with Coverage Reduction	New Missed Lines	%
qiskit_nature/drivers/second_quantization/gaussiand/gaussian_log_driver.py	2	46.43%
qiskit_nature/drivers/second_quantization/psi4d/psi4driver.py	2	86.51%
qiskit_nature/drivers/second_quantization/gaussiand/gaussiandriver.py	5	72.5%

Totals
Change from base Build 1796228481:	0.07%
Covered Lines:	8338
Relevant Lines:	10087

💛 - Coveralls

mrossinek

In general this looks good to me 👍 I have some minor comments.

I did grep for ImportError on the repo and did find a few more occurrences though. Maybe those can be refactored, too?

qiskit_nature/drivers/second_quantization/gaussiand/gaussiandriver.py:257:        except ImportError as mnfe:
test/drivers/second_quantization/fcidumpd/test_driver_fcidump_dumper.py:138:        except ImportError:
test/algorithms/ground_state_solvers/test_groundstate_eigensolver.py:320:        except ImportError as ex:
test/algorithms/ground_state_solvers/test_groundstate_eigensolver.py:408:        except ImportError as ex:
test/algorithms/ground_state_solvers/test_groundstate_eigensolver.py:434:        except ImportError as ex:
test/algorithms/ground_state_solvers/test_groundstate_eigensolver.py:465:        except ImportError as ex:
test/test_readme_sample.py:41:        except ImportError as ex:

Grepping for MissingOptionalLibraryError gives the following:

qiskit_nature/problems/second_quantization/lattice/lattices/lattice.py:216:            MissingOptionalLibraryError: Requires matplotlib.
qiskit_nature/drivers/second_quantization/vibrational_structure_molecule_driver.py:22:from qiskit.exceptions import MissingOptionalLibraryError
qiskit_nature/drivers/second_quantization/vibrational_structure_molecule_driver.py:54:            MissingOptionalLibraryError: Driver not installed.
qiskit_nature/drivers/second_quantization/vibrational_structure_molecule_driver.py:64:                    except MissingOptionalLibraryError as ex:
qiskit_nature/drivers/second_quantization/vibrational_structure_molecule_driver.py:73:                raise MissingOptionalLibraryError(
qiskit_nature/drivers/second_quantization/electronic_structure_molecule_driver.py:22:from qiskit.exceptions import MissingOptionalLibraryError
qiskit_nature/drivers/second_quantization/electronic_structure_molecule_driver.py:59:            MissingOptionalLibraryError: Driver not installed.
qiskit_nature/drivers/second_quantization/electronic_structure_molecule_driver.py:72:                    except (MissingOptionalLibraryError, UnsupportMethodError) as ex:
qiskit_nature/drivers/second_quantization/electronic_structure_molecule_driver.py:81:                raise MissingOptionalLibraryError(
test/drivers/second_quantization/test_molecule_driver.py:20:from qiskit.exceptions import MissingOptionalLibraryError
test/drivers/second_quantization/test_molecule_driver.py:73:        except MissingOptionalLibraryError as ex:
test/drivers/second_quantization/test_molecule_driver.py:92:                except MissingOptionalLibraryError as ex:
test/drivers/second_quantization/test_molecule_driver.py:156:        except MissingOptionalLibraryError as ex:

mrossinek · 2022-02-04T07:38:45Z

+if PSI4PROG is None:
+    PSI4PROG = PSI4


Why is this needed? If which cannot find the command how would we be able to use it?

@mrossinek Just because the LazySubprocessTester from Terra won't accept a None or empty command in the constructor and I still wanted to use its capabilities of actually running the command to see if it is installed.

Okay, then why is the same not being done in the gaussian case a few lines further up?

Ahhh, because unfortunately Gaussian doesn't have a command line option like version or whatever that lets me just simply run it without side effect of actually running input data. For this I created my own Lazy class derived from Terra that just checks if the command is None. Effectively just what was being done before:

class NatureLazyCommandTester(LazyDependencyManager):

I didn't use it for PSI4 because in that case I could do a better check of actually running it because it has a version option.

Oh I was not careful. The first time I read it, it looked like you used NatureLazyCommandTester for Gaussian and Psi4. But I see now that the latter actually uses the Terra-provided one 👍

What functionality would you like in LazySubprocessTester? I made it reject "empty" commands as a way of catching bugs because I couldn't think of any use-cases, but if you've got a use-case for empty commands, we could add it.

Here, I would note that calling which on import isn't lazy - if you wanted it for your Gaussian one, perhaps your derived class could run shutil.which to define its _is_available? It depends how lazy you care about being, though.

manoelmarques · 2022-02-04T14:06:20Z

In general this looks good to me 👍 I have some minor comments.

I did grep for ImportError on the repo and did find a few more occurrences though. Maybe those can be refactored, too?

qiskit_nature/drivers/second_quantization/gaussiand/gaussiandriver.py:257:        except ImportError as mnfe:
test/drivers/second_quantization/fcidumpd/test_driver_fcidump_dumper.py:138:        except ImportError:
test/algorithms/ground_state_solvers/test_groundstate_eigensolver.py:320:        except ImportError as ex:
test/algorithms/ground_state_solvers/test_groundstate_eigensolver.py:408:        except ImportError as ex:
test/algorithms/ground_state_solvers/test_groundstate_eigensolver.py:434:        except ImportError as ex:
test/algorithms/ground_state_solvers/test_groundstate_eigensolver.py:465:        except ImportError as ex:
test/test_readme_sample.py:41:        except ImportError as ex:

Grepping for MissingOptionalLibraryError gives the following:

qiskit_nature/problems/second_quantization/lattice/lattices/lattice.py:216:            MissingOptionalLibraryError: Requires matplotlib.
qiskit_nature/drivers/second_quantization/vibrational_structure_molecule_driver.py:22:from qiskit.exceptions import MissingOptionalLibraryError
qiskit_nature/drivers/second_quantization/vibrational_structure_molecule_driver.py:54:            MissingOptionalLibraryError: Driver not installed.
qiskit_nature/drivers/second_quantization/vibrational_structure_molecule_driver.py:64:                    except MissingOptionalLibraryError as ex:
qiskit_nature/drivers/second_quantization/vibrational_structure_molecule_driver.py:73:                raise MissingOptionalLibraryError(
qiskit_nature/drivers/second_quantization/electronic_structure_molecule_driver.py:22:from qiskit.exceptions import MissingOptionalLibraryError
qiskit_nature/drivers/second_quantization/electronic_structure_molecule_driver.py:59:            MissingOptionalLibraryError: Driver not installed.
qiskit_nature/drivers/second_quantization/electronic_structure_molecule_driver.py:72:                    except (MissingOptionalLibraryError, UnsupportMethodError) as ex:
qiskit_nature/drivers/second_quantization/electronic_structure_molecule_driver.py:81:                raise MissingOptionalLibraryError(
test/drivers/second_quantization/test_molecule_driver.py:20:from qiskit.exceptions import MissingOptionalLibraryError
test/drivers/second_quantization/test_molecule_driver.py:73:        except MissingOptionalLibraryError as ex:
test/drivers/second_quantization/test_molecule_driver.py:92:                except MissingOptionalLibraryError as ex:
test/drivers/second_quantization/test_molecule_driver.py:156:        except MissingOptionalLibraryError as ex:

@mrossinek I will review the ImportError to see if any can be refactored but the MissingOptionalLibraryError needs to be checked and needs to be kept in the documentation as being raised. That exception didn't go away. It is still being thrown by the Lazy import framework. It just has been delayed at the point where it is used.

mrossinek

Just one question otw this LGTM 🙂

jakelishman

I hope the tools are useful - let us/me know if there's anything about the existing Terra objects that might want expanding.

jakelishman · 2022-02-04T22:33:09Z

+            _optionals.HAS_GAUSSIAN.require_now("GaussianForcesDriver __init__")

    @staticmethod
+    @_optionals.HAS_GAUSSIAN.require_in_call("GaussianForcesDriver from_molecule")


If you don't supply an argument in the decorator form, it should use the qualified name of the method automatically. If you prefer, this line is equivalent to decorating with @_optionals.HAS_GAUSSIAN.require_in_call (i.e. without "calling" the decorator)

Oh that is helpful. Thanks for the tip. I will change it.

jakelishman · 2022-02-04T22:41:38Z

 logger = logging.getLogger(__name__)

-try:
+if _optionals.HAS_PYQUANTE2:


I don't know if you mind, but this isn't a lazy test - the Boolean evaluation will be triggered as soon as this module is imported.

Yes, I knew. We had those imports at the top because they are being used in different parts of the file and even in type annotations like:

self._mol: pyquante_molecule = None self._bfs: basisset = None

I will see if it is possible to move them inside the methods.

jakelishman · 2022-02-04T22:43:26Z

-            args = inspect.getfullargspec(PyQuanteDriver.__init__).args
+            args = inspect.signature(PyQuanteDriver.__init__).parameters.keys()


I put some effort into trying to make sure the wrappers would propagate through the signature and documentation (require_in_instance does a bit of magic to wrap the __init__ method). Let me know if something's broken about that.

(inspect.signature is the recommended way, though)

Yes, the getfullargspec args started returning an empty array. I changed to signature which works.

jakelishman · 2022-02-04T22:49:35Z

+if PSI4PROG is None:
+    PSI4PROG = PSI4


What functionality would you like in LazySubprocessTester? I made it reject "empty" commands as a way of catching bugs because I couldn't think of any use-cases, but if you've got a use-case for empty commands, we could add it.

Here, I would note that calling which on import isn't lazy - if you wanted it for your Gaussian one, perhaps your derived class could run shutil.which to define its _is_available? It depends how lazy you care about being, though.

jakelishman · 2022-02-04T22:56:26Z

+    not empty.
+    """
+
+    __slots__ = ("_command",)


Also, you don't need to do this if you don't want, especially if you'd like to attach data to the objects. To be honest, I probably shouldn't have done it in Terra to give you the choice of adding arbitrary data - from a Python perspective, it's a bad habit of mine that I over-use __slots__ (it comes from me preferring stricter typing, where you can't add attributes to stuff).

There's no real harm in it if you don't want to attach data to the objects, it's just a bit of overkill, and makes it harder if you later change your mind.

Yes, since I was creating a lazy class I decided to stick to the idiom there but already had someone else asking me about it too :-) I will remove it.

manoelmarques · 2022-02-05T14:32:00Z

@jakelishman About the LazySubprocessTester:

Nature has two drivers, PSI4 and Gaussian, that have their own commands. We use which as a way of figuring out if they are installed. In the PSI4 case, I used LazySubprocessTester because it is an improvement by running also the command. I don't use LazySubprocessTester for Gaussian though because it doesn't have any command option that would let it run without actually feeding chemistry input which gets complicated.

A subprocess tester that would be useful would be one where I construct it with a command string, but inside _is_available it runs which and if it returns None, is an error. Also it doesn't run the command automatically since there is no good way to run Gaussian. Maybe an additional flag in the __init__ telling if it should run the command.

I created a very simple subclass for Gaussian just to test None or not but I was thinking of doing what you suggested and put the which inside the _is_available. I will try this and you can take a look. If you think it is useful, it could be moved to Terra at some point.

mrossinek

LGTM! Thanks @manoelmarques!

* Adopt lazy imports from Terra * Migrate most ImportError to Lazy framework * Remove global optionals check from pyscf,pyquante * Create NatureLazySubprocessTester for Gaussian and PSI4

manoelmarques added the type: enhancement label Feb 4, 2022

manoelmarques requested a review from woodsp-ibm February 4, 2022 00:09

manoelmarques requested review from mrossinek, pbark and stefan-woerner as code owners February 4, 2022 00:09

manoelmarques self-assigned this Feb 4, 2022

manoelmarques force-pushed the lazy branch 2 times, most recently from 19e0059 to 6c10c53 Compare February 4, 2022 00:57

mrossinek reviewed Feb 4, 2022

View reviewed changes

manoelmarques requested a review from mrossinek February 4, 2022 14:20

manoelmarques force-pushed the lazy branch 2 times, most recently from 90dc2e2 to b894525 Compare February 4, 2022 16:02

mrossinek reviewed Feb 4, 2022

View reviewed changes

Comment thread test/algorithms/ground_state_solvers/test_groundstate_eigensolver.py

manoelmarques added the run_slow PR will run all unit tests decorated as slow label Feb 4, 2022

manoelmarques requested a review from mrossinek February 4, 2022 16:29

manoelmarques force-pushed the lazy branch 2 times, most recently from d95369e to 0c6ea0b Compare February 4, 2022 17:41

manoelmarques added 2 commits February 4, 2022 12:42

Adopt lazy imports from Terra

5f5f1d2

Migrate most ImportError to Lazy framework

03b5743

manoelmarques force-pushed the lazy branch from 0c6ea0b to 03b5743 Compare February 4, 2022 17:43

jakelishman reviewed Feb 4, 2022

View reviewed changes

Remove global optionals check from pyscf,pyquante

7ec8f4a

Create NatureLazySubprocessTester for Gaussian and PSI4

1dc2f13

mrossinek mentioned this pull request Feb 7, 2022

Support networkx.Graphs as input to Lattice #472

Merged

mrossinek approved these changes Feb 8, 2022

View reviewed changes

manoelmarques merged commit 508b81f into qiskit-community:main Feb 8, 2022

manoelmarques deleted the lazy branch February 8, 2022 14:03

mrossinek mentioned this pull request Feb 9, 2022

BOPESSampler does not support MinimumEigensolverFactories #510

Merged

		args = inspect.getfullargspec(PyQuanteDriver.__init__).args
		args = inspect.signature(PyQuanteDriver.__init__).parameters.keys()

Conversation

manoelmarques commented Feb 4, 2022

Summary

Details and comments

Uh oh!

coveralls commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1799728890

💛 - Coveralls

Uh oh!

mrossinek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manoelmarques Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

manoelmarques commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrossinek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manoelmarques Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manoelmarques commented Feb 5, 2022

Uh oh!

mrossinek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Feb 4, 2022 •

edited

Loading

manoelmarques Feb 4, 2022 •

edited

Loading

manoelmarques commented Feb 4, 2022 •

edited

Loading

manoelmarques Feb 4, 2022 •

edited

Loading

jakelishman Feb 4, 2022 •

edited

Loading