Skip to content

Commit

Permalink
Dependencies: version check on sqlite C-language (#6567)
Browse files Browse the repository at this point in the history
Enforce the minimum supported version of `sqlite` in `C`-language.
Check the version of sqlite3 (in C) wherever relevant. Mainly when, creating or initializing a new profile with either `sqlite_zip` or `sqlite_dos` backend.

Therefore it also appears during `verdi status` (only if the default profile is with `sqlite` storage backend)

We decided that the check should not happen during AiiDA installation, matching `sqlite` versions could be painful, and we want to keep installation as smooth as possible. Moreover, not everyone needs to be bothered, as not everyone uses `sqlite` backend.
  • Loading branch information
khsrali authored Jan 23, 2025
1 parent aa0aa26 commit d0c9572
Show file tree
Hide file tree
Showing 7 changed files with 119 additions and 2 deletions.
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
26 changes: 26 additions & 0 deletions tests/cmdline/commands/test_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,32 @@ 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(run_cli_command, isolated_config, tmp_path, entry_point, monkeypatch):
"""Test the ``verdi profile setup`` command.
Same as `test_setup`, here we test the functionality to check sqlite versions, before setting up profiles.
"""

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 'Storage backend requires sqlite 100.0.0 or higher. But you have' in result.stderr
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 @@ -99,3 +99,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 sqlite version is incompatible with the required version.
the main functionality of this test is triggered only by the pytest marker 'presto',
through `pytest -m 'presto'`"""

profile = get_profile()
storage_backend = profile._attributes['storage']['backend']
if storage_backend in ['core.sqlite_dos', 'core.sqlite_zip']:
# 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

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 @@ -5,7 +5,8 @@
import pytest
from pydantic_core import ValidationError

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


Expand Down Expand Up @@ -63,3 +64,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()

1 comment on commit d0c9572

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'pytest-benchmarks:ubuntu-22.04,psql_dos'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: d0c9572 Previous: aa0aa26 Ratio
tests/benchmark/test_json_contains.py::test_deep_json[4-4] 290.44964807605857 iter/sec (stddev: 0.00036240) 711.1960639434309 iter/sec (stddev: 0.000079233) 2.45

This comment was automatically generated by workflow using github-action-benchmark.

CC: @giovannipizzi @agoscinski @GeigerJ2 @khsrali @unkcpz

Please sign in to comment.