From d3a4db7c1d32731753a0f48eea8a8603ab1f0a09 Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Mon, 14 Sep 2026 08:32:47 +0200 Subject: [PATCH 1/3] Enforce the import layering with import-linter The package had no check on which module may import which, so the layering existed only by convention. .importlinter writes it down: configuration and data on top, then matching, comparison, the skill tables, plotting, model and network, obs, timeseries, and the leaf modules at the bottom. Four modules reached the root package for a name it re-exports (from . import Quantity), which makes every import an edge to the whole package and hides the real dependency. They now import from the defining module. timeseries._align imported Observation only for an annotation, so it moves under TYPE_CHECKING. Three imports stay ignored, each with its reason in the config. The one that is a real violation is plot.scatter(skill_table=True), which builds a Comparer to compute its table and so reaches up from plotting into matching. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/full_test.yml | 3 +++ .importlinter | 27 ++++++++++++++++++++++++ justfile | 8 +++++-- pyproject.toml | 1 + src/modelskill/comparison/_comparison.py | 2 +- src/modelskill/configuration.py | 4 +++- src/modelskill/matching.py | 2 +- src/modelskill/obs.py | 2 +- src/modelskill/timeseries/_align.py | 6 ++++-- 9 files changed, 47 insertions(+), 8 deletions(-) create mode 100644 .importlinter 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/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( From 5100c2a5612ed5b3f66a3702de3c2f7231e3b5cb Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Mon, 14 Sep 2026 08:40:04 +0200 Subject: [PATCH 2/3] Document the import layering in ADR-013 Includes the module dependency graph as a mermaid diagram, transitively reduced so the 44 edges read as 17. Co-Authored-By: Claude Opus 5 (1M context) --- adr/013-import-layering.md | 62 ++++++++++++++++++++++++++++++++++++++ adr/README.md | 1 + 2 files changed, 63 insertions(+) create mode 100644 adr/013-import-layering.md diff --git a/adr/013-import-layering.md b/adr/013-import-layering.md new file mode 100644 index 000000000..e5f8fd1f7 --- /dev/null +++ b/adr/013-import-layering.md @@ -0,0 +1,62 @@ +# 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 + 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 +``` + +Arrows point from importer to imported; 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 From d75957098b5f4bbb5130e75b9cdf73f1e6196e00 Mon Sep 17 00:00:00 2001 From: Henrik Andersson Date: Mon, 14 Sep 2026 08:44:07 +0200 Subject: [PATCH 3/3] Name the layers in the ADR-013 diagram Nine unlabelled rows of module names did not say what each layer is for. Co-Authored-By: Claude Opus 5 (1M context) --- adr/013-import-layering.md | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/adr/013-import-layering.md b/adr/013-import-layering.md index e5f8fd1f7..f9f76ad37 100644 --- a/adr/013-import-layering.md +++ b/adr/013-import-layering.md @@ -20,6 +20,41 @@ Modules may import downward and never upward. Modules sharing a layer may import ```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 @@ -39,7 +74,7 @@ flowchart TD plotting -. "scatter(skill_table=True)" .-> matching ``` -Arrows point from importer to imported; transitively implied edges are omitted. The dotted arrow is the one accepted violation. +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.