-
Notifications
You must be signed in to change notification settings - Fork 28
Log Warning if connecting to DB with older lightly-studio version #977
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
181cdf2
473acc0
d00abcb
3f4518c
f3a73d7
5de10f5
4074852
000f6e7
80c0ac7
014047c
c5c5250
ca6af51
c35d727
04473f3
a92f09f
fefc950
d808d62
f4b80b1
8470e42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,17 +18,19 @@ | |
| from collections.abc import Generator | ||
| from contextlib import contextmanager | ||
| from enum import Enum | ||
| from importlib import metadata | ||
| from pathlib import Path | ||
| from typing import Annotated | ||
|
|
||
| import sqlalchemy_utils | ||
| from fastapi import Depends | ||
| from sqlalchemy import StaticPool, text | ||
| from sqlalchemy import StaticPool, inspect, text | ||
| from sqlalchemy.engine import Engine | ||
| from sqlmodel import Session, SQLModel, create_engine | ||
| from sqlmodel import Session, SQLModel, create_engine, select | ||
|
|
||
| import lightly_studio.api.db_tables # noqa: F401, required for SQLModel to work properly | ||
| from lightly_studio.dataset.env import LIGHTLY_STUDIO_DATABASE_URL | ||
| from lightly_studio.models.database_version import DatabaseVersionTable | ||
|
|
||
|
|
||
| class DatabaseBackend(str, Enum): | ||
|
|
@@ -111,7 +113,7 @@ def __init__( | |
| SQLModel.metadata.drop_all(bind=self._engine) | ||
| logging.info("Dropped all tables in PostgreSQL database.") | ||
|
|
||
| SQLModel.metadata.create_all(self._engine) | ||
| _validate_or_initialize_database_version(engine=self._engine) | ||
|
|
||
| @contextmanager | ||
| def session(self) -> Generator[Session, None, None]: | ||
|
|
@@ -345,3 +347,43 @@ def _session_dependency() -> Generator[Session, None, None]: | |
|
|
||
|
|
||
| SessionDep = Annotated[Session, Depends(_session_dependency)] | ||
|
|
||
|
|
||
| def _validate_or_initialize_database_version(engine: Engine) -> None: | ||
| """Validate database schema version or initialize it for fresh databases. | ||
|
|
||
| The database schema version is tied to the full lightly-studio package version. | ||
| Only fresh databases get a version row written; existing databases with a | ||
| missing or mismatched version are warned about but never auto-updated, so | ||
| the warning persists until the user takes action. | ||
| """ | ||
| existing_managed_tables = set(inspect(engine).get_table_names()).intersection( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this really need to be handled? What happens if there is an existing database without the DatabaseVersionTable?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is needed to distinguish cases 1 and 2 here: |
||
| SQLModel.metadata.tables | ||
| ) | ||
| SQLModel.metadata.create_all(engine) | ||
| expected_version = metadata.version("lightly-studio") | ||
|
MalteEbner marked this conversation as resolved.
Outdated
|
||
|
|
||
| with Session(engine) as version_session: | ||
| db_versions = version_session.exec(select(DatabaseVersionTable)).all() | ||
| if len(db_versions) > 1: | ||
| raise RuntimeError( | ||
| f"Expected at most one row in 'database_version', got {len(db_versions)}." | ||
| ) | ||
|
|
||
| db_version = db_versions[0] if db_versions else None | ||
| if db_version is None: | ||
| if existing_managed_tables: | ||
| logging.warning( | ||
| f"Incompatible database schema version. Expected version " | ||
| f"'{expected_version}', but found missing version metadata." | ||
| ) | ||
| return | ||
| version_session.add(DatabaseVersionTable(version=expected_version)) | ||
| version_session.commit() | ||
|
MalteEbner marked this conversation as resolved.
Outdated
|
||
| return | ||
|
|
||
| if db_version.version != expected_version: | ||
| logging.warning( | ||
| f"Incompatible database schema version. Expected version " | ||
| f"'{expected_version}', got '{db_version.version}'." | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| """Model for tracking the LightlyStudio database schema version.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from sqlmodel import Field, SQLModel | ||
|
|
||
|
|
||
| class DatabaseVersionTable(SQLModel, table=True): | ||
| """Stores the schema version expected by the running LightlyStudio package.""" | ||
|
|
||
| __tablename__ = "database_version" | ||
|
|
||
| version: str = Field(primary_key=True) | ||
|
MalteEbner marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,11 +5,13 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| from importlib import metadata | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
| import sqlmodel | ||
| from pytest_mock import MockerFixture | ||
| from sqlmodel import select | ||
|
|
||
| from lightly_studio import ImageDataset, db_manager | ||
| from lightly_studio.core.dataset_query.image_sample_field import ImageSampleField | ||
|
|
@@ -20,12 +22,15 @@ | |
| _detect_backend_from_url, | ||
| ) | ||
| from lightly_studio.models.collection import CollectionTable | ||
| from lightly_studio.models.database_version import DatabaseVersionTable | ||
| from lightly_studio.resolvers import image_resolver | ||
| from tests.helpers_resolvers import ( | ||
| create_collection, | ||
| create_image, | ||
| ) | ||
|
|
||
| PACKAGE_VERSION = metadata.version("lightly-studio") | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def patch_engine_singleton(mocker: MockerFixture) -> None: | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved to lightly_studio/tests/conftest.py |
||
|
|
@@ -332,3 +337,102 @@ def test_get_backend( | |
|
|
||
| assert db_manager.get_backend() == DatabaseBackend.DUCKDB | ||
| db_manager.close() | ||
|
|
||
|
|
||
| def _create_database(db_url: str, version: str | None) -> None: | ||
| db_manager.connect(db_url=db_url) | ||
| with db_manager.session() as session: | ||
| db_version = session.exec(select(DatabaseVersionTable)).first() | ||
| assert db_version is not None | ||
| if version != db_version.version: | ||
| session.delete(db_version) | ||
| if version is not None: | ||
| session.add(DatabaseVersionTable(version=version)) | ||
| db_manager.close() | ||
|
|
||
|
|
||
| def test_connect__initializes_database_version_for_new_database( | ||
| tmp_path: Path, | ||
| caplog: pytest.LogCaptureFixture, | ||
| patch_engine_singleton: None, # noqa: ARG001 | ||
| ) -> None: | ||
| db_url = f"duckdb:///{tmp_path / 'no_db.db'}" | ||
| caplog.clear() | ||
| with caplog.at_level("WARNING"): | ||
| db_manager.connect(db_url=db_url, must_exist=False) | ||
| assert caplog.messages == [] | ||
| with db_manager.session() as session: | ||
| assert session.exec(select(DatabaseVersionTable.version)).first() == PACKAGE_VERSION | ||
| db_manager.close() | ||
|
|
||
|
|
||
| def test_connect__warns_for_database_without_version_metadata( | ||
| tmp_path: Path, | ||
| caplog: pytest.LogCaptureFixture, | ||
| patch_engine_singleton: None, # noqa: ARG001 | ||
| ) -> None: | ||
| db_url = f"duckdb:///{tmp_path / 'db_without_version.db'}" | ||
| _create_database(db_url=db_url, version=None) | ||
| expected_warning = ( | ||
| f"Incompatible database schema version. Expected version " | ||
| f"'{PACKAGE_VERSION}', but found missing version metadata." | ||
| ) | ||
| caplog.clear() | ||
| with caplog.at_level("WARNING"): | ||
| db_manager.connect(db_url=db_url, must_exist=True) | ||
| assert caplog.messages == [expected_warning] | ||
| with db_manager.session() as session: | ||
| assert session.exec(select(DatabaseVersionTable.version)).first() is None | ||
| db_manager.close() | ||
|
|
||
|
|
||
| def test_connect__does_not_warn_for_same_database_version( | ||
| tmp_path: Path, | ||
| caplog: pytest.LogCaptureFixture, | ||
| patch_engine_singleton: None, # noqa: ARG001 | ||
| ) -> None: | ||
| db_url = f"duckdb:///{tmp_path / 'db_with_same_version.db'}" | ||
| _create_database(db_url=db_url, version=PACKAGE_VERSION) | ||
| caplog.clear() | ||
| with caplog.at_level("WARNING"): | ||
| db_manager.connect(db_url=db_url, must_exist=True) | ||
| assert caplog.messages == [] | ||
| with db_manager.session() as session: | ||
| assert session.exec(select(DatabaseVersionTable.version)).first() == PACKAGE_VERSION | ||
| db_manager.close() | ||
|
|
||
|
|
||
| def test_connect__warns_for_other_database_version( | ||
| tmp_path: Path, | ||
| caplog: pytest.LogCaptureFixture, | ||
| patch_engine_singleton: None, # noqa: ARG001 | ||
| ) -> None: | ||
| db_url = f"duckdb:///{tmp_path / 'db_with_other_version.db'}" | ||
| _create_database(db_url=db_url, version="0.0.0") | ||
| expected_warning = ( | ||
| f"Incompatible database schema version. Expected version '{PACKAGE_VERSION}', got '0.0.0'." | ||
| ) | ||
| caplog.clear() | ||
| with caplog.at_level("WARNING"): | ||
| db_manager.connect(db_url=db_url, must_exist=True) | ||
| assert caplog.messages == [expected_warning] | ||
| with db_manager.session() as session: | ||
| assert session.exec(select(DatabaseVersionTable.version)).first() == "0.0.0" | ||
| db_manager.close() | ||
|
|
||
|
|
||
| def test_connect__raises_for_multiple_database_versions( | ||
| tmp_path: Path, | ||
| patch_engine_singleton: None, # noqa: ARG001 | ||
| ) -> None: | ||
| db_url = f"duckdb:///{tmp_path / 'db_with_multiple_versions.db'}" | ||
| _create_database(db_url=db_url, version=None) | ||
|
|
||
| db_manager.connect(db_url=db_url) | ||
| with db_manager.session() as session: | ||
| session.add(DatabaseVersionTable(version="0.0.0")) | ||
| session.add(DatabaseVersionTable(version="0.0.1")) | ||
| db_manager.close() | ||
|
|
||
| with pytest.raises(RuntimeError, match=r"Expected at most one row in 'database_version'"): | ||
| db_manager.connect(db_url=db_url, must_exist=True) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4120,6 +4120,8 @@ export interface operations { | |
| query?: never; | ||
| header?: never; | ||
| path: { | ||
| /** @description The ID of the collection */ | ||
| collection_id: string; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unrelated |
||
| tag_id: string; | ||
| }; | ||
| cookie?: never; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.