-
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
Open
MalteEbner
wants to merge
19
commits into
main
Choose a base branch
from
malte-lig-9225-raise-an-error-when-connecting-to-a-db-with-a-wrong-version
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
181cdf2
first draft of warning on wrong db version
MalteEbner 473acc0
Improvements to logic
MalteEbner d00abcb
more fixes
MalteEbner 3f4518c
update changelog
MalteEbner f3a73d7
easier tests
MalteEbner 5de10f5
shorter tests
MalteEbner 4074852
Merge branch 'main' into malte-lig-9225-raise-an-error-when-connectin…
MalteEbner 000f6e7
Bump version to 0.4.13
horatiualmasan 80c0ac7
update _EXCLUDED_TABLES_COUNT
MalteEbner 014047c
Update table_coverage_utils.py
MalteEbner c5c5250
Merge remote-tracking branch 'origin/release-v0.4.13' into malte-lig-…
MalteEbner ca6af51
Merge branch 'main' into malte-lig-9225-raise-an-error-when-connectin…
MalteEbner c35d727
Update CHANGELOG.md
MalteEbner 04473f3
Update CHANGELOG.md
MalteEbner a92f09f
Merge branch 'main' into malte-lig-9225-raise-an-error-when-connectin…
MalteEbner fefc950
Merge branch 'main' into malte-lig-9225-raise-an-error-when-connectin…
MalteEbner d808d62
Rework _validate_or_initialize_database_version
MalteEbner f4b80b1
Move validate_or_initialize_database_version to separate file
MalteEbner 8470e42
Merge branch 'main' into malte-lig-9225-raise-an-error-when-connectin…
MalteEbner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
13 changes: 13 additions & 0 deletions
13
lightly_studio/src/lightly_studio/models/database_version.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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.
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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,123 @@ def test_get_backend( | |
|
|
||
| assert db_manager.get_backend() == DatabaseBackend.DUCKDB | ||
| db_manager.close() | ||
|
|
||
|
|
||
| def _create_database(db_url: str, version: str | None = PACKAGE_VERSION) -> 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 _assert_database_version_after_connect( | ||
| db_url: str, | ||
| *, | ||
| caplog: pytest.LogCaptureFixture, | ||
| must_exist: bool, | ||
| expected_version: str | None, | ||
| expected_warning: str | None = None, | ||
| ) -> None: | ||
| caplog.clear() | ||
| with caplog.at_level("WARNING"): | ||
| db_manager.connect(db_url=db_url, must_exist=must_exist) | ||
| with db_manager.session() as session: | ||
| db_version = session.exec(select(DatabaseVersionTable)).first() | ||
| if expected_version is None: | ||
| assert db_version is None | ||
| else: | ||
| assert db_version is not None | ||
| assert db_version.version == expected_version | ||
| db_manager.close() | ||
| schema_warnings = [ | ||
| record.getMessage() | ||
| for record in caplog.records | ||
| if "database schema version" in record.getMessage().lower() | ||
| ] | ||
| if expected_warning is None: | ||
| assert not schema_warnings | ||
| else: | ||
| assert any(expected_warning in message for message in schema_warnings) | ||
|
|
||
|
|
||
| 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'}" | ||
| _assert_database_version_after_connect( | ||
| db_url=db_url, | ||
| caplog=caplog, | ||
| must_exist=False, | ||
| expected_version=PACKAGE_VERSION, | ||
| ) | ||
|
|
||
|
|
||
| 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) | ||
| _assert_database_version_after_connect( | ||
| db_url=db_url, | ||
| caplog=caplog, | ||
| must_exist=True, | ||
| expected_version=None, | ||
| expected_warning="missing version metadata", | ||
| ) | ||
|
|
||
|
|
||
| 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) | ||
| _assert_database_version_after_connect( | ||
| db_url=db_url, | ||
| caplog=caplog, | ||
| must_exist=True, | ||
| expected_version=PACKAGE_VERSION, | ||
| ) | ||
|
|
||
|
|
||
| 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") | ||
| _assert_database_version_after_connect( | ||
| db_url=db_url, | ||
| caplog=caplog, | ||
| must_exist=True, | ||
| expected_version="0.0.0", | ||
| expected_warning="got '0.0.0'", | ||
| ) | ||
|
|
||
|
|
||
| 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) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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; | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.