diff --git a/.teamcity/Templates/RegressionTest.kt b/.teamcity/Templates/RegressionTest.kt index 11ddd9a1d..a5e3d759b 100644 --- a/.teamcity/Templates/RegressionTest.kt +++ b/.teamcity/Templates/RegressionTest.kt @@ -59,6 +59,7 @@ open class RegressionTest (platformOs: String) : Template() { scriptContent = header + """ pixi run python utils/get_benchmark.py %MiniO_credential_token% "benchmark/" + pixi run python utils/get_benchmark.py %MiniO_credential_token% "hws_2024_7_0/" pixi run test-ribasim-regression """.trimIndent() } diff --git a/pixi.lock b/pixi.lock index 8a8786005..82f201795 100644 --- a/pixi.lock +++ b/pixi.lock @@ -37546,41 +37546,7 @@ packages: name: ribasim version: 2024.10.0 path: python/ribasim - sha256: dabdd02a24d0b7d6a745c1bcdd800f8afdeb527b8b294ded4eaaf7205939a9cd - requires_dist: - - geopandas - - matplotlib - - numpy - - pandas - - pandera>=0.20 - - pyarrow - - pydantic~=2.0 - - pyogrio - - shapely>=2.0 - - tomli - - tomli-w - - jinja2 ; extra == 'all' - - networkx ; extra == 'all' - - pytest ; extra == 'all' - - pytest-cov ; extra == 'all' - - pytest-xdist ; extra == 'all' - - ribasim-testmodels ; extra == 'all' - - xugrid ; extra == 'all' - - jinja2 ; extra == 'delwaq' - - networkx ; extra == 'delwaq' - - xugrid ; extra == 'delwaq' - - xugrid ; extra == 'netcdf' - - pytest ; extra == 'tests' - - pytest-cov ; extra == 'tests' - - pytest-xdist ; extra == 'tests' - - ribasim-testmodels ; extra == 'tests' - requires_python: '>=3.10' - editable: true -- kind: pypi - name: ribasim - version: 2024.10.0 - path: python/ribasim - sha256: dabdd02a24d0b7d6a745c1bcdd800f8afdeb527b8b294ded4eaaf7205939a9cd + sha256: c48692687129085ad19256cbf54c8df9853f6259b31997cc4a282fde86072751 requires_dist: - geopandas - matplotlib @@ -37622,31 +37588,6 @@ packages: - ribasim-testmodels ; extra == 'tests' requires_python: '>=3.10' editable: true -- kind: pypi - name: ribasim-api - version: 2024.10.0 - path: python/ribasim_api - sha256: dc882869854e0940ab3a12506f5b9508b04045793f6badb644579c43fa2b6cb9 - requires_dist: - - xmipy - - pytest ; extra == 'tests' - - ribasim ; extra == 'tests' - - ribasim-testmodels ; extra == 'tests' - requires_python: '>=3.10' - editable: true -- kind: pypi - name: ribasim-testmodels - version: 0.5.0 - path: python/ribasim_testmodels - sha256: 2b0165819d6cc5d79b0b65761e8c38b1cc9f34649dea3ed4406013080ea8b112 - requires_dist: - - geopandas - - numpy - - pandas - - ribasim - - pytest ; extra == 'tests' - requires_python: '>=3.10' - editable: true - kind: pypi name: ribasim-testmodels version: 0.5.0 diff --git a/pixi.toml b/pixi.toml index 346715472..d1b970a68 100644 --- a/pixi.toml +++ b/pixi.toml @@ -14,8 +14,8 @@ repository = "https://github.com/Deltares/Ribasim" [tasks] # Tests -test-ribasim-python = "pytest --numprocesses=4 python/ribasim/tests" -test-ribasim-python-cov = "pytest --numprocesses=4 --cov=ribasim --cov-report=xml python/ribasim/tests" +test-ribasim-python = "pytest --numprocesses=4 -m 'not regression' python/ribasim/tests" +test-ribasim-python-cov = "pytest --numprocesses=4 --cov=ribasim --cov-report=xml -m 'not regression' python/ribasim/tests" test-ribasim-api = "pytest --basetemp=python/ribasim_api/tests/temp --junitxml=report.xml python/ribasim_api/tests" [feature.dev.tasks] @@ -85,11 +85,13 @@ test-ribasim-cli = "pytest --numprocesses=4 --basetemp=build/tests/temp --junitx test-ribasim-core = { cmd = "julia --project=core --eval 'using Pkg; Pkg.test()'", depends_on = [ "generate-testmodels", ] } +test-ribasim-migration = { cmd = "pytest --numprocesses=4 -m regression python/ribasim/tests" } test-ribasim-core-cov = { cmd = "julia --project=core --eval 'using Pkg; Pkg.test(coverage=true, julia_args=[\"--check-bounds=yes\"])'", depends_on = [ "generate-testmodels", ] } test-ribasim-regression = { cmd = "julia --project=core --eval 'using Pkg; Pkg.test(julia_args=[\"--check-bounds=yes\"], test_args=[\"regression\"])'", depends_on = [ "generate-testmodels", + "test-ribasim-migration", ] } generate-testmodels = { cmd = "python utils/generate-testmodels.py", inputs = [ "python/ribasim", diff --git a/python/ribasim/pyproject.toml b/python/ribasim/pyproject.toml index 290d7d2db..5fa7a47b2 100644 --- a/python/ribasim/pyproject.toml +++ b/python/ribasim/pyproject.toml @@ -45,3 +45,8 @@ path = "ribasim/__init__.py" [tool.hatch.build.targets.sdist] artifacts = ["delwaq/reference/*", "delwaq/template/*"] + +[tool.pytest.ini_options] +markers = [ + "regression: Older models that are not on the current database schema.", +] diff --git a/python/ribasim/ribasim/__init__.py b/python/ribasim/ribasim/__init__.py index a6d0010a3..0c709580e 100644 --- a/python/ribasim/ribasim/__init__.py +++ b/python/ribasim/ribasim/__init__.py @@ -1,5 +1,5 @@ __version__ = "2024.10.0" - +__schema_version__ = 1 from ribasim.config import Allocation, Logging, Node, Solver from ribasim.geometry.edge import EdgeTable diff --git a/python/ribasim/ribasim/db_utils.py b/python/ribasim/ribasim/db_utils.py new file mode 100644 index 000000000..e85e1e31e --- /dev/null +++ b/python/ribasim/ribasim/db_utils.py @@ -0,0 +1,65 @@ +from contextlib import closing +from pathlib import Path +from sqlite3 import Connection, connect + + +def esc_id(identifier: str) -> str: + """Escape SQLite identifiers.""" + identifier = identifier.replace('"', '""') + return f'"{identifier}"' + + +def exists(connection: Connection, name: str) -> bool: + """Check if a table exists in a SQLite database.""" + with closing(connection.cursor()) as cursor: + cursor.execute( + "SELECT name FROM sqlite_master WHERE type='table' AND name=?", (name,) + ) + result = cursor.fetchone() + return result is not None + + +def _set_gpkg_attribute_table(connection: Connection, table: str) -> None: + # Set geopackage attribute table + with closing(connection.cursor()) as cursor: + sql = "INSERT OR REPLACE INTO gpkg_contents (table_name, data_type, identifier) VALUES (?, ?, ?)" + cursor.execute(sql, (table, "attributes", table)) + connection.commit() + + +CREATE_TABLE_SQL = """ +CREATE TABLE IF NOT EXISTS ribasim_metadata ( + key TEXT PRIMARY KEY, + value TEXT +); +""" + + +def _get_db_schema_version(db_path: Path) -> int: + """ + Get the schema version of the database. + + For older models, the version is assumed to be zero, + which is smaller than the initial schema version of the database. + """ + with closing(connect(db_path)) as connection: + if not exists(connection, "ribasim_metadata"): + return 0 + with closing(connection.cursor()) as cursor: + cursor.execute( + "SELECT value FROM ribasim_metadata WHERE key='schema_version'" + ) + return int(cursor.fetchone()[0]) + + +def _set_db_schema_version(db_path: Path, version: int = 1) -> None: + with closing(connect(db_path)) as connection: + if not exists(connection, "metadata"): + with closing(connection.cursor()) as cursor: + cursor.execute(CREATE_TABLE_SQL) + cursor.execute( + "INSERT OR REPLACE INTO ribasim_metadata (key, value) VALUES ('schema_version', ?)", + (version,), + ) + _set_gpkg_attribute_table(connection, "ribasim_metadata") + connection.commit() diff --git a/python/ribasim/ribasim/input_base.py b/python/ribasim/ribasim/input_base.py index 010fe85e7..be3bc05ff 100644 --- a/python/ribasim/ribasim/input_base.py +++ b/python/ribasim/ribasim/input_base.py @@ -4,7 +4,7 @@ from contextlib import closing from contextvars import ContextVar from pathlib import Path -from sqlite3 import Connection, connect +from sqlite3 import connect from typing import ( Any, Generic, @@ -31,6 +31,12 @@ ) import ribasim +from ribasim.db_utils import ( + _get_db_schema_version, + _set_gpkg_attribute_table, + esc_id, + exists, +) from ribasim.schemas import _BaseSchema from .styles import _add_styles_to_geopackage @@ -67,21 +73,6 @@ TableT = TypeVar("TableT", bound=_BaseSchema) -def esc_id(identifier: str) -> str: - """Escape SQLite identifiers.""" - return '"' + identifier.replace('"', '""') + '"' - - -def exists(connection: Connection, name: str) -> bool: - """Check if a table exists in a SQLite database.""" - with closing(connection.cursor()) as cursor: - cursor.execute( - "SELECT name FROM sqlite_master WHERE type='table' AND name=?", (name,) - ) - result = cursor.fetchone() - return result is not None - - TABLES = ["profile", "state", "static", "time", "logic", "condition"] @@ -178,9 +169,15 @@ class TableModel(FileModel, Generic[TableT]): @field_validator("df") @classmethod - def _check_extra_columns(cls, v: DataFrame[TableT]): + def _check_schema(cls, v: DataFrame[TableT]): """Allow only extra columns with `meta_` prefix.""" if isinstance(v, (pd.DataFrame, gpd.GeoDataFrame)): + # On reading from geopackage, migrate the tables when necessary + db_path = context_file_loading.get().get("database") + if db_path is not None: + version = _get_db_schema_version(db_path) + if version < ribasim.__schema_version__: + v = cls.tableschema().migrate(v) for colname in v.columns: if colname not in cls.columns() and not colname.startswith("meta_"): raise ValueError( @@ -277,11 +274,8 @@ def _write_geopackage(self, temp_path: Path) -> None: dtype={"fid": "INTEGER PRIMARY KEY AUTOINCREMENT"}, ) + _set_gpkg_attribute_table(connection, table) # Set geopackage attribute table - with closing(connection.cursor()) as cursor: - sql = "INSERT INTO gpkg_contents (table_name, data_type, identifier) VALUES (?, ?, ?)" - cursor.execute(sql, (table, "attributes", table)) - connection.commit() def _write_arrow(self, filepath: Path, directory: Path, input_dir: Path) -> None: """Write the contents of the input to a an arrow file.""" diff --git a/python/ribasim/ribasim/migrations.py b/python/ribasim/ribasim/migrations.py new file mode 100644 index 000000000..c272f5841 --- /dev/null +++ b/python/ribasim/ribasim/migrations.py @@ -0,0 +1,71 @@ +import warnings + +from geopandas import GeoDataFrame +from pandas import DataFrame + + +def nodeschema_migration(gdf: GeoDataFrame) -> GeoDataFrame: + if "node_id" in gdf.columns: + warnings.warn("Migrating outdated Node table.", UserWarning) + assert gdf["node_id"].is_unique, "Node IDs have to be unique." + gdf.set_index("node_id", inplace=True) + + return gdf + + +def edgeschema_migration(gdf: GeoDataFrame) -> GeoDataFrame: + if "from_node_type" in gdf.columns: + warnings.warn("Migrating outdated Edge table.", UserWarning) + gdf.drop("from_node_type", inplace=True, axis=1) + if "to_node_type" in gdf.columns: + warnings.warn("Migrating outdated Edge table.", UserWarning) + gdf.drop("to_node_type", inplace=True, axis=1) + if "edge_id" in gdf.columns: + warnings.warn("Migrating outdated Edge table.", UserWarning) + gdf.set_index("edge_id", inplace=True) + + return gdf + + +def basinstaticschema_migration(df: DataFrame) -> DataFrame: + if "urban_runoff" in df.columns: + warnings.warn("Migrating outdated Basin / Static table.", UserWarning) + df.drop("urban_runoff", inplace=True, axis=1) + + return df + + +def basintimeschema_migration(df: DataFrame) -> DataFrame: + if "urban_runoff" in df.columns: + warnings.warn("Migrating outdated Basin / Time table.", UserWarning) + df.drop("urban_runoff", inplace=True, axis=1) + + return df + + +def continuouscontrolvariableschema_migration(df: DataFrame) -> DataFrame: + if "listen_node_type" in df.columns: + warnings.warn( + "Migrating outdated ContinuousControl / Variable table.", UserWarning + ) + df.drop("listen_node_type", inplace=True, axis=1) + + return df + + +def discretecontrolvariableschema_migration(df: DataFrame) -> DataFrame: + if "listen_node_type" in df.columns: + warnings.warn( + "Migrating outdated DiscreteControl / Variable table.", UserWarning + ) + df.drop("listen_node_type", inplace=True, axis=1) + + return df + + +def pidcontrolstaticschema_migration(df: DataFrame) -> DataFrame: + if "listen_node_type" in df.columns: + warnings.warn("Migrating outdated PidControl / Static table.", UserWarning) + df.drop("listen_node_type", inplace=True, axis=1) + + return df diff --git a/python/ribasim/ribasim/model.py b/python/ribasim/ribasim/model.py index 947738e80..83aab0e61 100644 --- a/python/ribasim/ribasim/model.py +++ b/python/ribasim/ribasim/model.py @@ -41,6 +41,7 @@ Terminal, UserDemand, ) +from ribasim.db_utils import _set_db_schema_version from ribasim.geometry.edge import EdgeSchema, EdgeTable from ribasim.geometry.node import NodeTable from ribasim.input_base import ( @@ -180,12 +181,16 @@ def _save(self, directory: DirectoryPath, input_dir: DirectoryPath): db_path.parent.mkdir(parents=True, exist_ok=True) db_path.unlink(missing_ok=True) context_file_writing.get()["database"] = db_path + self.edge._save(directory, input_dir) node = self.node_table() assert node.df is not None node._save(directory, input_dir) + # Run after geopackage schema has been created + _set_db_schema_version(db_path, ribasim.__schema_version__) + for sub in self._nodes(): sub._save(directory, input_dir) diff --git a/python/ribasim/ribasim/schemas.py b/python/ribasim/ribasim/schemas.py index c2b11c298..1fd8b4e98 100644 --- a/python/ribasim/ribasim/schemas.py +++ b/python/ribasim/ribasim/schemas.py @@ -1,9 +1,13 @@ # Automatically generated file. Do not modify. +from typing import Any, Callable + import pandera as pa from pandera.dtypes import Int32, Timestamp from pandera.typing import Index, Series +from ribasim import migrations + class _BaseSchema(pa.DataFrameModel): class Config: @@ -14,6 +18,13 @@ class Config: def _index_name(self) -> str: return "fid" + @classmethod + def migrate(cls, df: Any) -> Any: + f: Callable[[Any], Any] = getattr( + migrations, str(cls.__name__).lower() + "_migration", lambda x: x + ) + return f(df) + class BasinConcentrationExternalSchema(_BaseSchema): fid: Index[Int32] = pa.Field(default=1, check_name=True, coerce=True) diff --git a/python/ribasim/tests/test_migrations.py b/python/ribasim/tests/test_migrations.py new file mode 100644 index 000000000..69e1950ae --- /dev/null +++ b/python/ribasim/tests/test_migrations.py @@ -0,0 +1,21 @@ +from pathlib import Path + +import pytest +from ribasim import Model +from ribasim.db_utils import _get_db_schema_version + +root_folder = Path(__file__).parent.parent.parent.parent +print(root_folder) + + +@pytest.mark.regression +def test_hws_migration(): + toml_path = root_folder / "models/hws_2024_7_0/hws.toml" + db_path = root_folder / "models/hws_2024_7_0/database.gpkg" + + assert ( + toml_path.exists() + ), "Can't find the model, did you retrieve it with get_benchmark.py?" + + assert _get_db_schema_version(db_path) == 0 + Model.read(toml_path) diff --git a/python/ribasim/tests/test_schemas.py b/python/ribasim/tests/test_schemas.py index c48f5bacc..b070ceed2 100644 --- a/python/ribasim/tests/test_schemas.py +++ b/python/ribasim/tests/test_schemas.py @@ -1,5 +1,9 @@ +from unittest.mock import patch + import pytest from pydantic import ValidationError +from ribasim import Model +from ribasim.db_utils import _get_db_schema_version, _set_db_schema_version from ribasim.nodes import basin from ribasim.schemas import BasinProfileSchema from shapely.geometry import Point @@ -10,6 +14,31 @@ def test_config_inheritance(): assert BasinProfileSchema.__config__.coerce +@patch("ribasim.schemas.migrations.nodeschema_migration") +def test_migration(migration, basic, tmp_path): + toml_path = tmp_path / "basic.toml" + db_path = tmp_path / "database.gpkg" + basic.write(toml_path) + + # Migration is not needed on default model + Model.read(toml_path) + assert not migration.called + + # Fake old schema that needs migration + _set_db_schema_version(db_path, 0) + Model.read(toml_path) + assert migration.called + + +def test_model_schema(basic, tmp_path): + toml_path = tmp_path / "basic.toml" + db_path = tmp_path / "database.gpkg" + basic.write(toml_path) + assert _get_db_schema_version(db_path) == 1 + _set_db_schema_version(db_path, 2) + assert _get_db_schema_version(db_path) == 2 + + def test_geometry_validation(): with pytest.raises( ValidationError, diff --git a/ribasim_qgis/core/geopackage.py b/ribasim_qgis/core/geopackage.py index 67b8adda6..b9e7611b4 100644 --- a/ribasim_qgis/core/geopackage.py +++ b/ribasim_qgis/core/geopackage.py @@ -28,6 +28,7 @@ def sqlite3_cursor(path: Path): yield cursor finally: cursor.close() + connection.commit() connection.close() @@ -50,6 +51,25 @@ def layers(path: Path) -> list[str]: return layers +def write_schema_version(path: Path, version: int = 1) -> None: + """Write the schema version to the geopackage.""" + with sqlite3_cursor(path) as cursor: + cursor.execute( + """ + CREATE TABLE IF NOT EXISTS ribasim_metadata ( + key TEXT PRIMARY KEY, + value TEXT + ); + """ + ) + cursor.execute( + "INSERT OR REPLACE INTO ribasim_metadata (key, value) VALUES ('schema_version', ?)", + (version,), + ) + sql = "INSERT INTO gpkg_contents (table_name, data_type, identifier) VALUES (?, ?, ?)" + cursor.execute(sql, ("ribasim_metadata", "attributes", "ribasim_metadata")) + + def write_layer( path: Path, layer: QgsVectorLayer, diff --git a/ribasim_qgis/tests/ui/test_load_plugin.py b/ribasim_qgis/tests/ui/test_load_plugin.py index 6cef3bb33..325d7772e 100644 --- a/ribasim_qgis/tests/ui/test_load_plugin.py +++ b/ribasim_qgis/tests/ui/test_load_plugin.py @@ -1,6 +1,11 @@ +from pathlib import Path + +from qgis.core import QgsProject from qgis.testing import unittest from qgis.utils import iface, plugins +from ribasim_qgis.core.geopackage import sqlite3_cursor + class TestPlugin(unittest.TestCase): def test_plugin_is_loaded(self): @@ -8,7 +13,7 @@ def test_plugin_is_loaded(self): plugin = plugins.get("ribasim_qgis") self.assertTrue(plugin, "Ribasim plugin not loaded") - def test_load_dock(self): + def test_plugin(self): """Triggers Ribasim button and checks that Dock is added.""" # This checks the *actual* QGIS interface, not just a stub @@ -34,3 +39,31 @@ def test_load_dock(self): c for c in iface.mainWindow().children() if c.objectName() == "RibasimDock" ] self.assertTrue(len(docks) == 1, "Ribasim dock not activated") + + # Get the required widgets via the dock + ribadock = docks[0] + ribawidget = ribadock.widget() + datawidget = ribawidget.tabwidget.widget(0) + + # Write an empty model + datawidget._new_model("test.toml") + self.assertTrue(Path("test.toml").exists(), "test.toml not created") + self.assertTrue(Path("database.gpkg").exists(), "database.gpkg not created") + self.assertTrue( + len(QgsProject.instance().mapLayers()) == 2, + "Not just the Node and Edge layers", + ) + + # Check schema version + with sqlite3_cursor("database.gpkg") as cursor: + cursor.execute( + "SELECT value FROM ribasim_metadata WHERE key='schema_version'" + ) + self.assertTrue(int(cursor.fetchone()[0]) == 1, "schema_version is wrong") + + # Open the model + datawidget._open_model("test.toml") + self.assertTrue( + len(QgsProject.instance().mapLayers()) == 4, + "Not just the Node and Edge layers twice", + ) diff --git a/ribasim_qgis/widgets/dataset_widget.py b/ribasim_qgis/widgets/dataset_widget.py index b84656e16..906b99f06 100644 --- a/ribasim_qgis/widgets/dataset_widget.py +++ b/ribasim_qgis/widgets/dataset_widget.py @@ -36,6 +36,7 @@ QgsVectorLayer, ) +from ribasim_qgis.core.geopackage import write_schema_version from ribasim_qgis.core.model import ( get_database_path_from_model_file, get_directory_path_from_model_file, @@ -316,10 +317,13 @@ def filterbyrel(relationships, feature_ids): def new_model(self) -> None: """Create a new Ribasim model file, and set it as the active dataset.""" path, _ = QFileDialog.getSaveFileName(self, "Select file", "", "*.toml") + self._new_model(path) + + def _new_model(self, path: str): if path != "": # Empty string in case of cancel button press self.dataset_line_edit.setText(path) geo_path = self.path.with_name("database.gpkg") - self._write_new_model() + self._write_toml() for input_type in (Node, Edge): instance = input_type.create( @@ -328,10 +332,11 @@ def new_model(self) -> None: names=[], ) instance.write() + write_schema_version(geo_path) self.load_geopackage() self.ribasim_widget.toggle_node_buttons(True) - def _write_new_model(self) -> None: + def _write_toml(self) -> None: with open(self.path, "w") as f: f.writelines( [ @@ -348,6 +353,9 @@ def open_model(self) -> None: """Open a Ribasim model file.""" self.dataset_tree.clear() path, _ = QFileDialog.getOpenFileName(self, "Select file", "", "*.toml") + self._open_model(path) + + def _open_model(self, path: str) -> None: if path != "": # Empty string in case of cancel button press self.dataset_line_edit.setText(path) self.load_geopackage() diff --git a/utils/templates/schemas.py.jinja b/utils/templates/schemas.py.jinja index 487d476e8..efebba5cd 100644 --- a/utils/templates/schemas.py.jinja +++ b/utils/templates/schemas.py.jinja @@ -1,9 +1,12 @@ # Automatically generated file. Do not modify. +from typing import Any, Callable + import pandera as pa from pandera.dtypes import Int32, Timestamp from pandera.typing import Index, Series +from ribasim import migrations class _BaseSchema(pa.DataFrameModel): class Config: @@ -14,6 +17,12 @@ class _BaseSchema(pa.DataFrameModel): def _index_name(self) -> str: return "fid" + @classmethod + def migrate(cls, df: Any) -> Any: + f: Callable[[Any], Any] = getattr( + migrations, str(cls.__name__).lower() + "_migration", lambda x: x + ) + return f(df) {% for m in models %} class {{m[:name]}}Schema(_BaseSchema):