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 4 commits
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
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
6 changes: 6 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 All @@ -242,6 +244,10 @@ def initialise(cls, profile: Profile, reset: bool = False) -> bool:

return super().initialise(profile, reset)

def __init__(self, profile: Profile) -> None:
validate_sqlite_version()
super().__init__(profile)

def __str__(self) -> str:
state = 'closed' if self.is_closed else 'open'
return f'SqliteDosStorage[{self.filepath_root}]: {state},'
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 @@ -111,6 +126,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 Expand Up @@ -168,6 +184,7 @@ def migrate(cls, profile: Profile):
def __init__(self, profile: Profile):
from .migrator import validate_storage

validate_sqlite_version()
super().__init__(profile)
self._path = Path(profile.storage_config['filepath'])
validate_storage(self._path)
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 functionality 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
29 changes: 29 additions & 0 deletions tests/cmdline/commands/test_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,32 @@ 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 (
'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()
20 changes: 19 additions & 1 deletion tests/storage/sqlite_zip/test_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
import pathlib

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 +63,20 @@ 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()
Loading