Our importing strategy needs some work

[obriente]

The current codebase is a bit of a mess w.r.t. imports - we have a fair few near-circular imports that are just hacked away and reappear during refactoring.

I'm not entirely sure what the correct standard is for importing in python, but especially as we're now Python3 only and don't have to worry about doing everything in a way that works for Python2 as well, if someone wants to find a more rigorous way to solve this problem it would be great.

@tanujkhattar - would be great if you could take a look at this, as it's a CS/standards issue rather than having anything to do with the physics.

[obriente]

Looking around, it appears that the solution to this is 'dont have everything depend on each other'. Lol - easier said than done.

However, maybe this is something we need to take into account in this new reorg, as we want to make sure that sensible code edits don't wind up breaking some hacked import that was working before.

One breaking refactor that might be nice would be to make modules like ops (and perhaps also linalg) not depend on any higher-level modules (e.g. transforms/utils). For example, QuadraticHamiltonian currently contains a lot of code to transform a QuadraticHamiltonian, that would probably be more appropriate inside transforms (this has caused a circular import issue in PR #632 that I could only fix by pulling some imports within functions, which is not ideal).

[obriente]

Looking at cirq's strategy (https://github.com/quantumlib/Cirq/blob/master/cirq/init.py), it seems it takes two parts:

1. Make a rough hierarchy of module dependencies which is enforced in testing with feedback --- https://github.com/quantumlib/Cirq/blob/master/dev_tools/import_test.py
2. For any circular imports that can't be avoided, delay them using the delay_import function in https://github.com/quantumlib/Cirq/blob/master/cirq/_import.py .

It seems this would be reasonable to enforce here as well, but then we should pin down exactly what our dependencies look like, and also see what of the testing/delaying code can be directly taken from cirq.

[tanujkhattar]

I did some reading and spent time investigating this issue. My thoughts are as follows:

1. import statements should be used for modules and packages only, not for individual classes or functions (except in __init__.py).

Errors due to circular imports happen mainly when, during a circular import, python is unable to find a symbol x from module m because m has already begun loading (reference to m was inserted into the cache and is fetched from the cache during next lookup) but it hasn't finished executing yet (and therefore a reference to m.x doesn't exist in the cache yet). While traversing the cycle, we lookup symbol x (for eg: during from m import x) and python throws an error. This can be avoided if we used import m at the last node in the cycle traversal instead. While accessing x via m.x inside the file, python would still complain in cases where m.x is needed at the top level because the whole file is executed during import, but it would not complain if m.x is used inside functions / classes etc. (which is the most common use case)

For eg: PR632 introduced a TODO to move the import statement out. If we do that, the error we get is as follows:

>>> import openfermion
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/__init__.py", line 20, in <module>
    from openfermion.ops import *
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/ops/__init__.py", line 20, in <module>
    from .representations import *
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/ops/representations/__init__.py", line 10, in <module>
    from .quadratic_hamiltonian import (QuadraticHamiltonian,
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/ops/representations/quadratic_hamiltonian.py", line 20, in <module>
    from openfermion.linalg.givens_rotations import (
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/linalg/__init__.py", line 12, in <module>
    from .davidson import (Davidson, DavidsonOptions, DavidsonError, QubitDavidson,
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/linalg/davidson.py", line 24, in <module>
    from openfermion.linalg.sparse_tools import get_linear_qubit_operator_diagonal
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/linalg/sparse_tools.py", line 26, in <module>
    from openfermion.transforms.opconversions import normal_ordered
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/transforms/__init__.py", line 13, in <module>
    from .opconversions import *
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/transforms/opconversions/__init__.py", line 31, in <module>
    from .reverse_jordan_wigner import reverse_jordan_wigner
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/transforms/opconversions/reverse_jordan_wigner.py", line 15, in <module>
    from openfermion.hamiltonians.special_operators import number_operator
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/hamiltonians/__init__.py", line 28, in <module>
    from .jellium_hf_state import hartree_fock_state_jellium
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/hamiltonians/jellium_hf_state.py", line 18, in <module>
    from openfermion.transforms.repconversions import inverse_fourier_transform
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/transforms/repconversions/__init__.py", line 1, in <module>
    from .conversions import (get_interaction_operator,
  File "/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/transforms/repconversions/conversions.py", line 22, in <module>
    from openfermion.ops.representations.quadratic_hamiltonian import (
ImportError: cannot import name 'QuadraticHamiltonian' from 'openfermion.ops.representations.quadratic_hamiltonian' (/Users/tanujkhattar/PycharmProjects/OpenFermion/src/openfermion/ops/representations/quadratic_hamiltonian.py)

Notice the cycle caused here is as follows:

ops/representations/quadratic_hamiltonian.py -->  ........ --> transforms/repconversions/conversions.py --> ops/representations/quadratic_hamiltonian.py 

and the error says

ImportError: cannot import name 'QuadraticHamiltonian' from 'openfermion.ops.representations.quadratic_hamiltonian'

This happened because we tried to access QuadraticHamiltonian after quadratic_hamiltonian.py had started executing, and therefore existed in module cache from where it was picked in conversions.py, but before it was executed fully, and therefore QuadraticHamiltonian was not yet defined.

But, we don't really need QuadraticHamiltonian at the top level in transforms/repconversions/conversions.py. Therefore, modifying the import statement in conversions.py from

from openfermion.ops.representations.quadratic_hamiltonian import (
    QuadraticHamiltonian, QuadraticHamiltonianError)

to

import openfermion.ops.representations.quadratic_hamiltonian as qh

and modifying the call sites of QuadraticHamiltonian and QuadraticHamiltonianError resolves the circular import issue in this case and hence is enough to fix the TODO.

This approach is also recommended by the google's python style guide and should be a fairly easy fix to do, as compared to other approaches like changing the design etc.

2. Dont have everything depend on each other, but first we need to understand what our dependencies look like?

This is problem is highlighted well in the comments above by @obriente. However, we need to find good ways of understanding the complex dependencies. I played around with tools like pydeps to generate the dependency graphs, but analyzing these graphs is non trivial and I don't have any concrete action items.

I think finding such bad dependencies would be a bigger project and would require significant time commitment. A good approach, IMO, would be to flag any such bad dependency which we observe while refactoring / adding new features and tackle them as and when we find them.

Disclaimer

I am still pretty new to python (my background is mainly in c++) so It would be great to hear from others who have more experience. I am very interested to learn more about better ways of solving problem 2 above.

[ncrubin]

@tanujkhattar Thanks for the analysis and recommendation! I agree with your second point. good to know about pydeps. Seems useful.

[obriente]

@tanujkhattar Thanks for the analysis! And thanks for finding pydeps. I'm running into a recursion depth error when I run it (similar to those in https://github.com/thebjorn/pydeps/issues/33) --- how did you get it to work?

Regarding your first recommendation, I know that we've forced using the full name of each import in openfermion up till now. I think this has made the possibility of simply importing files unwieldy as our filenames tend to be quite long.

The Google style guide suggests (copied):

• Use import x for importing packages and modules.
• Use from x import y where x is the package prefix and y is the module name with no prefix.
• Use from x import y as z if two modules named y are to be imported or if y is an inconveniently long name.
• Use import y as z only when z is a standard abbreviation (e.g., np for numpy).

Following this, it seems import openfermion.ops.representations.quadratic_hamiltonian as qh would technically not be in style, but from openfermion.ops.representations import quadratic_hamiltonian as qh would pass. Does this work the same as far as circular dependencies go?

@ncrubin , how do you feel about the style change if it works?

Cheers,

Tom.

[tanujkhattar]

I'm running into a recursion depth error when I run it (similar to those in thebjorn/pydeps#33) --- how did you get it to work?

This is because the graph size is too big so we need to increase python's default recursion depth in pydeps binary. To fix this, I added sys.setrecursionlimit(10000) at the beginning of the pydeps script, by directly editing the file present at path returned by:

$> type pydeps
pydeps is /Users/tanujkhattar/opt/anaconda3/bin/pydeps # edit this file. 

Does this work the same as far as circular dependencies go?

Yes! All the same arguments hold for this approach as well. IIUC, these two approaches are identical.

[obriente]

Ah ok, thanks - I had assumed pydeps was running into an infinite loop rather than just a really large one. I'll try that out.

If that works, it might be a simple solution to this circular dependencies problem, though it will be quite a large change throughout. I'm still a bit worried though that a) this might reappear in the future under another guise, and b) having an import hierarchy is probably something we want to aim for regardless. (Especially as I notice cirq doesn't follow this.)

There's also the relatively minor complaint that this makes basically every function call 3-4 characters longer, though this isn't such a big deal in the big scheme of things.

@ncrubin , and maybe also @babbush, @mpharrigan and @kevinsung , what's your opinion on this?

[mpharrigan]

One thing that needs to be ironed out is whether we're committed to percolating everything up to the top-level. This can provide convenience and discoverability of functionality and we do it in cirq by being very careful about layering our intra-package dependencies and importing submodules (not functions; classes directly) within the package. Crucially, all user code [including tests! which should be written as if a user was executing a script!] uses the top level cirq.x imports.

In openfermion, it seems that everything is written assuming you'll use pycharm, press alt+. to get the fully-qualified import everywhere and be on your merry way using classes and functions directly. This is another style. As pointed out, this isn't the official google style but I personally like it better :) As you all are finding out, this doesn't mesh well with throwing everything into a top-level import as well.

In fact, I was able to solve the specific instance @tanujkhattar identified above (i.e. moving the local import to the top of the package) by strategically blowing away some imports from __init__.py and changing a couple of things in the test to use fully-qualified names

[mpharrigan]

Here's a patch of my hack-job:

click to expand

diff --git a/src/openfermion/__init__.py b/src/openfermion/__init__.py
index 8c74832..032ec93 100644
--- a/src/openfermion/__init__.py
+++ b/src/openfermion/__init__.py
@@ -223,16 +223,6 @@ from openfermion.ops import (
     shift_decoder,
     BinaryCode,
     BinaryCodeError,
-    PolynomialTensor,
-    PolynomialTensorError,
-    general_basis_change,
-    DiagonalCoulombHamiltonian,
-    InteractionOperator,
-    InteractionOperatorError,
-    InteractionRDM,
-    InteractionRDMError,
-    QuadraticHamiltonian,
-    QuadraticHamiltonianError,
 )
 
 from openfermion.testing import (
diff --git a/src/openfermion/circuits/slater_determinants.py b/src/openfermion/circuits/slater_determinants.py
index 4098cca..9ada1c3 100644
--- a/src/openfermion/circuits/slater_determinants.py
+++ b/src/openfermion/circuits/slater_determinants.py
@@ -15,12 +15,12 @@ Slater determinants and fermionic Gaussian states."""
 import numpy
 
 from openfermion.config import EQ_TOLERANCE
-from openfermion.ops import QuadraticHamiltonian
 from openfermion.linalg.givens_rotations import (
     fermionic_gaussian_decomposition, givens_decomposition)
 from openfermion.linalg.sparse_tools import (
     jw_configuration_state, jw_sparse_givens_rotation,
     jw_sparse_particle_hole_transformation_last_mode)
+from openfermion.ops.representations.quadratic_hamiltonian import QuadraticHamiltonian
 
 
 def gaussian_state_preparation_circuit(quadratic_hamiltonian,
diff --git a/src/openfermion/circuits/trotter/algorithms/linear_swap_network.py b/src/openfermion/circuits/trotter/algorithms/linear_swap_network.py
index 3d4b606..7776a16 100644
--- a/src/openfermion/circuits/trotter/algorithms/linear_swap_network.py
+++ b/src/openfermion/circuits/trotter/algorithms/linear_swap_network.py
@@ -13,11 +13,12 @@
 
 from typing import cast, Optional, Sequence, Tuple
 
+from openfermion.ops.representations import DiagonalCoulombHamiltonian
+
 import cirq
 
 from openfermion.circuits.gates import (Ryxxy, Rxxyy, CRyxxy, CRxxyy, rot111,
                                         rot11)
-from openfermion.ops import DiagonalCoulombHamiltonian
 from openfermion.circuits.primitives import swap_network
 from openfermion.circuits.trotter.trotter_algorithm import (Hamiltonian,
                                                             TrotterStep,
diff --git a/src/openfermion/circuits/trotter/algorithms/low_rank.py b/src/openfermion/circuits/trotter/algorithms/low_rank.py
index 1db05df..69af74d 100644
--- a/src/openfermion/circuits/trotter/algorithms/low_rank.py
+++ b/src/openfermion/circuits/trotter/algorithms/low_rank.py
@@ -14,6 +14,7 @@
 from typing import cast, Optional, Sequence, TYPE_CHECKING, Tuple
 
 import numpy
+from openfermion.ops.representations import InteractionOperator
 
 import cirq
 
@@ -60,7 +61,7 @@ class LowRankTrotterAlgorithm(TrotterAlgorithm):
     where x is a truncation threshold specified by user.
     """
 
-    supported_types = {ops.InteractionOperator}
+    supported_types = {InteractionOperator}
 
     def __init__(self,
                  truncation_threshold: Optional[float] = 1e-8,
diff --git a/src/openfermion/circuits/trotter/algorithms/split_operator.py b/src/openfermion/circuits/trotter/algorithms/split_operator.py
index 7fe83fa..237fbe1 100644
--- a/src/openfermion/circuits/trotter/algorithms/split_operator.py
+++ b/src/openfermion/circuits/trotter/algorithms/split_operator.py
@@ -13,6 +13,8 @@
 
 from typing import cast, Optional, Sequence, Tuple
 
+from openfermion.ops.representations import DiagonalCoulombHamiltonian
+
 import cirq
 
 # import openfermion.circuits.gates as gates
@@ -38,7 +40,7 @@ class SplitOperatorTrotterAlgorithm(TrotterAlgorithm):
     """
     # TODO Maybe use FFFT
 
-    supported_types = {ops.DiagonalCoulombHamiltonian}
+    supported_types = {DiagonalCoulombHamiltonian}
 
     def symmetric(self, hamiltonian: Hamiltonian) -> Optional[TrotterStep]:
         return SymmetricSplitOperatorTrotterStep(hamiltonian)
diff --git a/src/openfermion/circuits/trotter/trotter_algorithm.py b/src/openfermion/circuits/trotter/trotter_algorithm.py
index 2040aaa..eeff1ac 100644
--- a/src/openfermion/circuits/trotter/trotter_algorithm.py
+++ b/src/openfermion/circuits/trotter/trotter_algorithm.py
@@ -15,6 +15,8 @@ from typing import Optional, Sequence, TYPE_CHECKING, Tuple, Union
 
 import abc
 
+from openfermion.ops.representations import DiagonalCoulombHamiltonian, InteractionOperator
+
 import cirq
 from openfermion import ops
 
@@ -22,8 +24,8 @@ if TYPE_CHECKING:
     # pylint: disable=unused-import
     from typing import Set, Type
 
-Hamiltonian = Union[ops.FermionOperator, ops.QubitOperator, ops.
-                    InteractionOperator, ops.DiagonalCoulombHamiltonian]
+Hamiltonian = Union[
+    ops.FermionOperator, ops.QubitOperator, InteractionOperator, DiagonalCoulombHamiltonian]
 
 
 class TrotterStep(metaclass=abc.ABCMeta):
diff --git a/src/openfermion/ops/__init__.py b/src/openfermion/ops/__init__.py
index 8d978d5..9de7a13 100644
--- a/src/openfermion/ops/__init__.py
+++ b/src/openfermion/ops/__init__.py
@@ -32,15 +32,3 @@ from .operators import (
     BinaryCodeError,
 )
 
-from .representations import (
-    PolynomialTensor,
-    PolynomialTensorError,
-    general_basis_change,
-    DiagonalCoulombHamiltonian,
-    InteractionOperator,
-    InteractionOperatorError,
-    InteractionRDM,
-    InteractionRDMError,
-    QuadraticHamiltonian,
-    QuadraticHamiltonianError,
-)
diff --git a/src/openfermion/ops/representations/__init__.py b/src/openfermion/ops/representations/__init__.py
index 78599ad..9703a50 100644
--- a/src/openfermion/ops/representations/__init__.py
+++ b/src/openfermion/ops/representations/__init__.py
@@ -16,7 +16,3 @@ from .interaction_rdm import (
     InteractionRDMError,
 )
 
-from .quadratic_hamiltonian import (
-    QuadraticHamiltonian,
-    QuadraticHamiltonianError,
-)
diff --git a/src/openfermion/ops/representations/quadratic_hamiltonian.py b/src/openfermion/ops/representations/quadratic_hamiltonian.py
index 5534271..d705a8f 100644
--- a/src/openfermion/ops/representations/quadratic_hamiltonian.py
+++ b/src/openfermion/ops/representations/quadratic_hamiltonian.py
@@ -17,6 +17,8 @@ import numpy
 from scipy.linalg import schur
 
 from openfermion.ops.representations import PolynomialTensor
+from openfermion.linalg.givens_rotations import (
+    fermionic_gaussian_decomposition, givens_decomposition_square)
 
 
 class QuadraticHamiltonianError(Exception):
@@ -373,8 +375,6 @@ class QuadraticHamiltonian(PolynomialTensor):
         """
         # Adding inline import here to prevent circular issues
         # TODO: move this out once we have a better solution
-        from openfermion.linalg.givens_rotations import (
-            fermionic_gaussian_decomposition, givens_decomposition_square)
         _, transformation_matrix, _ = self.diagonalizing_bogoliubov_transform()
 
         if self.conserves_particle_number:
diff --git a/src/openfermion/ops/representations/quadratic_hamiltonian_test.py b/src/openfermion/ops/representations/quadratic_hamiltonian_test.py
index 343d651..c33a648 100644
--- a/src/openfermion/ops/representations/quadratic_hamiltonian_test.py
+++ b/src/openfermion/ops/representations/quadratic_hamiltonian_test.py
@@ -18,7 +18,7 @@ import scipy.sparse
 
 from openfermion.hamiltonians.special_operators import majorana_operator
 from openfermion.ops.operators import FermionOperator
-from openfermion.ops.representations import QuadraticHamiltonian
+from openfermion.ops.representations.quadratic_hamiltonian import QuadraticHamiltonian
 from openfermion.transforms.opconversions import (get_fermion_operator,
                                                   normal_ordered, reorder)
 from openfermion.transforms.repconversions import get_quadratic_hamiltonian
diff --git a/src/openfermion/testing/testing_utils.py b/src/openfermion/testing/testing_utils.py
index 03624d7..2e83154 100644
--- a/src/openfermion/testing/testing_utils.py
+++ b/src/openfermion/testing/testing_utils.py
@@ -18,8 +18,8 @@ import numpy
 from scipy.linalg import qr
 
 from openfermion.ops.operators import QubitOperator
+from openfermion.ops.representations.quadratic_hamiltonian import QuadraticHamiltonian
 from openfermion.ops.representations import (DiagonalCoulombHamiltonian,
-                                             QuadraticHamiltonian,
                                              InteractionOperator)