From 05d99322d69bda9af3c45f3acbc7089c2a0f9a20 Mon Sep 17 00:00:00 2001 From: Daniel Hollas Date: Wed, 17 Apr 2024 11:04:54 +0100 Subject: [PATCH 1/2] Set PIP_USER in base image (#437) Ensure that pip installs to ~/.local by default, instead of /opt/conda. Other minor changes: * Do not pin pip version and always upgrade to latest, as recommended by pip maintainers. f there are any breaking changes in the future, they should be caught by the integration tests. * Install appmode from PyPI * Cleanup custom logo setup * pytest: Make --variant a required parameter * Simplify aiidalab_exec fixture usage * Move test_pip_check to common tests --- docker-bake.hcl | 4 +++ stack/base/Dockerfile | 11 ++++--- stack/lab/Dockerfile | 23 +++++++-------- tests/conftest.py | 32 +++++++++++++++++++-- tests/test-base-with-services.py | 10 ++----- tests/test-base.py | 49 +++++++++++++++++++------------- tests/test-common.py | 8 ++++-- tests/test-full-stack.py | 9 ++---- tests/test-lab.py | 21 ++++++++++---- 9 files changed, 107 insertions(+), 60 deletions(-) diff --git a/docker-bake.hcl b/docker-bake.hcl index b9a204de..81fa26a7 100644 --- a/docker-bake.hcl +++ b/docker-bake.hcl @@ -83,6 +83,9 @@ target "base-with-services" { "PGSQL_VERSION" = "${PGSQL_VERSION}" } } +# PYTHON_MINOR_VERSION is a Python version string +# without the patch version (e.g. "3.9") +# Used to construct paths to Python site-packages folder. target "lab" { inherits = ["lab-meta"] context = "stack/lab" @@ -93,6 +96,7 @@ target "lab" { args = { "AIIDALAB_VERSION" = "${AIIDALAB_VERSION}" "AIIDALAB_HOME_VERSION" = "${AIIDALAB_HOME_VERSION}" + "PYTHON_MINOR_VERSION" = join(".", slice(split(".", "${PYTHON_VERSION}"), 0, 2)) } } target "full-stack" { diff --git a/stack/base/Dockerfile b/stack/base/Dockerfile index d0ee2b27..b74691aa 100644 --- a/stack/base/Dockerfile +++ b/stack/base/Dockerfile @@ -20,8 +20,7 @@ ARG AIIDA_VERSION # We pin aiida-core to the exact installed version, # to prevent accidental upgrade or downgrade, that might # induce DB migration or break shared dependencies of AiiDAlab Apps. -RUN echo "pip==23.3.1" > /opt/requirements.txt && \ - echo "aiida-core==${AIIDA_VERSION}" >> /opt/requirements.txt +RUN echo "aiida-core==${AIIDA_VERSION}" > /opt/requirements.txt # Install the shared requirements. RUN mamba install --yes \ @@ -32,11 +31,15 @@ RUN mamba install --yes \ fix-permissions "${CONDA_DIR}" && \ fix-permissions "/home/${NB_USER}" -# Pin shared requirements in the base environemnt. +# Pin shared requirements in the conda base environment. RUN cat /opt/requirements.txt | xargs -I{} conda config --system --add pinned_packages {} +# Upgrade pip to latest +RUN pip install --upgrade --no-cache-dir pip # Configure pip to use requirements file as constraints file. -ENV PIP_CONSTRAINT=/opt/requirements.txt +ENV PIP_CONSTRAINT /opt/requirements.txt +# Ensure that pip installs packages to ~/.local by default +ENV PIP_USER 1 # Enable verdi autocompletion. RUN mkdir -p "${CONDA_DIR}/etc/conda/activate.d" && \ diff --git a/stack/lab/Dockerfile b/stack/lab/Dockerfile index 9b36cc98..565c74e8 100644 --- a/stack/lab/Dockerfile +++ b/stack/lab/Dockerfile @@ -41,20 +41,20 @@ ARG AIIDALAB_HOME_VERSION RUN git clone https://github.com/aiidalab/aiidalab-home && \ cd aiidalab-home && \ git checkout v"${AIIDALAB_HOME_VERSION}" && \ - pip install --quiet --no-cache-dir "./" && \ + pip install --no-user --quiet --no-cache-dir "./" && \ fix-permissions "./" && \ fix-permissions "${CONDA_DIR}" && \ fix-permissions "/home/${NB_USER}" -# Install and enable appmode. -RUN git clone https://github.com/oschuett/appmode.git && \ - cd appmode && \ - git checkout v0.8.0 -COPY gears.svg ./appmode/appmode/static/gears.svg -RUN pip install ./appmode --no-cache-dir && \ - jupyter nbextension enable --py --sys-prefix appmode && \ +# Install and enable appmode, turning Jupyter notebooks to Apps +RUN pip install appmode==0.8.0 --no-cache-dir --no-user +# Enable appmode extension +RUN jupyter nbextension enable --py --sys-prefix appmode && \ jupyter serverextension enable --py --sys-prefix appmode +# Swap appmode icon for AiiDAlab gears icon, shown during app load +COPY --chown=${NB_UID}:${NB_GID} gears.svg ${CONDA_DIR}/share/jupyter/nbextensions/appmode/gears.svg + # Copy start-up scripts for AiiDAlab. COPY before-notebook.d/* /usr/local/bin/before-notebook.d/ @@ -107,8 +107,5 @@ ENV NOTEBOOK_ARGS \ "--TerminalManager.cull_interval=60" # Set up the logo of notebook interface -COPY --chown=${NB_UID}:${NB_GID} aiidalab-wide-logo.png /tmp/notebook-logo.png - -# The directory location of logo.png is in the `${CONDA_DIR}/lib/python3.9/site-packages/notebook/static/base/images/logo.png`, -# but the python version may change in the future, thus we use the wildcard to match the python version. -RUN mv /tmp/notebook-logo.png ${CONDA_DIR}/lib/python3*/site-packages/notebook/static/base/images/logo.png +ARG PYTHON_MINOR_VERSION +COPY --chown=${NB_UID}:${NB_GID} aiidalab-wide-logo.png ${CONDA_DIR}/lib/python${PYTHON_MINOR_VERSION}/site-packages/notebook/static/base/images/logo.png diff --git a/tests/conftest.py b/tests/conftest.py index 5524cc44..860167e1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,6 +6,8 @@ from requests.exceptions import ConnectionError +VARIANTS = ("base", "lab", "base-with-services", "full-stack") + def is_responsive(url): try: @@ -16,12 +18,20 @@ def is_responsive(url): return False +def variant_checker(value): + msg = f"Invalid image variant '{value}', must be one of: {VARIANTS}" + if value not in VARIANTS: + raise pytest.UsageError(msg) + return value + + def pytest_addoption(parser): parser.addoption( "--variant", action="store", - default="base", + required=True, help="Variant (image name) of the docker-compose file to use.", + type=variant_checker, ) @@ -54,14 +64,30 @@ def execute(command, user=None, **kwargs): command = f"exec -T --user={user} aiidalab {command}" else: command = f"exec -T aiidalab {command}" - return docker_compose.execute(command, **kwargs) + out = docker_compose.execute(command, **kwargs) + return out.decode() return execute @pytest.fixture def nb_user(aiidalab_exec): - return aiidalab_exec("bash -c 'echo \"${NB_USER}\"'").decode().strip() + return aiidalab_exec("bash -c 'echo \"${NB_USER}\"'").strip() + + +@pytest.fixture +def pip_install(aiidalab_exec, nb_user): + """Temporarily install package via pip""" + package = None + + def _pip_install(pkg, **args): + nonlocal package + package = pkg + return aiidalab_exec(f"pip install {pkg}", **args) + + yield _pip_install + if package: + aiidalab_exec(f"pip uninstall --yes {package}") @pytest.fixture(scope="session") diff --git a/tests/test-base-with-services.py b/tests/test-base-with-services.py index ddcfcea1..235fb712 100644 --- a/tests/test-base-with-services.py +++ b/tests/test-base-with-services.py @@ -5,11 +5,8 @@ def test_correct_pgsql_version_installed(aiidalab_exec, pgsql_version): - info = json.loads( - aiidalab_exec( - "mamba list -n aiida-core-services --json --full-name postgresql" - ).decode() - )[0] + cmd = "mamba list -n aiida-core-services --json --full-name postgresql" + info = json.loads(aiidalab_exec(cmd))[0] assert info["name"] == "postgresql" assert parse(info["version"]).major == parse(pgsql_version).major @@ -18,5 +15,4 @@ def test_rabbitmq_can_start(aiidalab_exec): """Test the rabbitmq-server can start, the output should be empty if the command is successful.""" output = aiidalab_exec("mamba run -n aiida-core-services rabbitmq-server -detached") - - assert output == b"" + assert output == "" diff --git a/tests/test-base.py b/tests/test-base.py index 3a7a4b56..25143233 100644 --- a/tests/test-base.py +++ b/tests/test-base.py @@ -5,50 +5,61 @@ from packaging.version import parse -@pytest.mark.parametrize("incompatible_version", ["2.3.0"]) -def test_prevent_pip_install_of_incompatible_aiida_version( - aiidalab_exec, nb_user, aiida_version, incompatible_version +@pytest.mark.parametrize("pkg_manager", ["pip", "mamba"]) +def test_prevent_installation_of_aiida( + aiidalab_exec, nb_user, aiida_version, pkg_manager ): - package_manager = "pip" + """aiida-core is pinned to the exact version in the container, + test that both pip and mamba refuse to install a different version""" + + incompatible_version = "2.3.0" assert parse(aiida_version) != parse(incompatible_version) + # Expected to succeed, although should be a no-op. - aiidalab_exec( - f"{package_manager} install aiida-core=={aiida_version}", user=nb_user - ) + aiidalab_exec(f"{pkg_manager} install aiida-core=={aiida_version}", user=nb_user) with pytest.raises(Exception): aiidalab_exec( - f"{package_manager} install aiida-core=={incompatible_version}", + f"{pkg_manager} install aiida-core=={incompatible_version}", user=nb_user, ) def test_correct_python_version_installed(aiidalab_exec, python_version): - info = json.loads(aiidalab_exec("mamba list --json --full-name python").decode())[0] + info = json.loads(aiidalab_exec("mamba list --json --full-name python"))[0] assert info["name"] == "python" assert parse(info["version"]) == parse(python_version) def test_create_conda_environment(aiidalab_exec, nb_user): - output = aiidalab_exec("conda create -y -n tmp", user=nb_user).decode().strip() + output = aiidalab_exec("conda create -y -n tmp", user=nb_user).strip() assert "conda activate tmp" in output # New conda environments should be created in ~/.conda/envs/ - output = aiidalab_exec("conda env list", user=nb_user).decode().strip() + output = aiidalab_exec("conda env list", user=nb_user).strip() assert f"/home/{nb_user}/.conda/envs/tmp" in output -def test_pip_check(aiidalab_exec): - aiidalab_exec("pip check") - - def test_correct_aiida_version_installed(aiidalab_exec, aiida_version): - info = json.loads( - aiidalab_exec("mamba list --json --full-name aiida-core").decode() - )[0] + cmd = "mamba list --json --full-name aiida-core" + info = json.loads(aiidalab_exec(cmd))[0] assert info["name"] == "aiida-core" assert parse(info["version"]) == parse(aiida_version) def test_path_local_pip(aiidalab_exec, nb_user): """test that the pip local bin path ~/.local/bin is added to PATH""" - output = aiidalab_exec("bash -c 'echo \"${PATH}\"'", user=nb_user).decode() + output = aiidalab_exec("bash -c 'echo \"${PATH}\"'", user=nb_user) assert f"/home/{nb_user}/.local/bin" in output + + +def test_pip_user_install(aiidalab_exec, pip_install, nb_user): + """Test that pip installs packages to ~/.local/ by default""" + import email + + # We use 'tuna' as an example of python-only package without dependencies + pkg = "tuna" + pip_install(pkg) + output = aiidalab_exec(f"pip show {pkg}") + + # `pip show` output is in the RFC-compliant email header format + msg = email.message_from_string(output) + assert msg.get("Location").startswith(f"/home/{nb_user}/.local/") diff --git a/tests/test-common.py b/tests/test-common.py index 01ac9f57..fad551ee 100644 --- a/tests/test-common.py +++ b/tests/test-common.py @@ -9,14 +9,18 @@ def test_notebook_service_available(notebook_service): def test_verdi_status(aiidalab_exec, nb_user): - output = aiidalab_exec("verdi status", user=nb_user).decode().strip() + output = aiidalab_exec("verdi status", user=nb_user).strip() assert "Connected to RabbitMQ" in output assert "Daemon is running" in output def test_ssh_agent_is_running(aiidalab_exec, nb_user): - output = aiidalab_exec("ps aux | grep ssh-agent", user=nb_user).decode().strip() + output = aiidalab_exec("ps aux | grep ssh-agent", user=nb_user).strip() assert "ssh-agent" in output # also check only one ssh-agent process is running assert len(output.splitlines()) == 1 + + +def test_pip_check(aiidalab_exec): + aiidalab_exec("pip check") diff --git a/tests/test-full-stack.py b/tests/test-full-stack.py index 55ad4a9c..bcd7ba6b 100644 --- a/tests/test-full-stack.py +++ b/tests/test-full-stack.py @@ -4,13 +4,10 @@ @pytest.fixture(scope="function") def generate_aiidalab_install_output(aiidalab_exec, nb_user): def _generate_aiidalab_install_output(package_name): - output = ( - aiidalab_exec(f"aiidalab install --yes --pre {package_name}", user=nb_user) - .decode() - .strip() - ) + cmd = f"aiidalab install --yes --pre {package_name}" + output = aiidalab_exec(cmd, user=nb_user).strip() - output += aiidalab_exec(f"pip check", user=nb_user).decode().strip() + output += aiidalab_exec("pip check", user=nb_user).strip() # Uninstall the package to make sure the test is repeatable app_name = package_name.split("@")[0] diff --git a/tests/test-lab.py b/tests/test-lab.py index df4775dc..9e350af5 100644 --- a/tests/test-lab.py +++ b/tests/test-lab.py @@ -4,21 +4,30 @@ def test_correct_aiidalab_version_installed(aiidalab_exec, aiidalab_version): - info = json.loads(aiidalab_exec("mamba list --json --full-name aiidalab").decode())[ - 0 - ] + cmd = "mamba list --json --full-name aiidalab" + info = json.loads(aiidalab_exec(cmd))[0] assert info["name"] == "aiidalab" assert parse(info["version"]) == parse(aiidalab_version) def test_correct_aiidalab_home_version_installed(aiidalab_exec, aiidalab_home_version): - info = json.loads( - aiidalab_exec("mamba list --json --full-name aiidalab-home").decode() - )[0] + cmd = "mamba list --json --full-name aiidalab-home" + info = json.loads(aiidalab_exec(cmd))[0] assert info["name"] == "aiidalab-home" assert parse(info["version"]) == parse(aiidalab_home_version) +def test_appmode_installed(aiidalab_exec): + """Test that appmode pip package is installed in correct location""" + import email + + output = aiidalab_exec("pip show appmode") + + # `pip show` output is in the RFC-compliant email header format + msg = email.message_from_string(output) + assert msg.get("Location").startswith("/opt/conda/lib/python") + + @pytest.mark.parametrize("incompatible_version", ["22.7.1"]) def test_prevent_pip_install_of_incompatible_aiidalab_version( aiidalab_exec, From 7aba343e83139460ca0de9433d27c81b1146e1ae Mon Sep 17 00:00:00 2001 From: Daniel Hollas Date: Wed, 17 Apr 2024 12:03:50 +0100 Subject: [PATCH 2/2] Autoskip tests based on image variant (#442) Currently, one has to manually pass in test files for tests for various image variants (targets). Here we automatically skip all tests that are not intended to pass for a given image. Note that there is a behavioural change: Previously, for e.g. lab variant, we would only run test-common.py and test-lab.py. Now we also test it against test-base.py, since all images should pass all the tests for the lower layers as well. full-stack should pass all tests. * Use ruff formatter and linter --- .../workflows/docker-build-test-upload.yml | 2 +- .pre-commit-config.yaml | 8 +++--- dodo.py | 17 ++++++------ ruff.toml | 25 ++++++++++++++++++ tests/conftest.py | 26 ++++++++++++------- tests/{test-base.py => test_base.py} | 3 ++- ...services.py => test_base_with_services.py} | 13 ++++++++++ tests/{test-common.py => test_common.py} | 0 ...{test-full-stack.py => test_full_stack.py} | 11 ++++++++ tests/{test-lab.py => test_lab.py} | 14 +++++++++- 10 files changed, 94 insertions(+), 25 deletions(-) create mode 100644 ruff.toml rename tests/{test-base.py => test_base.py} (99%) rename tests/{test-base-with-services.py => test_base_with_services.py} (69%) rename tests/{test-common.py => test_common.py} (100%) rename tests/{test-full-stack.py => test_full_stack.py} (87%) rename tests/{test-lab.py => test_lab.py} (84%) diff --git a/.github/workflows/docker-build-test-upload.yml b/.github/workflows/docker-build-test-upload.yml index bfa6ac78..0c59fe08 100644 --- a/.github/workflows/docker-build-test-upload.yml +++ b/.github/workflows/docker-build-test-upload.yml @@ -53,7 +53,7 @@ jobs: shell: bash - name: Run tests ✅ - run: VERSION=newly-build pytest -s tests/test-common.py tests/test-${{ inputs.image }}.py --variant ${{ inputs.image }} + run: VERSION=newly-build pytest -s --target ${{ inputs.image }} shell: bash - name: Save image as a tar for later use 💾 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 63ba5ed3..f665917b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -18,7 +18,9 @@ repos: - id: yamlfmt args: [--preserve-quotes] - - repo: https://github.com/psf/black - rev: 24.3.0 + - repo: https://github.com/astral-sh/ruff-pre-commit + rev: v0.3.5 hooks: - - id: black + - id: ruff-format + - id: ruff + args: [--fix, --exit-non-zero-on-fix, --show-fixes] diff --git a/dodo.py b/dodo.py index a75f779f..99d53d42 100644 --- a/dodo.py +++ b/dodo.py @@ -2,9 +2,8 @@ import platform from pathlib import Path -from doit.tools import title_with_actions - import docker +from doit.tools import title_with_actions from dunamai import Version _DOCKER_CLIENT = docker.from_env() @@ -74,13 +73,13 @@ def generate_version_override( Path("docker-bake.override.json").write_text( json.dumps( - dict( - VERSION=version, - REGISTRY=registry, - TARGETS=targets, - ORGANIZATION=organization, - PLATFORMS=platforms, - ) + { + "VERSION": version, + "REGISTRY": registry, + "TARGETS": targets, + "ORGANIZATION": organization, + "PLATFORMS": platforms, + } ) ) diff --git a/ruff.toml b/ruff.toml new file mode 100644 index 00000000..899dfb7c --- /dev/null +++ b/ruff.toml @@ -0,0 +1,25 @@ +line-length = 88 +show-fixes = true +target-version = "py38" + +[lint] +ignore = ["E501", "E402", "B904", "TRY003"] +select = [ + "A", # flake8-builtins + "ARG", # flake8-unused-arguments + "B", # flake8-bugbear + "C4", # flake8-comprehensions + "E", # pycodestyle + "F", # pyflakes + "I", # isort + "N", # pep8-naming + "PLE", # pylint error rules + "PLW", # pylint warning rules + "PLC", # pylint convention rules + "RUF", # ruff-specific rules + "TRY", # Tryceratops + "UP" # pyupgrade +] + +[lint.per-file-ignores] +"tests/*" = ["ARG001", "B017"] diff --git a/tests/conftest.py b/tests/conftest.py index 860167e1..6e9d9854 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,10 +3,9 @@ import pytest import requests - from requests.exceptions import ConnectionError -VARIANTS = ("base", "lab", "base-with-services", "full-stack") +TARGETS = ("base", "lab", "base-with-services", "full-stack") def is_responsive(url): @@ -18,27 +17,34 @@ def is_responsive(url): return False -def variant_checker(value): - msg = f"Invalid image variant '{value}', must be one of: {VARIANTS}" - if value not in VARIANTS: +def target_checker(value): + msg = f"Invalid image target '{value}', must be one of: {TARGETS}" + if value not in TARGETS: raise pytest.UsageError(msg) return value def pytest_addoption(parser): parser.addoption( - "--variant", + "--target", action="store", required=True, - help="Variant (image name) of the docker-compose file to use.", - type=variant_checker, + help="target (image name) of the docker-compose file to use.", + type=target_checker, ) +@pytest.fixture(scope="session") +def target(pytestconfig): + return pytestconfig.getoption("target") + + @pytest.fixture(scope="session") def docker_compose_file(pytestconfig): - variant = pytestconfig.getoption("variant") - return f"stack/docker-compose.{variant}.yml" + target = pytestconfig.getoption("target") + compose_file = f"stack/docker-compose.{target}.yml" + print(f"Using docker compose file {compose_file}") + return compose_file @pytest.fixture(scope="session") diff --git a/tests/test-base.py b/tests/test_base.py similarity index 99% rename from tests/test-base.py rename to tests/test_base.py index 25143233..a83b8e38 100644 --- a/tests/test-base.py +++ b/tests/test_base.py @@ -1,7 +1,8 @@ """This module contains tests for the base image, which are AiiDA and package management related tests.""" -import pytest import json + +import pytest from packaging.version import parse diff --git a/tests/test-base-with-services.py b/tests/test_base_with_services.py similarity index 69% rename from tests/test-base-with-services.py rename to tests/test_base_with_services.py index 235fb712..6dc16ca4 100644 --- a/tests/test-base-with-services.py +++ b/tests/test_base_with_services.py @@ -1,8 +1,21 @@ """Services related tests.""" import json + +import pytest from packaging.version import parse +# Tests in this file should pass for the following images +TESTED_TARGETS = ("base-with-services", "full-stack") + + +@pytest.fixture(autouse=True) +def skip_if_incompatible_target(target): + if target in TESTED_TARGETS: + yield + else: + pytest.skip() + def test_correct_pgsql_version_installed(aiidalab_exec, pgsql_version): cmd = "mamba list -n aiida-core-services --json --full-name postgresql" diff --git a/tests/test-common.py b/tests/test_common.py similarity index 100% rename from tests/test-common.py rename to tests/test_common.py diff --git a/tests/test-full-stack.py b/tests/test_full_stack.py similarity index 87% rename from tests/test-full-stack.py rename to tests/test_full_stack.py index bcd7ba6b..62939272 100644 --- a/tests/test-full-stack.py +++ b/tests/test_full_stack.py @@ -1,5 +1,16 @@ import pytest +# Tests in this file should pass for the following images +TESTED_TARGETS = "full-stack" + + +@pytest.fixture(autouse=True) +def skip_if_incompatible_target(target): + if target in TESTED_TARGETS: + yield + else: + pytest.skip() + @pytest.fixture(scope="function") def generate_aiidalab_install_output(aiidalab_exec, nb_user): diff --git a/tests/test-lab.py b/tests/test_lab.py similarity index 84% rename from tests/test-lab.py rename to tests/test_lab.py index 9e350af5..478b494b 100644 --- a/tests/test-lab.py +++ b/tests/test_lab.py @@ -1,7 +1,19 @@ -import pytest import json + +import pytest from packaging.version import parse +# Tests in this file should pass for the following images +TESTED_TARGETS = ("lab", "full-stack") + + +@pytest.fixture(autouse=True) +def skip_if_incompatible_target(target): + if target in TESTED_TARGETS: + yield + else: + pytest.skip() + def test_correct_aiidalab_version_installed(aiidalab_exec, aiidalab_version): cmd = "mamba list --json --full-name aiidalab"