refactor(relational): one data-validation module for this lane - #355
Conversation
Answers #351 for the relational lane. Spec validation was already separate — `validation.py` is 298 lines with zero `DataError`, collecting into an error list rather than raising. The data half was mixed into the executor, which owns the model frames and the label assignment as well. Five checks move to `relational/data_validation.py`, as functions over frames and declarations holding no executor state: one row per coordinate, labels the dimension actually has, coordinates single-valued per label, coordinate containment. The executor keeps orchestration and drops 123 lines. `_check_no_undefined_divisor` stays where it is. It reads the assembled matrix, so it is genuinely coupled to the assembly rather than to the binding — one of the two positions where law 8 grants no default, and the reason the module docstring names them rather than pretending the split is total. **Scoped to this lane deliberately**, per @FBumann: some duplication for the eager lane is fine. These take tidy polars frames; the eager lane reads pandas/xarray natively because that is what linopy wants, and adapting it first would cost a copy of every parameter on the lane whose whole point is that the arrays are already in memory. What the lanes share instead is the wording (`errors.py`, three helpers now) and the contract — `tests/test_data_parity.py` asserts they reach the same verdict on the same bad data. That table is what keeps the duplication honest, and it is why this is a move rather than a rewrite. Hot path, so measured rather than assumed: `dispatch` at size l, alternating runs, minimum of three — 0.613 s against main's 0.603 s, inside the noise a single run showed at 0.95 s.
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Answers #351 for the relational lane, at the scope you asked for: one data-validation module here, some duplication for the eager lane accepted.
Spec validation was already separate
validation.pyis 298 lines with zeroDataError, and it collects into an error list rather than raising — so a model with several problems reports them together. Law 2, §9 and theLanguageError/DataErrorexception tree already name the boundary. Nothing needed moving on that side.The data half moves out of the executor
Five checks become functions over frames and declarations, holding no executor state (
grep -c 'self\.'→ 0):check_one_row_per_coordinate_unknown_label_probes/_raise_unknown_labelcheck_coordinates_single_valuedcheck_coordinate_containmentThe executor keeps orchestration and the model frames, and drops 123 lines (1192 → 1069).
_check_no_undefined_divisorstays. It reads the assembled matrix, so it is coupled to the assembly rather than to the binding — it is one of the two positions where law 8 grants no default, and the module docstring names that rather than pretending the split is total.Why this lane only
These take tidy polars frames. The eager lane reads pandas/xarray natively because that is what linopy wants, and adapting it to tidy frames first would cost a copy of every parameter on the lane whose whole point is that the arrays are already in memory.
What the lanes share instead:
errors.py, now three helpers (sparse_divisor_message,null_bounds_message,duplicate_coordinate_message)tests/test_data_parity.py(test(data): both lanes owe the same verdict on the same bad data #354) asserts both lanes reach the same verdict on the same bad dataThat table is what keeps the duplication honest, and it is why this is a move rather than a rewrite: the behaviour is pinned from outside, so the refactor cannot quietly change a verdict.
Measured, not assumed
This is the build hot path.
dispatchat sizel, alternating runs, minimum of three:origin/main1.7%, inside the noise a single run showed at 0.95 s. Function-call indirection at per-parameter granularity costs nothing measurable.
Verified
699 passed, 4 skipped, 1 xfailed.
ruff check,ruff format --check,pyrefly check(0 errors),mkdocs build --strict.docs/ARCHITECTURE.mdgains the module-map row —tests/test_architecture.pyfails on a module that neither the map nor a directory README documents, which is how I found it wanted one.🤖 Generated with Claude Code