diff --git a/.github/workflows/full_test.yml b/.github/workflows/full_test.yml index 341923a71..bf3eeb8d4 100644 --- a/.github/workflows/full_test.yml +++ b/.github/workflows/full_test.yml @@ -51,6 +51,9 @@ jobs: - name: Show pandas version run: uv run python -c "import pandas; print(f'pandas {pandas.__version__}')" + - name: Check import layering + run: just layers + - name: Type check run: just typecheck diff --git a/.importlinter b/.importlinter new file mode 100644 index 000000000..2cc063591 --- /dev/null +++ b/.importlinter @@ -0,0 +1,27 @@ +[importlinter] +root_package = modelskill +# Type-only imports do not couple the modules at runtime. +exclude_type_checking_imports = True + +[importlinter:contract:layers] +name = ModelSkill layers +type = layers +layers = + modelskill.configuration : modelskill.data + modelskill.matching + modelskill.comparison + modelskill.skill : modelskill.skill_grid : modelskill.skill_profile + modelskill.plotting + modelskill.model : modelskill.network + modelskill.obs + modelskill.timeseries + modelskill.metrics : modelskill.quantity : modelskill.settings : modelskill.types : modelskill.utils +ignore_imports = + # __version__ is defined in modelskill/__init__.py, so reading it pulls in + # the whole package. Moving it to its own module would remove these. + modelskill.comparison._comparison -> modelskill + modelskill.timeseries._timeseries -> modelskill + # plot.scatter(skill_table=True) builds a Comparer to compute the table, + # so plotting reaches up into matching. Deferred to a function body to keep + # it importable. This is the one real layering violation. + modelskill.plotting._scatter -> modelskill diff --git a/adr/013-import-layering.md b/adr/013-import-layering.md new file mode 100644 index 000000000..f9f76ad37 --- /dev/null +++ b/adr/013-import-layering.md @@ -0,0 +1,97 @@ +# ADR-013: Enforced Import Layering + +**Status**: Accepted + +**Date**: 2026-09 + +## Context + +ModelSkill's modules have an order to them — `timeseries` knows nothing about `comparison`, and `comparison` builds on `model` and `obs` — but nothing recorded or checked it. A module could import any other, and the only way to find out whether the structure still held was to read the imports. + +It had already started to give. Four modules imported a name from the root package (`from . import Quantity`) rather than from the module defining it. Since `modelskill/__init__.py` imports the whole package, each of those is an edge to everything, and an import cycle back through `__init__`: `obs` → `modelskill` → `obs`. It works only because the names happen to resolve in the order `__init__` runs. `plot.scatter(skill_table=True)` calls `from_matched` to build a `Comparer` for its table, a call from `plotting` up into `matching`, which had to be deferred into the function body or the import would fail outright. + +Both are the same failure: a dependency pointing the wrong way, worked around at the call site instead of being noticed. + +## Decision + +Write the layering down in `.importlinter` and check it with [import-linter](https://import-linter.readthedocs.io/) on every build (`just layers`, part of `just check`). + +Modules may import downward and never upward. Modules sharing a layer may import each other (`model` and `network` do). + +```mermaid +flowchart TD + subgraph L1["Entry points"] + configuration + data + end + subgraph L2["Matching"] + matching + end + subgraph L3["Comparison"] + comparison + end + subgraph L4["Skill tables"] + skill + skill_grid + skill_profile + end + subgraph L5["Plotting"] + plotting + end + subgraph L6["Model results"] + model + network + end + subgraph L7["Observations"] + obs + end + subgraph L8["Time series"] + timeseries + end + subgraph L9["Foundations"] + metrics + quantity + settings + types + utils + end + configuration --> matching + data --> comparison + matching --> comparison + comparison --> skill + comparison --> skill_grid + comparison --> skill_profile + skill --> plotting + plotting --> model + plotting --> metrics + model <--> network + model --> obs + obs --> timeseries + timeseries --> quantity + timeseries --> types + timeseries --> utils + metrics --> settings + plotting -. "scatter(skill_table=True)" .-> matching +``` + +Each box is a layer, named for what it contributes; modules inside a layer may import each other. Arrows point from importer to imported, and transitively implied edges are omitted. The dotted arrow is the one accepted violation. + +Three imports are ignored, each with its reason in the config. Two are reads of `__version__`, which lives in `__init__.py` and so pulls in the package. The third is the `plotting` → `matching` call above. + +## Alternatives Considered + +**Leave it to review** — the four root-package imports and the deferred `from_matched` all passed review. A reviewer sees one import, not what it does to the graph. + +**ruff's banned-api or a custom check** — ruff is per-file and has no import graph, so it cannot express "below `comparison`" or catch a violation that only exists as a chain through a third module. + +**Acyclic-siblings or independence contracts instead of layers** — these forbid cycles without saying which direction is correct. Layers state the intended shape, so a new module has somewhere to belong. + +**Fix `scatter(skill_table=True)` first** — worth doing, but it is a behaviour change to a public plotting argument. Recording it as a listed exception makes it visible now; the contract would otherwise have to wait on it. + +## Consequences + +- A new module has to be placed in a layer, which is the question worth asking when adding one. +- Type-only imports are excluded (`exclude_type_checking_imports`), so an annotation may point upward. `timeseries._align` imports `Observation` this way. +- The ignore list is the debt list. It is three lines; if it grows, the layering is wrong or the code is. +- `from . import X` inside the package is now a violation wherever it crosses a layer, which is the right default anyway — it names the re-export rather than the source. +- import-linter is a test-group dependency, next to mypy, since that is what CI installs. diff --git a/adr/README.md b/adr/README.md index 59b6d3bf2..5b316ae4a 100644 --- a/adr/README.md +++ b/adr/README.md @@ -31,6 +31,7 @@ Each ADR follows this structure: - [ADR-010](010-optional-domain-dependencies.md) - Optional dependencies for domain-specific model types (Draft) - [ADR-011](011-vertical-pre-extracted-columns.md) - VerticalModelResult ingests pre-extracted columns - [ADR-012](012-network-format-constructors.md) - One Network constructor per modelling product (Draft) +- [ADR-013](013-import-layering.md) - Enforced import layering ## Contributing diff --git a/justfile b/justfile index d027887a5..0e1e8f8cb 100644 --- a/justfile +++ b/justfile @@ -1,7 +1,7 @@ set windows-shell := ["powershell.exe", "-NoLogo", "-Command"] -# Run all checks: lint, typecheck, test, doctest -check: lint typecheck test doctest +# Run all checks: lint, layers, typecheck, test, doctest +check: lint layers typecheck test doctest # Build package (after typecheck and test) build: typecheck test @@ -19,6 +19,10 @@ format: test: uv run pytest --disable-warnings +# Check the import layering in .importlinter +layers: + uv run lint-imports + # Type check with mypy typecheck: uv run mypy src/ --config-file pyproject.toml diff --git a/pyproject.toml b/pyproject.toml index 890fc7574..e5398b42a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -58,6 +58,7 @@ test = [ "mypy==1.19.1", "types-PyYAML", "geopandas", + "import-linter", ] notebooks = ["nbformat", "nbconvert", "jupyter", "plotly", "shapely", "seaborn"] diff --git a/src/modelskill/comparison/_comparison.py b/src/modelskill/comparison/_comparison.py index 8838184e5..f74ee83f9 100644 --- a/src/modelskill/comparison/_comparison.py +++ b/src/modelskill/comparison/_comparison.py @@ -24,7 +24,7 @@ from .. import metrics as mtr -from .. import Quantity +from ..quantity import Quantity from ..types import GeometryType from ..obs import PointObservation, TrackObservation, NodeObservation from ..model import PointModelResult, TrackModelResult, VerticalModelResult diff --git a/src/modelskill/configuration.py b/src/modelskill/configuration.py index b4281e6d1..e51d2250e 100644 --- a/src/modelskill/configuration.py +++ b/src/modelskill/configuration.py @@ -4,7 +4,9 @@ import yaml from typing import Union -from . import model_result, match, Quantity +from .quantity import Quantity +from .model import model_result +from .matching import match from .obs import PointObservation, TrackObservation from .comparison import ComparerCollection diff --git a/src/modelskill/matching.py b/src/modelskill/matching.py index b7ac4b983..31991683d 100644 --- a/src/modelskill/matching.py +++ b/src/modelskill/matching.py @@ -21,7 +21,7 @@ from modelskill.model.point import PointModelResult -from . import Quantity +from .quantity import Quantity from .comparison import Comparer, ComparerCollection from .model.dfsu import DfsuModelResult from .model.dummy import DummyModelResult diff --git a/src/modelskill/obs.py b/src/modelskill/obs.py index fa2cc01ca..be06e258f 100644 --- a/src/modelskill/obs.py +++ b/src/modelskill/obs.py @@ -20,7 +20,7 @@ import xarray as xr from .types import PointType, TrackType, VerticalType, GeometryType, DataInputType -from . import Quantity +from .quantity import Quantity from .timeseries import ( TimeSeries, _parse_xyz_point_input, diff --git a/src/modelskill/timeseries/_align.py b/src/modelskill/timeseries/_align.py index 5e0b6b1b8..6595dbed6 100644 --- a/src/modelskill/timeseries/_align.py +++ b/src/modelskill/timeseries/_align.py @@ -4,8 +4,10 @@ import numpy as np import pandas as pd import xarray as xr -from typing import Any -from ..obs import Observation +from typing import Any, TYPE_CHECKING + +if TYPE_CHECKING: + from ..obs import Observation def _get_valid_times(