Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependencies: version check on sqlite C-language #6567

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/aiida/cmdline/commands/cmd_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@
from aiida.cmdline.commands.cmd_verdi import verdi
from aiida.cmdline.params import options
from aiida.cmdline.utils import echo
from aiida.common.exceptions import CorruptStorage, IncompatibleStorageSchema, UnreachableStorage
from aiida.common.exceptions import (
CorruptStorage,
IncompatibleExternalDependencies,
IncompatibleStorageSchema,
UnreachableStorage,
)
from aiida.common.log import override_log_level
from aiida.common.warnings import warn_deprecation

Expand Down Expand Up @@ -96,6 +101,12 @@
storage_cls = profile.storage_cls
storage_head_version = storage_cls.version_head()
storage_backend = storage_cls(profile)

if storage_cls.__name__ in ['SqliteZipBackend', 'SqliteDosStorage']:
from aiida.storage.sqlite_zip.backend import validate_sqlite_version

Check warning on line 106 in src/aiida/cmdline/commands/cmd_status.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/cmdline/commands/cmd_status.py#L106

Added line #L106 was not covered by tests

validate_sqlite_version()

except UnreachableStorage as exc:
message = "Unable to connect to profile's storage."
print_status(ServiceStatus.DOWN, 'storage', message, exception=exc, print_traceback=print_traceback)
Expand All @@ -111,6 +122,10 @@
message = 'Storage is corrupted.'
print_status(ServiceStatus.DOWN, 'storage', message, exception=exc, print_traceback=print_traceback)
exit_code = ExitCode.CRITICAL
except IncompatibleExternalDependencies as exc:
message = "Storage backend version doesn't satisfy the requirements of the installed AiiDA version. "
print_status(ServiceStatus.DOWN, 'storage', message, exception=exc)
exit_code = ExitCode.CRITICAL

Check warning on line 128 in src/aiida/cmdline/commands/cmd_status.py

View check run for this annotation

Codecov / codecov/patch

src/aiida/cmdline/commands/cmd_status.py#L125-L128

Added lines #L125 - L128 were not covered by tests
except Exception as exc:
message = "Unable to instatiate profile's storage."
print_status(ServiceStatus.ERROR, 'storage', message, exception=exc, print_traceback=print_traceback)
Expand Down
7 changes: 7 additions & 0 deletions src/aiida/common/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,13 @@ class IncompatibleDatabaseSchema(ConfigurationError): # noqa: N818
"""


class IncompatibleExternalDependencies(ConfigurationError): # noqa: N818
"""Raised when incomptabale external depencies are found.

This could happen, when the dependency is not a python package and therefore not checked during installation.
"""


class IncompatibleStorageSchema(IncompatibleDatabaseSchema):
"""Raised when the storage schema is incompatible with that of the code."""

Expand Down
2 changes: 2 additions & 0 deletions src/aiida/storage/sqlite_dos/backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from aiida.storage.log import MIGRATE_LOGGER
from aiida.storage.psql_dos.models.settings import DbSetting
from aiida.storage.sqlite_zip import models, orm
from aiida.storage.sqlite_zip.backend import validate_sqlite_version
from aiida.storage.sqlite_zip.utils import create_sqla_engine

from ..migrations import TEMPLATE_INVALID_SCHEMA_VERSION
Expand Down Expand Up @@ -226,6 +227,7 @@ def filepath_database(self) -> Path:

@classmethod
def initialise(cls, profile: Profile, reset: bool = False) -> bool:
validate_sqlite_version()
filepath = Path(profile.storage_config['filepath'])

try:
Expand Down
19 changes: 18 additions & 1 deletion src/aiida/storage/sqlite_zip/backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from sqlalchemy.orm import Session

from aiida import __version__
from aiida.common.exceptions import ClosedStorage, CorruptStorage
from aiida.common.exceptions import ClosedStorage, CorruptStorage, IncompatibleExternalDependencies
from aiida.common.log import AIIDA_LOGGER
from aiida.manage import Profile
from aiida.orm.entities import EntityTypes
Expand All @@ -45,6 +45,21 @@
__all__ = ('SqliteZipBackend',)

LOGGER = AIIDA_LOGGER.getChild(__file__)
SUPPORTED_VERSION = '3.35.0' # minimum supported version of sqlite


def validate_sqlite_version():
import sqlite3

from packaging.version import parse

sqlite_installed_version = parse(sqlite3.sqlite_version)
if sqlite_installed_version < parse(SUPPORTED_VERSION):
message = (
f'Storage backend requires sqlite {parse(SUPPORTED_VERSION)} or higher.'
f' But you have {sqlite_installed_version} installed.'
)
raise IncompatibleExternalDependencies(message)


class SqliteZipBackend(StorageBackend):
Expand Down Expand Up @@ -89,6 +104,7 @@ def version_head(cls) -> str:
def create_profile(filepath: str | Path, options: dict | None = None) -> Profile:
"""Create a new profile instance for this backend, from the path to the zip file."""
profile_name = Path(filepath).name
validate_sqlite_version()
return Profile(
profile_name,
{
Expand All @@ -111,6 +127,7 @@ def initialise(cls, profile: 'Profile', reset: bool = False) -> bool:
tests having run.
:returns: ``True`` if the storage was initialised by the function call, ``False`` if it was already initialised.
"""
validate_sqlite_version()
from archive_path import ZipPath

filepath_archive = Path(profile.storage_config['filepath'])
Expand Down
29 changes: 29 additions & 0 deletions tests/cmdline/commands/test_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,35 @@ def test_delete_force(run_cli_command, mock_profiles, pg_test_cluster):
assert 'When the `-f/--force` flag is used either `--delete-data` or `--keep-data`' in result.output


@pytest.mark.parametrize('entry_point', ('core.sqlite_dos', 'core.sqlite_zip'))
def test_setup_with_validating_sqlite_version(
config_psql_dos, run_cli_command, isolated_config, tmp_path, entry_point, monkeypatch
):
"""Test the ``verdi profile setup`` command.
Same as `test_setup`, here we check the fucntionality to check sqlite versions, before setting up profiles.
Note that this test should be run before the `test_delete_storage` test.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just to know if profile setup is failing, then I won't check why test_delete_storage is failing.
(Only because test_delete_storage creates profiles, also with the sqlite backend.)
I've the habit of putting them in such order, maybe it doesn't matter that much...

I took the comment away, as this is not a most just easier to debug this way..

"""

if entry_point == 'core.sqlite_zip':
tmp_path = tmp_path / 'archive.aiida'
create_archive([], filename=tmp_path)

profile_name = 'temp-profile'
options = [entry_point, '-n', '--profile-name', profile_name, '--email', 'email@host', '--filepath', str(tmp_path)]

# Should raise if installed version is lower than the supported one.
monkeypatch.setattr('aiida.storage.sqlite_zip.backend.SUPPORTED_VERSION', '100.0.0')
result = run_cli_command(cmd_profile.profile_setup, options, use_subprocess=False, raises=True)
# assert f'Storage backend requires sqlite 100.0.0 or higher. But you have' in result.stderr
khsrali marked this conversation as resolved.
Show resolved Hide resolved
assert profile_name not in isolated_config.profile_names

# Should not raise if installed version is higher than the supported one.
monkeypatch.setattr('aiida.storage.sqlite_zip.backend.SUPPORTED_VERSION', '0.0.0')
result = run_cli_command(cmd_profile.profile_setup, options, use_subprocess=False)
assert profile_name in isolated_config.profile_names
assert f'Created new profile `{profile_name}`.' in result.output


@pytest.mark.parametrize('entry_point', ('core.sqlite_dos', 'core.sqlite_zip'))
def test_delete_storage(run_cli_command, isolated_config, tmp_path, entry_point):
"""Test the ``verdi profile delete`` command with the ``--delete-storage`` option."""
Expand Down
32 changes: 32 additions & 0 deletions tests/cmdline/commands/test_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,35 @@ def storage_cls(*args, **kwargs):
result = run_cli_command(cmd_status.verdi_status, raises=True, use_subprocess=False)
assert 'Storage is corrupted' in result.output
assert result.exit_code is ExitCode.CRITICAL


def test_sqlite_version(run_cli_command, monkeypatch):
"""Test `verdi status` when the storage is found to be corrupt (e.g. non-matching repository UUIDs)."""
khsrali marked this conversation as resolved.
Show resolved Hide resolved

profile = get_profile()
storage_backend = profile._attributes['storage']['backend']
# This should be True, only if `pytest -m 'presto'` is used.
# and that is essential to guarantee the funtionality of the code!
if storage_backend in ['core.sqlite_dos', 'core.sqlite_zip']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this ever be the case though? If you really want to test this behavior in verdi status, I think you should force creating a profile with the relevant storage backend and then test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this works as expected only in tests-presto. It's being triggered only via pytest -m 'presto'.

I've tried your suggestion to create a profile for each storage backend, but there is a strange problem there, which I don't understand.
Suppose I do:

@pytest.mark.parametrize('entry_point', ('core.sqlite_zip', 'core.sqlite_dos', 'core.psql_dos'))
def test_sqlite_version(run_cli_command, monkeypatch, entry_point, tmp_path, config_psql_dos):
    if entry_point == 'core.sqlite_zip':
        tmp_path = tmp_path / 'archive.aiida'
        create_archive([], filename=tmp_path)

    if entry_point == 'core.psql_dos':
        options = []
        for key, value in config_psql_dos().items():
            options.append(f'--{key.replace("_", "-")}')
            options.append(str(value))
    else:
        options = ['--filepath', str(tmp_path)]

    profile_name = f'temp-profile{entry_point}'
    options = [entry_point, '-n', '--profile-name', profile_name, '--email', 'email@host', '--set-as-default', *options]
    result = run_cli_command(cmd_profile.profile_setup, options, use_subprocess=True)
    assert f'Created new profile `{profile_name}`.' in result.output

    if entry_point in ['core.sqlite_dos', 'core.sqlite_zip']:
->        result = run_cli_command(cmd_status.verdi_status, use_subprocess=True, raises=True)

This last line returns wrong results for some other profile, even if I set-as-default explicitly:
AssertionError: ✔ version: AiiDA v2.6.2.post0
✔ config: /tmp/pytest-of-khosra_a/pytest-13/c048339d34c915b04f9e61ba1aa470df0/.aiida
✔ profile: f4366537365f007119dd90576b2ab04d
✔ storage: Storage for 'f4366537365f007119dd90576b2ab04d' [open] @ postgresql+psycopg://guest:***@localhost:54571/61eeacfc-85be-458a-93de-c198106f23c5 / DiskObjectStoreRepository: 43c774c5c42a44df98360169b9f08ea2 | /tmp/pytest-of-khosra_a/pytest-13/repository0/container
Warning: RabbitMQ v3.12.1 is not supported and will cause unexpected problems!
Warning: It can cause long-running workflows to crash and jobs to be submitted multiple times.
Warning: See https://github.com/aiidateam/aiida-core/wiki/RabbitMQ-version-to-use for details.
✔ broker: RabbitMQ v3.12.1 @ amqp://guest:[email protected]:5672?heartbeat=600
⏺ daemon: The daemon is not running.

While if you do this:

(Pdb) import os
(Pdb) os.system("verdi status")

returns the correct one:

✔ version: AiiDA v2.6.2.post0
✔ config: /tmp/pytest-of-khosra_a/pytest-10/fc80b65e071f67ef50d89cc715645faa0/.aiida
✔ profile: temp-profilecore.sqlite_dos
✔ storage: SqliteDosStorage[/tmp/pytest-of-khosra_a/pytest-10/test_sqlite_version_core_sqlit0]: open,
Warning: RabbitMQ v3.12.1 is not supported and will cause unexpected problems!
Warning: It can cause long-running workflows to crash and jobs to be submitted multiple times.
Warning: See https://github.com/aiidateam/aiida-core/wiki/RabbitMQ-version-to-use for details.
✔ broker: RabbitMQ v3.12.1 @ amqp://guest:[email protected]:5672?heartbeat=600
⏺ daemon: The daemon is not running.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure why it is not picking up the new profile. But anyway, this is not the way you probably want to go about this. You are now creating a profile in the test configuration. This can affect other tests. You probably want to create a new temporary isolated config, create the profile in that one, and just run the cli command in memory. See this test as an example of how to create the profile:

def test_create_profile(isolated_config, tmp_path, entry_point):

And then just run run_cli_command(use_subprocess=False). This should be a lot faster and should keep things nicely isolated.

# Should raise if installed version is lower than the supported one.
monkeypatch.setattr('aiida.storage.sqlite_zip.backend.SUPPORTED_VERSION', '100.0.0')
result = run_cli_command(cmd_status.verdi_status, use_subprocess=False, raises=True)
assert (
"Storage backend version doesn't satisfy the requirements of the installed AiiDA version" in result.output
)
assert (
'IncompatibleExternalDependencies: Storage backend requires sqlite 100.0.0 or higher. But you have'
in result.stderr
)

# Should not raise if installed version is higher than the supported one.
monkeypatch.setattr('aiida.storage.sqlite_zip.backend.SUPPORTED_VERSION', '0.0.0')
result = run_cli_command(cmd_status.verdi_status, use_subprocess=False)

else:
from unittest.mock import MagicMock

mock_ = MagicMock()
monkeypatch.setattr('aiida.storage.sqlite_zip.backend.validate_sqlite_version', mock_)
result = run_cli_command(cmd_status.verdi_status, use_subprocess=False)
assert mock_.call_count == 0
14 changes: 14 additions & 0 deletions tests/storage/sqlite_dos/test_backend.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Tests for :mod:`aiida.storage.sqlite_dos.backend`."""

import pathlib
from unittest.mock import MagicMock

import pytest
from aiida.storage.sqlite_dos.backend import FILENAME_CONTAINER, FILENAME_DATABASE, SqliteDosStorage
Expand Down Expand Up @@ -38,3 +39,16 @@ def test_backup(aiida_config, aiida_profile_factory, tmp_path, manager):
dirpath_backup = filepath_last.resolve()
assert (dirpath_backup / FILENAME_DATABASE).exists()
assert (dirpath_backup / FILENAME_CONTAINER).exists()


def test_initialise_version_check(tmp_path, monkeypatch):
"""Test :meth:`aiida.storage.sqlite_zip.backend.SqliteZipBackend.create_profile`
only if calls on validate_sqlite_version."""

mock_ = MagicMock()
monkeypatch.setattr('aiida.storage.sqlite_dos.backend.validate_sqlite_version', mock_)

# Here, we don't care about functionality of initialise itself, but only that it calls validate_sqlite_version.
with pytest.raises(AttributeError):
SqliteDosStorage.initialise('')
mock_.assert_called_once()
35 changes: 34 additions & 1 deletion tests/storage/sqlite_zip/test_backend.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
"""Tests for :mod:`aiida.storage.sqlite_zip.backend`."""

import pathlib
from unittest.mock import MagicMock

import pytest
from aiida.storage.sqlite_zip.backend import SqliteZipBackend
from aiida.common.exceptions import IncompatibleExternalDependencies
from aiida.storage.sqlite_zip.backend import SqliteZipBackend, validate_sqlite_version
from aiida.storage.sqlite_zip.migrator import validate_storage
from pydantic_core import ValidationError

Expand Down Expand Up @@ -62,3 +64,34 @@ def test_model():

model = SqliteZipBackend.Model(filepath=filepath.name)
assert pathlib.Path(model.filepath).is_absolute()


def test_validate_sqlite_version(monkeypatch):
"""Test :meth:`aiida.storage.sqlite_zip.backend.validate_sqlite_version`."""

# Test when sqlite version is not supported, should read sqlite version from sqlite3.sqlite_version
monkeypatch.setattr('sqlite3.sqlite_version', '0.0.0')
monkeypatch.setattr('aiida.storage.sqlite_zip.backend.SUPPORTED_VERSION', '100.0.0')
with pytest.raises(
IncompatibleExternalDependencies, match=r'.*Storage backend requires sqlite 100.0.0 or higher.*'
):
validate_sqlite_version()

# Test when sqlite version is supported
monkeypatch.setattr('sqlite3.sqlite_version', '100.0.0')
monkeypatch.setattr('aiida.storage.sqlite_zip.backend.SUPPORTED_VERSION', '0.0.0')
validate_sqlite_version()


def test_initialise_version_check(tmp_path, monkeypatch):
"""Test :meth:`aiida.storage.sqlite_zip.backend.SqliteZipBackend.create_profile`
only if calls on validate_sqlite_version."""

filepath_archive = tmp_path / 'archive.zip'

mock_ = MagicMock()
monkeypatch.setattr('aiida.storage.sqlite_zip.backend.validate_sqlite_version', mock_)
profile = SqliteZipBackend.create_profile(filepath_archive)
assert mock_.call_count == 1
SqliteZipBackend.initialise(profile)
assert mock_.call_count == 2
Loading