Skip to content

refactor: one lazy import left, and it is the only real cycle - #110

Merged
FBumann merged 1 commit into
refactor/split-relational-enginefrom
refactor/untangle-language-layer
Jul 26, 2026
Merged

refactor: one lazy import left, and it is the only real cycle#110
FBumann merged 1 commit into
refactor/split-relational-enginefrom
refactor/untangle-language-layer

Conversation

@FBumann

@FBumann FBumann commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Stacked on #107. Retarget to main once that merges; the diff below is this PR's own.

The package had eight in-function linopy_yaml imports. One broke a genuine cycle. The other seven broke nothing — and made that one impossible to spot.

The one real cycle went through a private name

piecewise.py and lowering.py imported each other lazily, and piecewise reached for lowering._lower_expr, using it as a predicate and discarding the result:

def _expr_dims(schema, text, ctx):
    from linopy_yaml.lowering import _lower_expr   # private, in-function
    ...
    _lower_expr(resolved, schema, ctx)             # called for its exceptions only
    return dims_of(resolved, schema, ctx)

The dependency is real and its direction is right: a formulation sits above lowering, and it needs to know whether a link expression is in the streaming subset. So lowering now offers a public name for exactly that —

def check_core_subset(node, schema, context) -> None:
    """The subset test *is* the lowering — there is no second definition of
    what the engine accepts, which is what stops the two from drifting."""
    _lower_expr(node, schema, context)

— piecewise imports it at module level, and only the reverse edge stays lazy. It is now the sole entry in DELIBERATE_LAZY_IMPORTS, with the reason written down.

That guard was load-bearing, and untested

I checked whether it could just go (by removing the call, not by monkeypatching — the first attempt patched lowering._lower_expr globally, which also disabled lowering's own use and falsely reported the guard as dead).

With it removed, p ** 2 and p * p in a piecewise: link are still refused — by lowering, on the expanded declarations:

LanguageError: constraint 'cost_curve_link0': operator '**' is not in the language

cost_curve_link0 is a declaration the formulation generated. The user wrote cost_curve, link 0. The guard exists to keep the message pointing at the file. Two tests now pin that, so the next person to find this "redundant" call has the reason in front of them.

The other six were leftovers

resolution.py imported errors, where_parser and helpers in-function while already importing them at the top; dimensions.py and validation.py deferred imports of modules with no path back. All hoisted.

A new architecture test keeps it that way, and fails in both directions — an undeclared in-function import, or a declaration for an import that no longer exists:

DELIBERATE_LAZY_IMPORTS = {
    ('lowering.py', 'linopy_yaml.piecewise'): (
        'formulations expand before lowering, and expanding needs the subset '
        'test that lowering defines — piecewise imports lowering at module '
        'level, so this direction has to stay lazy'
    ),
}

ARCHITECTURE.md gains this as hard rule 0, since it is the mechanical form of "the layers are ordered".

It earned itself on the rebase. #104 landed while this was open and added a ninth in-function import — piecewise.pyrelational.arrow. The test failed on rebase and named it. It turned out to be hoistable (arrow.py imports pyarrow lazily itself, so module-level costs nothing), so that is a tenth import gone rather than a second declaration.

tidy_sourcessources.py

Rebased onto #104's Arrow rewrite of that function, so sources.py carries the new body, not the pandas one. It was in lowering.py, sharing no code and no concept with anything there: lowering turns an AST into a plan and touches no data; that function touches only data and knows nothing about expressions. They were in one file because api.build calls them on consecutive lines, which is not a reason.

Verification

uv run pytest — 311 passed, 1 xfailed (3 new) · ruff check and ruff format --check clean · pyrefly check 0 errors.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8118e0a0-5a71-43dc-ad36-821dd37e018b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/untangle-language-layer

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@FBumann
FBumann force-pushed the refactor/untangle-language-layer branch from e792234 to 5916635 Compare July 26, 2026 16:32
@FBumann
FBumann force-pushed the refactor/split-relational-engine branch from f0105b1 to 432b534 Compare July 26, 2026 16:45
The package had eight in-function `linopy_yaml` imports. One broke a genuine
cycle; the other seven broke nothing and made that one impossible to spot.

`piecewise.py` and `lowering.py` imported each other lazily, and piecewise
reached for a *private* name — `lowering._lower_expr` — using it as a
predicate and throwing the result away. The dependency is real and its
direction is right: a formulation sits above lowering, and it needs to know
whether a link expression is in the streaming subset. So lowering now offers
`check_core_subset()` for exactly that, piecewise imports it at module level,
and only the reverse edge stays lazy — documented, and now the sole entry in
`DELIBERATE_LAZY_IMPORTS`.

That guard turned out to be load-bearing and untested. Removing it, `p ** 2`
and `p * p` in a `piecewise:` link are still refused — but by lowering, on
the *expanded* declarations, so the message names `cost_curve_link0`, which
the user never wrote. The guard exists to keep the error pointing at the
block and link index in the file. Two tests now pin that.

The other six were leftovers: `resolution` imported `errors`, `where_parser`
and `helpers` in-function while already importing them at the top, and
`dimensions` and `validation` deferred imports of modules with no path back.
All hoisted. `test_lazy_intra_package_imports_are_all_declared` fails on any
undeclared one from here, and also on a stale declaration — so the list
cannot rot in either direction.

`tidy_sources` moves to a new `sources.py`. It was in `lowering.py`, where it
shared no code and no concept with anything: lowering turns an AST into a plan
and touches no data, that function touches only data and knows nothing about
expressions. They were together because `api.build` calls them on consecutive
lines.

311 tests pass, 3 of them new.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@FBumann
FBumann force-pushed the refactor/untangle-language-layer branch from 5916635 to 4076ca6 Compare July 26, 2026 16:47
@FBumann
FBumann merged commit b32e6e8 into refactor/split-relational-engine Jul 26, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant