Repository navigation
refactor(architecture): four directories, four enforced fences - #373
Conversation
typeset/ was the one subpackage whose import rule was prose only, and it was already crossed: typeset/__init__.py reached api.load_schema, so rendering a model imported the executor, the plan and polars. Nothing failed, because no test read the rule. - load_schema moves to language/validation.py (api re-exports it). Parsing and validating a model is the language's own job; a consumer that binds no data must reach it without reaching a runner. - piecewise.py moves into language/. Its one tie to an engine was lowering.check_core_subset, whose tested payload is degree — so degree.check_expression states it where degree lives, and the error still names the link the user wrote rather than the _link0 it expands to. The lazy import goes with it: DELIBERATE_LAZY_IMPORTS is now empty. - validate_piecewise_data moves to sources.py, beside the data it needs. - two new fence tests: one on the names typeset imports, one on the transitive closure behind them, since a consumer's cost is what it drags in. Also corrects three claims in ARCHITECTURE.md that the code did not support, redraws the pipeline diagram as the four directories with their rules, and fixes chunking.py's docstring (two passes use it, not three). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change centralizes schema and expression validation in ChangesLanguage validation and architecture boundaries
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant API
participant load_schema
participant expand_piecewise
participant check_expression
API->>load_schema: schema path, mapping, or MathSchema
load_schema->>expand_piecewise: expand piecewise declarations
load_schema->>check_expression: validate expanded expressions
load_schema-->>API: original MathSchema
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lpspec/sources.py`:
- Line 98: Update validate_piecewise_data and _as_dataarray to replace Any-based
data-source annotations with xr.Dataset | xr.DataArray, and add the xarray
import as xr inside the existing TYPE_CHECKING block so no runtime dependency is
introduced. Preserve the current behavior while keeping pyrefly strict checking
free of errors.
- Line 128: Ensure the link ordering expected by validate_piecewise_data is
explicit and enforced: for convex piecewise blocks, require pw.links[0] to be
the x-breakpoint link and pw.links[1] to be the y-range link, rejecting reversed
declarations with a clear validation error. Update the PiecewiseBlock schema or
piecewise documentation to describe this ordering, while preserving non-convex
behavior.
- Around line 128-151: The convex validation block should validate pw.over
coordinates before broadcasting. After constructing xa and ya in the existing
try block, compare xa.indexes[pw.over] and ya.indexes[pw.over]; if they differ,
raise PiecewiseExpansionError identifying both mismatched coordinate sets, then
retain the existing xr.broadcast and curvature checks for matching coordinates.
In `@tests/test_architecture.py`:
- Around line 76-77: Move the convexity validation in sources.py from xarray to
the supported Polars-based dataframe path, preserving its validation behavior
and avoiding any direct xarray dependency. Then remove the sources.py entry from
LAZY_ORACLE_ALLOWED so only permitted xarray usage remains documented.
- Around line 203-281: Update the import-analysis helpers used by
test_typeset_reads_the_language_and_reaches_no_engine,
test_typesets_import_closure_needs_no_third_party_engine, and the lazy-import
checks to normalize absolute, relative, and package-level imports against each
importing file before enforcing fences. Reuse one normalized module-edge helper
across all three tests, including function-local imports, and add coverage
proving relative imports such as relational and package-level lowering cannot
bypass the engine boundary.
- Around line 244-281: The import-closure analysis misses third-party imports
inside module-level conditional blocks. Update _module_level_imports to traverse
ast.If bodies and orelse branches while excluding recognized TYPE_CHECKING
guards, so test_typesets_import_closure_needs_no_third_party_engine captures
imports that can execute during module import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1913d8f6-fb47-464e-b9c6-e5ae8c95e0f4
📒 Files selected for processing (13)
docs/ARCHITECTURE.mdsrc/lpspec/api.pysrc/lpspec/language/degree.pysrc/lpspec/language/piecewise.pysrc/lpspec/language/schema.pysrc/lpspec/language/validation.pysrc/lpspec/linopy/__init__.pysrc/lpspec/lowering.pysrc/lpspec/relational/chunking.pysrc/lpspec/sources.pysrc/lpspec/typeset/__init__.pytests/test_architecture.pytests/test_piecewise.py
| return sources | ||
|
|
||
|
|
||
| def validate_piecewise_data(schema: MathSchema, values: Mapping[str, Any] | Any) -> None: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Type the data-source parameter precisely instead of widening to Any.
values: Mapping[str, Any] | Any is equivalent to Any, since a union with Any absorbs the other member. _as_dataarray also returns Any. The file already has a TYPE_CHECKING block for Mapping; add a TYPE_CHECKING-guarded xr import there so both signatures can name xr.Dataset / xr.DataArray precisely without a runtime dependency.
As per coding guidelines, "Keep pyrefly strict checking at zero errors; fix types rather than widening them."
♻️ Proposed fix to narrow the types
if TYPE_CHECKING:
from collections.abc import Mapping
+ import xarray as xr
-def validate_piecewise_data(schema: MathSchema, values: Mapping[str, Any] | Any) -> None:
+def validate_piecewise_data(schema: MathSchema, values: Mapping[str, Any] | xr.Dataset) -> None:-def _as_dataarray(schema: MathSchema, pname: str, values: Mapping[str, Any] | Any) -> Any:
+def _as_dataarray(schema: MathSchema, pname: str, values: Mapping[str, Any] | xr.Dataset) -> xr.DataArray:Also applies to: 154-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lpspec/sources.py` at line 98, Update validate_piecewise_data and
_as_dataarray to replace Any-based data-source annotations with xr.Dataset |
xr.DataArray, and add the xarray import as xr inside the existing TYPE_CHECKING
block so no runtime dependency is introduced. Preserve the current behavior
while keeping pyrefly strict checking free of errors.
Source: Coding guidelines
| ) | ||
| raise ModuleNotFoundError(msg) from exc | ||
| ctx = f"piecewise '{name}'" | ||
| (x_link, y_link) = pw.links # convex requires exactly two links |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for documentation or validation of piecewise link order (x first, y second) for convex blocks.
rg -n 'convex' docs/SPEC.md -A 5 -B 5
rg -n 'x_link|y_link|links\[0\]|links\[1\]' --type=pyRepository: FBumann/lpspec
Length of output: 1271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked relevant files:"
git ls-files | rg '(^src/lpspec/sources\.py$|(^src/lpspec/.+\.py$)|(^docs/SPEC\.md$)|(^src/lpspec/schemas/py$)' || true
echo
echo "Relevant src/lpspec/sources.py area:"
sed -n '90,145p' src/lpspec/sources.py
echo
echo "Search for PiecewiseBlock and links schema/docs:"
rg -n 'class PiecewiseBlock|links|Piecewise|convex' src docs -g '*.py' -g '*.md' -C 3Repository: FBumann/lpspec
Length of output: 42425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
Path('src/lpspec/language/schema.py'),
Path('src/lpspec/language/piecewise.py'),
Path('src/lpspec/sources.py'),
Path('docs/SPEC.md'),
Path('docs/models/piecewise.md'),
Path('docs/models/transport_pwl.md'),
]
text = '\n'.join(f.read_text(errors='replace') for f in files)
terms = ['x-link', 'x link', 'x_link', 'y-link', 'y link', 'y_link', 'x-domain',
'y-range', 'first link', 'second link', 'link0', 'link1']
for term in terms:
if term in text:
print(f"FIND:{term}")
PY
sed -n '235,294p' src/lpspec/language/schema.py
sed -n '83,90p' src/lpspec/language/piecewise.pyRepository: FBumann/lpspec
Length of output: 3210
Document or enforce link ordering for convex piecewise blocks.
validate_piecewise_data() treats pw.links[0] as the x-breakpoint link and pw.links[1] as the y-range link, but PiecewiseBlock and the piecewise docs only describe links as interchangeable [expression, values_parameter] pairs. A user who declares the range parameter first gets curvature/monotonicity checks on the wrong axis. Add this invariant in the schema/docs, or enforce it with an error when convex: true.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lpspec/sources.py` at line 128, Ensure the link ordering expected by
validate_piecewise_data is explicit and enforced: for convex piecewise blocks,
require pw.links[0] to be the x-breakpoint link and pw.links[1] to be the
y-range link, rejecting reversed declarations with a clear validation error.
Update the PiecewiseBlock schema or piecewise documentation to describe this
ordering, while preserving non-convex behavior.
| (x_link, y_link) = pw.links # convex requires exactly two links | ||
| try: | ||
| xa = _as_dataarray(schema, x_link[1], values) | ||
| ya = _as_dataarray(schema, y_link[1], values) | ||
| except KeyError: | ||
| continue | ||
| xa, ya = xr.broadcast(xa, ya) | ||
| other = [d for d in xa.dims if d != pw.over] | ||
| stacked_x = xa.transpose(*other, pw.over).values.reshape(-1, xa.sizes[pw.over]) | ||
| stacked_y = ya.transpose(*other, pw.over).values.reshape(-1, ya.sizes[pw.over]) | ||
| for xs, ys in zip(stacked_x, stacked_y, strict=False): | ||
| dx = np.diff(xs) | ||
| if not (dx > 0).all(): | ||
| raise PiecewiseExpansionError( | ||
| f"{ctx}: convex: true requires strictly increasing breakpoints in '{x_link[1]}' (got {xs.tolist()})" | ||
| ) | ||
| curvature = np.diff(np.diff(ys) / dx) | ||
| tol = 1e-9 * max(1.0, float(np.abs(ys).max())) | ||
| if (curvature > tol).any() and (curvature < -tol).any(): | ||
| raise PiecewiseExpansionError( | ||
| f'{ctx}: convex: true is not exact for the mixed-curvature ' | ||
| f"curve in '{y_link[1]}' — the hull relaxation would silently " | ||
| f'cut corners; drop convex: true to use the exact MILP form' | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does xarray 2024.2.0 xr.broadcast default to an outer join with NaN fill for mismatched coordinates?
💡 Result:
No, xarray.broadcast does not default to an outer join with NaN filling for mismatched coordinates [1][2]. The xarray.broadcast function is primarily designed to insert new dimensions into input arrays so that they share a common shape and set of dimensions [1][2]. It does not perform coordinate alignment or reindexing in the manner of an outer join [1][2]. If your input arrays have mismatched coordinate labels, xr.broadcast will generally preserve the coordinates as they are, rather than aligning them to a union of labels [1][2]. In contrast, operations that do involve coordinate alignment in xarray—such as binary arithmetic (e.g., a + b)—default to an "inner" join [3][4]. You can change this behavior globally or within a context manager using xr.set_options(arithmetic_join="outer"), which then uses an outer join (filling mismatched coordinates with NaN) for those operations [3][4]. If you need to align coordinates explicitly before broadcasting, you should use the xr.align function, which allows you to specify a join type (e.g., join="outer") to handle mismatched indices [3][5].
Citations:
- 1: https://docs.xarray.dev/en/v2024.02.0/generated/xarray.broadcast.html
- 2: https://docs.xarray.dev/en/v2024.10.0/generated/xarray.broadcast.html
- 3: https://xarray.pydata.org/en/v2024.06.0/user-guide/computation.html
- 4: https://docs.xarray.dev/en/v2023.10.0/user-guide/computation.html
- 5: https://docs.xarray.dev/en/v2025.04.0/user-guide/computation.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate sources and project files =="
git ls-files | rg '(^|/)(src/lpspec/sources\.py|pyproject\.toml|poetry\.lock|uv\.lock|requirements|environment|tox\.ini|setup\.cfg)$' || true
echo
echo "== relevant sources.py excerpt =="
if [ -f src/lpspec/sources.py ]; then
nl -ba src/lpspec/sources.py | sed -n '1,220p'
fi
echo
echo "== xarray version constraints =="
rg -n 'xarray|xarray(' pyproject.toml poetry.lock uv.lock requirements* environment* tox.ini setup.cfg . 2>/dev/null | head -120 || trueRepository: FBumann/lpspec
Length of output: 304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant sources.py excerpt =="
if [ -f src/lpspec/sources.py ]; then
cat -n src/lpspec/sources.py | sed -n '1,220p'
fi
echo
echo "== xarray version constraints =="
cat pyproject.toml | sed -n '1,220p'
if grep -R 'xarray' pyproject.toml poetry.lock uv.lock requirements* environment* tox.ini setup.cfg . 2>/dev/null >/tmp/xarray_matches.txt; then
sed -n '1,160p' /tmp/xarray_matches.txt
fi
echo
echo "== Python xarray broadcast behavior probe =="
python3 - <<'PY'
try:
import sys, numpy as np, xarray as xr
print("xarray", xr.__version__)
print("numpy", np.__version__)
xa = xr.DataArray(np.arange(3), dims=["x"], coords={"x": [1, 2, 3]})
ya = xr.DataArray(np.arange(4), dims=["x"], coords={"x": [2, 3, 4, 5]}, name="y")
bx, by = xr.broadcast(xa, ya)
print("xa.values:", xa.values)
print("bx.values:", bx.values)
print("bx.coords:", dict(bx.coords))
print("by.values:", by.values)
print("by.coords:", dict(by.coords))
print("has mismatched x labels:", not xa.indexes["x"].equals(ya.indexes["x"]))
print("outer aligned values via align:", xr.align(xa, ya, join="outer"))
print("outer arithmetic join result:", (xa + by).values)
except RuntimeError as e:
printRuntimeError(e)
raise
except Exception as e:
print("dependency unavailable or other", repr(e))
PYRepository: FBumann/lpspec
Length of output: 18390
🌐 Web query:
xarray broadcast documentation broadcast variables to same dimensions outer index
💡 Result:
In xarray, the primary tool for expanding variables to share the same dimensions and shape is xarray.broadcast [1][2]. Key points regarding broadcasting in xarray: Functionality: The xarray.broadcast function takes any number of DataArray or Dataset objects and broadcasts them against each other [1][3]. It inserts additional dimensions where necessary so that all output objects have the same dimensions and shape [1][2]. Automatic Broadcasting: Xarray objects automatically broadcast against each other during arithmetic operations [1][4]. Consequently, explicit use of xarray.broadcast is often unnecessary for standard mathematical operations [1][5]. Alternatives: - xarray.DataArray.broadcast_like(other): This method broadcasts a DataArray against another object (DataArray or Dataset) [6]. It is functionally equivalent to calling xr.broadcast(other, self)[1] [6]. - xarray.align: If you need to ensure objects share the same dimension sizes and indexes (joining them by outer, inner, left, or right index), xarray.align is the appropriate function [7]. Unlike broadcast, which expands data to fill new dimensions, align focuses on aligning the labels of the existing indexes [7]. - xarray.NamedArray.broadcast_to: This is a lower-level method for internal or specific array structures that allows broadcasting to a new shape, provided no new dimensions are added [8][9]. Usage Example: To broadcast two DataArrays, a and b, use: a2, b2 = xr.broadcast(a, b) [1][4] To broadcast all variables within a single Dataset: (ds2,) = xr.broadcast(ds) [1][2]
Citations:
- 1: https://docs.xarray.dev/en/latest/generated/xarray.broadcast.html
- 2: https://xarray.pydata.org/en/v2024.10.0/generated/xarray.broadcast.html
- 3: https://github.com/pydata/xarray/blob/main/xarray/structure/alignment.py
- 4: https://docs.xarray.dev/en/v2026.01.0/generated/xarray.broadcast.html
- 5: https://docs.xarray.dev/en/v2024.06.0/generated/xarray.broadcast.html
- 6: https://docs.xarray.dev/en/stable/generated/xarray.DataArray.broadcast_like.html
- 7: https://docs.xarray.dev/en/v2025.01.0/generated/xarray.align.html
- 8: https://docs.xarray.dev/en/latest/generated/xarray.NamedArray.broadcast_to.html
- 9: [namedarray] split
.set_dims()into.expand_dims()andbroadcast_to()pydata/xarray#8380
🌐 Web query:
GitHub xarray broadcast align outer_index "outer_index" broadcast
💡 Result:
In Xarray, alignment and broadcasting are related but distinct concepts used to prepare objects for operations [1][2]. Broadcasting refers to the technique of expanding the dimensions or shapes of arrays so they can be combined despite initially having different shapes [1][2]. This happens automatically in Xarray based on dimension names [3][4]. Alignment refers to the process of ensuring that two or more objects have the same dimensions and coordinates, which is required for binary operations [1][2]. Xarray's xr.align() function is used to explicitly align objects, offering several join strategies (e.g., 'outer', 'inner', 'left', 'right') [5][6]. The term "outer_index" in your query relates to indexing mechanics rather than high-level broadcasting or alignment functions. In Xarray (and underlying NumPy indexing), an "outer indexer" (implemented via indexing.OuterIndexer) refers to applying indices orthogonally across dimensions [7][8]. When provided as a tuple of indices for different dimensions, Xarray treats them as "outer" products, resulting in an output array whose shape is the concatenation of the lengths of the provided indices [7]. This is distinct from "vectorized" indexing (or "inner" indexing), where indices are combined coordinate-wise to select specific elements [7]. In summary: - Broadcasting automatically expands dimensions by name to make operations possible [3][2]. - Alignment explicitly reconciles index labels across objects based on a specified join method (e.g., 'outer' join preserves all indices) [5][1]. - Outer indexing is a low-level indexing strategy for selecting subsets of data by applying indexers orthogonally [7]. If you are encountering issues combining objects, ensure your dimensions are named correctly [4]. For cases where you have a dimension of length 1 that you wish to broadcast across a larger dimension, calling .squeeze() to remove the dimension or explicitly aligning may be necessary [9][4].
Citations:
- 1: https://tutorial.xarray.dev/fundamentals/02.3_aligning_data_objects.html
- 2: https://docs.xarray.dev/en/latest/user-guide/terminology.html
- 3: https://docs.xarray.dev/en/stable/generated/xarray.broadcast.html
- 4: https://stackoverflow.com/questions/70161261/how-to-align-xarray-dataarray-with-length-1-dimension-with-a-larger-array
- 5: https://docs.xarray.dev/en/stable/generated/xarray.align.html
- 6: https://docs.xarray.dev/en/latest/generated/xarray.align.html
- 7: https://github.com/pydata/xarray/blob/50218d01/xarray/tests/test_indexing.py
- 8: https://github.com/google/xarray-tensorstore/blob/main/xarray_tensorstore_test.py
- 9: Support alignment/broadcasting with unlabeled dimensions of size 1 pydata/xarray#2171
Check pw.over coordinates before broadcasting.
xr.broadcast() expands shared dimensions, but it does not replace mismatched coordinate labels. If x_link and y_link have different pw.over labels, broadcasting can keep the mismatch and the check can pass/raise for the wrong reason instead of naming the alignment error.
Check that xa.indexes[pw.over] and ya.indexes[pw.over] are identical before xr.broadcast(xa, ya), and raise PiecewiseExpansionError with the mismatched coordinates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lpspec/sources.py` around lines 128 - 151, The convex validation block
should validate pw.over coordinates before broadcasting. After constructing xa
and ya in the existing try block, compare xa.indexes[pw.over] and
ya.indexes[pw.over]; if they differ, raise PiecewiseExpansionError identifying
both mismatched coordinate sets, then retain the existing xr.broadcast and
curvature checks for matching coordinates.
Source: Coding guidelines
| LAZY_ORACLE_ALLOWED = { | ||
| 'piecewise.py': 'convex curvature validation needs xarray broadcast (issue #27: make it numpy-only)', | ||
| 'sources.py': 'convex curvature validation needs xarray broadcast (issue #27: make it numpy-only)', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove the sources.py xarray exemption.
Line 77 permits sources.py to use xarray for internal convexity validation. This is not an optional bridge out of the Polars-based data model. It also conflicts with the documented rule that only linopy/ may import xarray.
Move this validation to the supported dataframe path, then remove sources.py from LAZY_ORACLE_ALLOWED. As per coding guidelines, “Use polars as the dataframe library; expose pandas and xarray only as optional bridges out.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_architecture.py` around lines 76 - 77, Move the convexity
validation in sources.py from xarray to the supported Polars-based dataframe
path, preserving its validation behavior and avoiding any direct xarray
dependency. Then remove the sources.py entry from LAZY_ORACLE_ALLOWED so only
permitted xarray usage remains documented.
Source: Coding guidelines
| def test_typeset_reads_the_language_and_reaches_no_engine(): | ||
| """The fourth fence — the one that was prose only. | ||
|
|
||
| ``typeset/`` is the proof that a consumer of the AST is cheap: it renders | ||
| any model the lanes can build, from one walk, holding no opinion they do | ||
| not already hold. That claim is only worth anything if it *cannot* reach | ||
| the plan, a sink, a solver or a dataframe — so ``import lpspec.typeset`` | ||
| must not drag in an engine. It used to, through ``api.load_schema``, and | ||
| nothing failed; the module map said otherwise and no test read it. | ||
|
|
||
| Membership is off the path, like the other three, so a new renderer cannot | ||
| land outside the fence by being spelled differently. | ||
| """ | ||
| offenders = {} | ||
| for path in (PKG / 'typeset').rglob('*.py'): | ||
| if '__pycache__' in path.parts: | ||
| continue | ||
| bad = [] | ||
| for node in ast.walk(ast.parse(path.read_text())): # lazy imports included — the rule is total | ||
| names = ( | ||
| [a.name for a in node.names] | ||
| if isinstance(node, ast.Import) | ||
| else [node.module] | ||
| if isinstance(node, ast.ImportFrom) and node.module | ||
| else [] | ||
| ) | ||
| bad += [ | ||
| n | ||
| for n in names | ||
| if n.startswith('lpspec') | ||
| and not n.startswith(('lpspec.language', 'lpspec.typeset')) | ||
| and n not in TYPESET_MAY_IMPORT | ||
| ] | ||
| if bad: | ||
| offenders[str(path.relative_to(PKG))] = sorted(set(bad)) | ||
| assert not offenders, ( | ||
| f'typeset reaches past the language: {offenders} — a renderer reads the AST ' | ||
| f'and writes text; it may not reach a plan, a sink, a solver or a dataframe' | ||
| ) | ||
|
|
||
|
|
||
| def test_typesets_import_closure_needs_no_third_party_engine(): | ||
| """The same fence, transitively — the half a one-hop name check misses. | ||
|
|
||
| A renderer that imports only ``language/`` still pays for polars if some | ||
| language module does. So this walks the *closure* of module-level imports | ||
| from ``typeset/`` and asserts no engine third-party lands in it: what a | ||
| consumer binding no data costs is the point of the fence, not which names | ||
| it happens to spell. | ||
|
|
||
| Deliberately not observed via ``import lpspec.typeset`` in a subprocess: | ||
| importing a submodule runs ``lpspec/__init__.py``, which eagerly exposes | ||
| ``build``/``solve`` and so loads the runner whatever this subpackage does. | ||
| That is a property of the top-level namespace, not of ``typeset/``. | ||
| """ | ||
| heavy = {'polars', 'highspy', 'numpy', 'pandas'} | FORBIDDEN_RUNTIME | ||
| by_module = { | ||
| f'lpspec.{p.relative_to(PKG).with_suffix("").as_posix().replace("/", ".").removesuffix(".__init__")}': p | ||
| for p in _all_modules() | ||
| } | ||
| seen, stack, reached = set(), ['lpspec.typeset'], {} | ||
| while stack: | ||
| mod = stack.pop() | ||
| if mod in seen or mod not in by_module: | ||
| continue | ||
| seen.add(mod) | ||
| for imported in _module_level_imports(by_module[mod]): | ||
| if imported in heavy: | ||
| reached.setdefault(imported, []).append(mod) | ||
| # module-level `from lpspec.x import y` edges, resolved to modules | ||
| for node in ast.walk(ast.parse(by_module[mod].read_text())): | ||
| if isinstance(node, ast.ImportFrom) and node.module and node.module.startswith('lpspec'): | ||
| stack.append(node.module) | ||
| elif isinstance(node, ast.Import): | ||
| stack += [a.name for a in node.names if a.name.startswith('lpspec')] | ||
| assert not reached, ( | ||
| f'the typeset import closure reaches an engine dependency: {reached} — ' | ||
| f'typesetting a model must not cost a dataframe library or a solver binding' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize all intra-package import forms before enforcing fences.
from ..relational import plan has node.module == "relational", and from .. import lowering has node.module is None. Lines 221-235 and 273-277 ignore both forms. Lines 422-429 also ignore function-local import lpspec.relational.plan.
A typeset/ module can therefore cross into the engine with a relative import, and an undeclared lazy cycle can use import lpspec... without failing either test. Resolve relative imports against the importing file, then use the same normalized module-edge helper for the direct-fence, closure, and lazy-import tests. Add coverage for these import forms.
Also applies to: 394-404
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_architecture.py` around lines 203 - 281, Update the
import-analysis helpers used by
test_typeset_reads_the_language_and_reaches_no_engine,
test_typesets_import_closure_needs_no_third_party_engine, and the lazy-import
checks to normalize absolute, relative, and package-level imports against each
importing file before enforcing fences. Reuse one normalized module-edge helper
across all three tests, including function-local imports, and add coverage
proving relative imports such as relational and package-level lowering cannot
bypass the engine boundary.
| def test_typesets_import_closure_needs_no_third_party_engine(): | ||
| """The same fence, transitively — the half a one-hop name check misses. | ||
|
|
||
| A renderer that imports only ``language/`` still pays for polars if some | ||
| language module does. So this walks the *closure* of module-level imports | ||
| from ``typeset/`` and asserts no engine third-party lands in it: what a | ||
| consumer binding no data costs is the point of the fence, not which names | ||
| it happens to spell. | ||
|
|
||
| Deliberately not observed via ``import lpspec.typeset`` in a subprocess: | ||
| importing a submodule runs ``lpspec/__init__.py``, which eagerly exposes | ||
| ``build``/``solve`` and so loads the runner whatever this subpackage does. | ||
| That is a property of the top-level namespace, not of ``typeset/``. | ||
| """ | ||
| heavy = {'polars', 'highspy', 'numpy', 'pandas'} | FORBIDDEN_RUNTIME | ||
| by_module = { | ||
| f'lpspec.{p.relative_to(PKG).with_suffix("").as_posix().replace("/", ".").removesuffix(".__init__")}': p | ||
| for p in _all_modules() | ||
| } | ||
| seen, stack, reached = set(), ['lpspec.typeset'], {} | ||
| while stack: | ||
| mod = stack.pop() | ||
| if mod in seen or mod not in by_module: | ||
| continue | ||
| seen.add(mod) | ||
| for imported in _module_level_imports(by_module[mod]): | ||
| if imported in heavy: | ||
| reached.setdefault(imported, []).append(mod) | ||
| # module-level `from lpspec.x import y` edges, resolved to modules | ||
| for node in ast.walk(ast.parse(by_module[mod].read_text())): | ||
| if isinstance(node, ast.ImportFrom) and node.module and node.module.startswith('lpspec'): | ||
| stack.append(node.module) | ||
| elif isinstance(node, ast.Import): | ||
| stack += [a.name for a in node.names if a.name.startswith('lpspec')] | ||
| assert not reached, ( | ||
| f'the typeset import closure reaches an engine dependency: {reached} — ' | ||
| f'typesetting a model must not cost a dataframe library or a solver binding' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include conditional module-level imports in the closure.
test_typesets_import_closure_needs_no_third_party_engine relies on _module_level_imports, but that helper descends into ast.Try only. A module-level if condition: import polars executes during import when the condition is true, but the closure omits it.
Traverse module-level ast.If blocks while excluding recognized TYPE_CHECKING guards. This keeps the closure check aligned with its import-cost contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_architecture.py` around lines 244 - 281, The import-closure
analysis misses third-party imports inside module-level conditional blocks.
Update _module_level_imports to traverse ast.If bodies and orelse branches while
excluding recognized TYPE_CHECKING guards, so
test_typesets_import_closure_needs_no_third_party_engine captures imports that
can execute during module import.
docs/ARCHITECTURE.mdclaims "Four subpackages, and the directory is the rule in every case". Three of the four had a test. The fourth —typeset/— had prose, and the prose was already false:So typesetting a model imported the plan, the executor and polars, against a map that said "a reader, not a lane: no model, no data, no plan" and a narrative that said "a consumer that needs no data needs no runner." Nothing failed, because nothing read the rule.
Closing that fence turned out to unblock three other things.
The fence
load_schema→language/validation.py. Parsing and validating a model is the language's own job; a consumer that binds no data has to reach it without reaching a runner.api.pyre-exports it, solps.load_schemais unchanged.typeset/imports, one on the transitive closure behind them — a renderer importing onlylanguage/still pays for polars if some language module does, and what a consumer drags in is the point of the fence, not what it spells.typeset/now importslanguage/anderrors.py. Nothing else.Deliberately not asserted by importing
lpspec.typesetin a subprocess: importing a submodule runslpspec/__init__.py, which eagerly exposesbuild/solveand loads the runner whatever this subpackage does. That is a property of the top-level namespace, and the test docstring says so rather than quietly measuring something weaker.piecewise.pymoves intolanguage/It sat flat on the grounds that it emitted declarations (language) but consulted
lowering.check_core_subsetto do it (plan). One call, four consequences: a SPEC-documented construct living outside the directory that owns the language, a formulation running in every lane enforcing what a plan node can represent,lowering.pyimporting it back lazily, andtypeset/reaching an engine through it.The call's tested payload is degree —
p ** 2andp * p. Everything elsecheck_core_subsetwould catch (dim rules, call shapes, unknown helpers) resolution anddimensions.pyalready answer; the plan-shape residue was never exercised. Sodegree.check_expressionstates it where degree lives — the existing per-nodecheck_binaryapplied over a whole expression, for a caller holding one rather than a descent to hang it on.Same sentences, and the error still names the link the user wrote rather than the
curve_link0the expansion generates, which is the whole reason the check was there.This is the same move that put degree in
language/degree.pyin the first place, one layer up — and the doc's own test for it ("a rule is language iff two consumers answering it separately would be a bug") now has a second worked example.DELIBERATE_LAZY_IMPORTSis empty. The cycle is gone rather than deferred, so hard rule 0 goes from "ordered, with exactly one declared exception" to "ordered".check_core_subsethad no other caller and is deleted.Also
validate_piecewise_data→sources.py, beside the data it needs. The xarray lazy import follows it rather than disappearing —LAZY_ORACLE_ALLOWEDstill has its one entry pending Streaming-subset completeness audit: every lowering rejection is now a product gap #27. Placement, not elimination.tests/test_architecture.pyenforces.language/was edge labels andtypeset/was absent; a reader could not see from either diagram that there are four fences. Verified throughmermaid-cli.chunking.py's docstring — it said three passes use it; two do, both viaModelTables, andlabels.pyhas no chunking at all.plan.pythe vocabulary,frames.py/status.pythe two boundaries,chunking.py/data_validation.pysingle rules lifted out.Verification
707 passed / 4 skipped / 1 xfailed ·
ruff checkclean ·pyrefly0 errors ·mkdocs build --strictclean.No behaviour change. The one user-visible difference is the wording of a
PiecewiseExpansionErroron a link expression outside the language: it no longer carries theis not a core-subset affine expressionwrapper, since the verdict is now degree's and degree states its own sentence. Both parametrised cases intest_a_link_outside_the_language_is_named_where_the_user_wrote_itstill pass unchanged.Not in scope
lpspec/__init__.pystill eagerly importsapi, soimport lpspeccosts the engine even forto_latex, andpython -m lpspec latexpays for a solver binding it never calls. Making that lazy (PEP 562__getattr__) would deliver the "no runner" promise at the CLI too — but it reintroduces a deliberate lazy import one commit after emptying that list, so it wants its own decision.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Refactor