Skip to content

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

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

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

Conversation

@FBumann

@FBumann FBumann commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Re-opening #110. It was stacked on #107 and got merged into that branch a minute after #107 squash-merged into main, so its content never reached main. Same commit, replayed onto current main, re-validated. No review comments were left on #110.

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. With it gone, 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 and named it. It turned out to be hoistable (arrow.py imports pyarrow lazily itself, so module level costs nothing), so that is one more 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

Against current main (eae4d40): 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

Warning

Review limit reached

@FBumann, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 41 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3247e343-b336-4ec6-9a5d-d610f95ff873

📥 Commits

Reviewing files that changed from the base of the PR and between ecc0855 and e15b6f1.

📒 Files selected for processing (18)
  • ARCHITECTURE.md
  • CLAUDE.md
  • examples/walkthrough.py
  • src/linopy_yaml/api.py
  • src/linopy_yaml/dimensions.py
  • src/linopy_yaml/lowering.py
  • src/linopy_yaml/piecewise.py
  • src/linopy_yaml/resolution.py
  • src/linopy_yaml/sources.py
  • src/linopy_yaml/validation.py
  • tests/test_architecture.py
  • tests/test_expansion.py
  • tests/test_group_sum.py
  • tests/test_lowering.py
  • tests/test_milp.py
  • tests/test_piecewise_block.py
  • tests/test_piecewise_convex.py
  • tests/test_roll.py
✨ 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.

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 1f864a3 to e15b6f1 Compare July 26, 2026 17:48
@FBumann
FBumann merged commit ecad711 into main Jul 26, 2026
7 checks passed
@FBumann
FBumann deleted the refactor/untangle-language-layer branch July 31, 2026 10:52
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