Fix cirq-core dependencies (#4369)

Some of the cirq-XXX modules were lacking the dependency on cirq-core.
This PR adds tests in isolation for each of the modules where they are installed and pytested in a clean environment.

Fixes #4361.

Author: balopat

.github/workflows/ci.yml

@@ -130,19 +130,19 @@ jobs:
         run: pip install -r dev_tools/requirements/deps/tensorflow-docs.txt
       - name: Doc check
         run: check/nbformat
-  cirq-only:
-    name: Pytest (cirq-only) Ubuntu
+  isolated-modules:
+    name: Isolated pytest Ubuntu
     runs-on: ubuntu-20.04
     steps:
       - uses: actions/checkout@v2
       - uses: actions/setup-python@v1
         with:
           python-version: '3.8'
           architecture: 'x64'
-      - name: Install requirements
-        run: pip install -r dev_tools/requirements/cirq-only.env.txt
-      - name: Pytest check
-        run: check/pytest --cirq-only --ignore=cirq-core/cirq/contrib --actually-quiet
+      - name: Install dependencies
+        run: pip install -r dev_tools/requirements/isolated-base.env.txt
+      - name: Test each module in isolation
+        run: pytest -n auto -m slow dev_tools/packaging/isolated_packages_test.py
   pytest:
     name: Pytest Ubuntu
     strategy:
@@ -267,7 +267,7 @@ jobs:
           python-version: '3.8'
           architecture: 'x64'
       - name: Install requirements
-        run: pip install -r dev_tools/requirements/isolated-notebooks.env.txt
+        run: pip install -r dev_tools/requirements/isolated-base.env.txt
       - name: Notebook tests
         run: check/pytest -n auto -m slow dev_tools/notebooks/isolated_notebook_test.py -k ${{matrix.partition}}
       - uses: actions/upload-artifact@v2

check/pytest

@@ -4,11 +4,9 @@
 # Runs pytest on the repository.
 #
 # Usage:
-#     check/pytest [--actually-quiet] [--cirq-only] [--flags for pytest] [file-paths-relative-to-repo-root]
+#     check/pytest [--actually-quiet] [--flags for pytest] [file-paths-relative-to-repo-root]
 #
 # The --actually-quiet argument filters out any progress output from pytest.
-# If --cirq-only is specified, only cirq-core tests are executed other cirq modules won't be available on the
-# PYTHONPATH, which is useful to test cirq-core's ability to function independently.
 #
 # You may specify pytest flags and specific files to test. The file paths
 # must be relative to the repository root. If no files are specified, everything
@@ -21,24 +19,15 @@ cd "$(git rev-parse --show-toplevel)"
 
 PYTEST_ARGS=()
 ACTUALLY_QUIET=""
-CIRQ_ONLY=""
 for arg in $@; do
     if [[ "${arg}" == "--actually-quiet" ]]; then
         ACTUALLY_QUIET=1
-    elif [[ "${arg}" == "--cirq-only" ]]; then
-        CIRQ_ONLY=1
     else
         PYTEST_ARGS+=("${arg}")
     fi
 done
 
-if [ -z "${CIRQ_ONLY}" ]; then
-  source dev_tools/pypath
-else
-  export PYTHONPATH=cirq-core
-  PYTEST_ARGS+=("./cirq-core")
-fi
-
+source dev_tools/pypath
 PYTHON_VERSION=$(python -V 2>&1 | sed 's/.* \([0-9]\).\([0-9]\).*/\1\2/')
 if [ "$PYTHON_VERSION" -lt "37" ]; then
   PYTEST_ARGS+=("--ignore=cirq-rigetti")

cirq-aqt/setup.py

@@ -45,6 +45,7 @@
 # Read in requirements
 requirements = open('requirements.txt').readlines()
 requirements = [r.strip() for r in requirements]
+requirements += [f'cirq-core=={__version__}']
 
 cirq_packages = ['cirq_aqt'] + [
     'cirq_aqt.' + package for package in find_packages(where='cirq_aqt')

cirq-ionq/requirements.txt

@@ -0,0 +1 @@
+requests~=2.18

cirq-ionq/setup.py

@@ -51,6 +51,8 @@
 # Sanity check
 assert __version__, 'Version string cannot be empty'
 
+requirements += [f'cirq-core=={__version__}']
+
 setup(
     name=name,
     version=__version__,

cirq-pasqal/requirements.txt

@@ -0,0 +1 @@
+requests~=2.18

cirq-pasqal/setup.py

@@ -43,14 +43,15 @@
 # Read in requirements
 requirements = open('requirements.txt').readlines()
 requirements = [r.strip() for r in requirements]
+# Sanity check
+assert __version__, 'Version string cannot be empty'
+
+requirements += [f'cirq-core=={__version__}']
 
 cirq_packages = ['cirq_pasqal'] + [
     'cirq_pasqal.' + package for package in find_packages(where='cirq_pasqal')
 ]
 
-# Sanity check
-assert __version__, 'Version string cannot be empty'
-
 setup(
     name=name,
     version=__version__,

cirq-rigetti/setup.py

@@ -44,12 +44,15 @@
 requirements = open('requirements.txt').readlines()
 requirements = [r.strip() for r in requirements]
 
+# Sanity check
+assert __version__, 'Version string cannot be empty'
+
+requirements += [f'cirq-core=={__version__}']
+
 cirq_packages = ['cirq_rigetti'] + [
     'cirq_rigetti.' + package for package in find_packages(where='cirq_rigetti')
 ]
 
-# Sanity check
-assert __version__, 'Version string cannot be empty'
 
 setup(
     name=name,

cirq-web/setup.py

@@ -46,7 +46,6 @@
 # Sanity check
 assert __version__, 'Version string cannot be empty'
 
-# This is a pure metapackage that installs all our packages
 requirements += [f'cirq-core=={__version__}']
 
 # Gather all packages from cirq_web, and the dist/ folder from cirq_ts

dev_tools/bash_scripts_test.py

@@ -16,17 +16,12 @@
 from typing import TYPE_CHECKING, Iterable
 
 from dev_tools import shell_tools
+from dev_tools.test_utils import only_on_posix
 
 if TYPE_CHECKING:
     import _pytest.tmpdir
 
 
-def only_on_posix(func):
-    if os.name != 'posix':
-        return None
-    return func
-
-
 def run(
     *,
     script_file: str,

dev_tools/cloned_env_test.py

@@ -0,0 +1,42 @@
+# Copyright 2020 The Cirq Developers
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Tests the cloned_env fixture in conftest.py"""
+import json
+import os
+from unittest import mock
+
+import pytest
+
+from dev_tools import shell_tools
+from dev_tools.test_utils import only_on_posix
+
+
+# due to shell_tools dependencies windows builds break on this
+# see https://github.com/quantumlib/Cirq/issues/4394
+@only_on_posix
+# ensure that no cirq packages are on the PYTHONPATH, this is important, otherwise
+# the "isolation" fails and all the cirq modules would be in the list
+@mock.patch.dict(os.environ, {"PYTHONPATH": ""})
+@pytest.mark.parametrize('param', ['a', 'b', 'c'])
+def test_isolated_env_cloning(cloned_env, param):
+    env = cloned_env("test_isolated", "flynt==0.64")
+    assert (env / "bin" / "pip").is_file()
+
+    result = shell_tools.run_cmd(
+        *f"{env}/bin/pip list --format=json".split(), out=shell_tools.TeeCapture()
+    )
+    packages = json.loads(result.out)
+    assert {"name": "flynt", "version": "0.64"} in packages
+    assert {"astor", "flynt", "pip", "setuptools", "wheel"} == set(p['name'] for p in packages)

dev_tools/conftest.py

@@ -11,8 +11,19 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import os
+import shutil
+import sys
+import tempfile
+import uuid
+from pathlib import Path
+from typing import Tuple
 
 import pytest
+from filelock import FileLock
+
+from dev_tools import shell_tools
+from dev_tools.env_tools import create_virtual_env
 
 
 def pytest_configure(config):
@@ -29,3 +40,85 @@ def pytest_collection_modifyitems(config, items):
     for item in items:
         if 'slow' in item.keywords:
             item.add_marker(skip_slow_marker)
+
+
+@pytest.fixture(scope="session")
+def cloned_env(testrun_uid, worker_id):
+    """Fixture to allow tests to run in a clean virtual env.
+
+    It de-duplicates installation of base packages. Assuming `virtualenv-clone` exists on the PATH,
+    it creates first a prototype environment and then clones for each new request the same env.
+    This fixture is safe to use with parallel execution, i.e. pytest-xdist. The workers synchronize
+    via a file lock, the first worker will (re)create the prototype environment, the others will
+    reuse it via cloning.
+
+    A group of tests that share the same base environment is identified by a name, `env_dir`,
+    which will become the directory within the temporary directory to hold the virtualenv.
+
+    Usage:
+
+    >>> def test_something_in_clean_env(cloned_env):
+            # base_env will point to a pathlib.Path containing the virtual env which will
+            # have quimb, jinja and whatever reqs.txt contained.
+            base_env = cloned_env("some_tests", "quimb", "jinja", "-r", "reqs.txt")
+
+            # To install new packages (that are potentially different for each test instance)
+            # just run pip install from the virtual env
+            subprocess.run(f"{base_env}/bin/pip install something".split(" "))
+            ...
+
+    Returns:
+        a function to create the cloned base environment with signature
+        `def base_env_creator(env_dir: str, *pip_install_args: str) -> Path`.
+        Use `env_dir` to specify the directory name per shared base packages.
+        Use `pip_install_args` varargs to pass arguments to `pip install`, these
+        can be requirements files, e.g. `'-r','dev_tools/.../something.txt'` or
+        actual packages as well, e.g.`'quimb'`.
+    """
+    base_dir = None
+
+    def base_env_creator(env_dir_name: str, *pip_install_args: str) -> Path:
+        """The function to create a cloned base environment."""
+        # get/create a temp directory shared by all workers
+        base_temp_path = Path(tempfile.gettempdir()) / "cirq-pytest"
+        os.makedirs(name=base_temp_path, exist_ok=True)
+        nonlocal base_dir
+        base_dir = base_temp_path / env_dir_name
+        with FileLock(str(base_dir) + ".lock"):
+            if _check_for_reuse_or_recreate(base_dir):
+                print(f"Pytest worker [{worker_id}] is reusing {base_dir} for '{env_dir_name}'.")
+            else:
+                print(f"Pytest worker [{worker_id}] is creating {base_dir} for '{env_dir_name}'.")
+                _create_base_env(base_dir, pip_install_args)
+
+        clone_dir = base_temp_path / str(uuid.uuid4())
+        shell_tools.run_cmd("virtualenv-clone", str(base_dir), str(clone_dir))
+        return clone_dir
+
+    def _check_for_reuse_or_recreate(env_dir: Path):
+        reuse = False
+        if env_dir.is_dir() and (env_dir / "testrun.uid").is_file():
+            uid = open(env_dir / "testrun.uid").readlines()[0]
+            # if the dir is from this test session, let's reuse it
+            if uid == testrun_uid:
+                reuse = True
+            else:
+                # if we have a dir from a previous test session, recreate it
+                shutil.rmtree(env_dir)
+        return reuse
+
+    def _create_base_env(base_dir: Path, pip_install_args: Tuple[str, ...]):
+        try:
+            create_virtual_env(str(base_dir), [], sys.executable, True)
+            with open(base_dir / "testrun.uid", mode="w") as f:
+                f.write(testrun_uid)
+            if pip_install_args:
+                shell_tools.run_cmd(f"{base_dir}/bin/pip", "install", *pip_install_args)
+        except BaseException as ex:
+            # cleanup on failure
+            if base_dir.is_dir():
+                print(f"Removing {base_dir}, due to error: {ex}")
+                shutil.rmtree(base_dir)
+            raise
+
+    return base_env_creator

dev_tools/modules.py

@@ -55,6 +55,7 @@ class Module:
     version: str = dataclasses.field(init=False)
     top_level_packages: List[str] = dataclasses.field(init=False)
     top_level_package_paths: List[Path] = dataclasses.field(init=False)
+    install_requires: List[str] = dataclasses.field(init=False)
 
     def __post_init__(self) -> None:
         self.name = self.raw_setup['name']
@@ -64,6 +65,9 @@ def __post_init__(self) -> None:
             self.top_level_packages = []
         self.top_level_package_paths = [self.root / p for p in self.top_level_packages]
         self.version = self.raw_setup['version']
+        self.install_requires = (
+            [] if 'install_requires' not in self.raw_setup else self.raw_setup['install_requires']
+        )
 
 
 def list_modules(

dev_tools/modules_test.py

@@ -46,6 +46,7 @@ def test_modules():
     assert mod1.version == '0.12.0.dev'
     assert mod1.top_level_packages == ['pack1']
     assert mod1.top_level_package_paths == [Path('mod1') / 'pack1']
+    assert mod1.install_requires == ['req1', 'req2']
 
     mod2 = Module(
         root=Path('mod2'), raw_setup={'name': 'module2', 'version': '1.2.3', 'packages': ['pack2']}
@@ -55,6 +56,7 @@ def test_modules():
     assert mod2.version == '1.2.3'
     assert mod2.top_level_packages == ['pack2']
     assert mod2.top_level_package_paths == [Path('mod2') / 'pack2']
+    assert mod2.install_requires == []
     assert modules.list_modules(search_dir=Path("dev_tools/modules_test_data")) == [mod1, mod2]
 
     parent = Module(