One language, two lanes: closed helper set, no monkey-patch, compat as a pure-producer shim - #43
Conversation
|
Warning Review limit reached
Next review available in: 50 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 (34)
📝 WalkthroughWalkthroughThe project replaces monkey-patched YAML integration with explicit native and compat APIs, removes custom helper registration, formalizes bounded language and escape semantics, adds parquet solution streaming and cleanup, and updates documentation, dependencies, and tests. ChangesLanguage and API transition
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Caller
participant linopy_yaml
participant StreamingEngine
participant DuckdbExecutor
participant Solver
Caller->>linopy_yaml: solve(model, sources)
linopy_yaml->>StreamingEngine: load, validate, and lower YAML
StreamingEngine->>DuckdbExecutor: execute relational model
DuckdbExecutor->>Solver: solve generated model
Solver-->>DuckdbExecutor: solution tables
Caller->>DuckdbExecutor: to_parquet(directory)
DuckdbExecutor-->>Caller: parquet paths
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
…cer shim Two lanes that accept different languages cannot be an oracle for each other. Two changes close that gap, and a third follows from it. - Remove `@register` and the helper registry. The built-in set is closed, so the streaming lane and the compat lane accept exactly the same language. Compositions belong in `macros:`; math the language cannot say belongs in a declared `escape:` island (#38), which is visible in the file rather than reading like a built-in on the page. - Remove the `linopy.Model` monkey-patch (`_patch.py`) and `YamlAccessor`. `compat` now exposes `build()` and `extend()`, both pure producers: YAML in, model out, nothing retained. The returned object is a plain `linopy.Model` that stands for itself. This also drops the dependency on `Model.__slots__` carrying `__weakref__`, so "pure consumer of linopy's public API" is now true rather than aspirational. - The accessor's accumulated parameter dataset let a YAML file reference parameters declared by an earlier file — meaning through Python-side state, which hard rule 5 forbids. Gone: every file declares what it uses, and the caller supplies that data per call. State derived from the model argument (its variables, its coords) stays. `validate_expressions(known_parameters=)` goes with it. Also: `[oracle]` extra -> `[compat]`; `extend()` now expands `piecewise:` blocks like `build()` does; lowering rejections name the construct and its rewrite instead of pointing at "the eager backend", which hard rule 3 abolished; the linopy-free architecture check now sees through module-level `try:` guards, which could previously hide a forbidden import. Docs record the decisions this came out of, not just the API delta: - ARCHITECTURE: hard rule 3 restated as "one language, two lanes" (the property the code now enforces); new "expressive ceiling" section defining the admissible closure as degree 1 ∩ relational ∩ local, with the sink-/budget-/design-bounded split that says which exclusions can ever move. - ROADMAP (new): how capability is measured — the closure as spec, Calliope and PyPSA constraint modules as corpora (#27), scale as the second axis — plus the primitive backlog, the degree axis, and the deliberate non-primitives with their rewrites. - SPEC: §2 and §9 describe the native path first and the shim second; §7.5 replaces the custom-helper section; non-goals now name parity-with-other- languages and xarray-operation-parity explicitly; §12's fallback framing marked superseded; Q3 restated as a capability gap. Dissolves #12 — accessor state cannot be lost across to_netcdf/pickle/ deepcopy when nothing is attached in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sdist already carries ARCHITECTURE.md and SPEC.md; ROADMAP.md joins them. CLAUDE.md's install comment still named the pre-rename `oracle` extra. pyarrow stays a runtime dependency: the solver_direct sink reaches it through duckdb's `.to_arrow_reader()`, so it never appears as an `import pyarrow` — now noted next to the dependency so the next reader does not mistake it for dead weight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@CLAUDE.md`:
- Around line 35-51: Update CLAUDE.md’s module map to remove the duplicate
compat.py entry and describe the pure-producer compat lane. Update README.md’s
overview and Mermaid diagram to document that unsupported constructs fail during
loading rather than route to eager fallback.
- Line 37: Update the module-tree fenced code block in CLAUDE.md to include an
appropriate language tag, preferably text, immediately after the opening fence
while preserving its contents.
- Around line 55-61: Update every public solve example to use the
context-managed Solution: in CLAUDE.md lines 55-61, README.md lines 37-41, and
README.md lines 100-113, wrap each ly.solve(...) assignment in a with block and
keep the existing solution access inside it.
In `@GLOSSARY.md`:
- Around line 96-100: Update ROADMAP.md to include the exponentiation (`**`)
streaming parity gap described in GLOSSARY.md, aligning it with the existing
roadmap item for related language-parity gaps; preserve the glossary’s
description and classification.
- Around line 267-271: Update SPEC §12.4 in GLOSSARY.md to include the
implemented IR nodes Div, Shift, Defined, and Bool in its IR listing. Also align
the where-AST and IR terminology for equivalent concepts by mapping
ExistenceCheck to Defined, Comparison to Cmp, and BoolLiteral to Bool, keeping
the documentation consistent with relational/ir.py.
- Around line 323-329: Update load-time validation in validation.py to validate
dimension-valued kwargs, including sum’s over=, into=, and roll/shift dimension
keys, against the declared dimensions before either execution lane dispatches.
Do not skip these kwarg NameNodes; emit clear, actionable errors for unknown
dimension names, while preserving existing validation for data references and
valid dimensions.
In `@linopy_yaml/compat.py`:
- Around line 125-149: Update the coordinate validation flow around
_infer_coords and build_master_coords so YAML dimension values are compared
against the model’s inferred coordinates before applying explicit coords
overrides. Preserve the inferred mapping for consistency checks, then merge
coords into the mapping used for master-coordinate resolution, retaining the
existing mismatch error behavior and actionable message.
In `@linopy_yaml/relational/executor.py`:
- Around line 777-789: Update _solution_to_parquet so the destination path used
in the DuckDB COPY statement is safely escaped as a SQL string literal before
interpolation, preserving valid paths containing apostrophes and preventing
injection. Apply the escaping specifically to out while keeping the existing
parquet output and written mapping behavior unchanged.
In `@README.md`:
- Around line 47-49: Add the linopy_yaml compat import to the README example and
explicitly identify m as an existing linopy.Model before the compat.extend call,
keeping the example otherwise unchanged.
In `@tests/test_api.py`:
- Around line 169-182: Update test_solution_context_manager_and_to_parquet to
import pyarrow.parquet through pytest.importorskip, so the test is skipped when
pyarrow is unavailable while preserving the existing parquet assertions when it
is installed.
🪄 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: b7df1535-37ec-4c96-b3d6-44e6b66bc49f
📒 Files selected for processing (37)
.github/workflows/ci.ymlARCHITECTURE.mdCLAUDE.mdGLOSSARY.mdREADME.mdROADMAP.mdSPEC.mdlinopy_yaml/__init__.pylinopy_yaml/_patch.pylinopy_yaml/accessor.pylinopy_yaml/api.pylinopy_yaml/compat.pylinopy_yaml/expansion.pylinopy_yaml/helpers.pylinopy_yaml/loader.pylinopy_yaml/lowering.pylinopy_yaml/piecewise.pylinopy_yaml/relational/__init__.pylinopy_yaml/relational/executor.pylinopy_yaml/validation.pypyproject.tomltests/conftest.pytests/test_accessor.pytests/test_api.pytests/test_architecture.pytests/test_compat.pytests/test_dispatch.pytests/test_error_notes.pytests/test_expansion.pytests/test_group_sum.pytests/test_language_boundary.pytests/test_lowering.pytests/test_milp.pytests/test_piecewise_block.pytests/test_piecewise_convex.pytests/test_roll.pytests/test_validation.py
💤 Files with no reviewable changes (3)
- tests/test_accessor.py
- linopy_yaml/accessor.py
- linopy_yaml/_patch.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
🤖 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 `@CLAUDE.md`:
- Around line 35-51: Update CLAUDE.md’s module map to remove the duplicate
compat.py entry and describe the pure-producer compat lane. Update README.md’s
overview and Mermaid diagram to document that unsupported constructs fail during
loading rather than route to eager fallback.
- Line 37: Update the module-tree fenced code block in CLAUDE.md to include an
appropriate language tag, preferably text, immediately after the opening fence
while preserving its contents.
- Around line 55-61: Update every public solve example to use the
context-managed Solution: in CLAUDE.md lines 55-61, README.md lines 37-41, and
README.md lines 100-113, wrap each ly.solve(...) assignment in a with block and
keep the existing solution access inside it.
In `@GLOSSARY.md`:
- Around line 96-100: Update ROADMAP.md to include the exponentiation (`**`)
streaming parity gap described in GLOSSARY.md, aligning it with the existing
roadmap item for related language-parity gaps; preserve the glossary’s
description and classification.
- Around line 267-271: Update SPEC §12.4 in GLOSSARY.md to include the
implemented IR nodes Div, Shift, Defined, and Bool in its IR listing. Also align
the where-AST and IR terminology for equivalent concepts by mapping
ExistenceCheck to Defined, Comparison to Cmp, and BoolLiteral to Bool, keeping
the documentation consistent with relational/ir.py.
- Around line 323-329: Update load-time validation in validation.py to validate
dimension-valued kwargs, including sum’s over=, into=, and roll/shift dimension
keys, against the declared dimensions before either execution lane dispatches.
Do not skip these kwarg NameNodes; emit clear, actionable errors for unknown
dimension names, while preserving existing validation for data references and
valid dimensions.
In `@linopy_yaml/compat.py`:
- Around line 125-149: Update the coordinate validation flow around
_infer_coords and build_master_coords so YAML dimension values are compared
against the model’s inferred coordinates before applying explicit coords
overrides. Preserve the inferred mapping for consistency checks, then merge
coords into the mapping used for master-coordinate resolution, retaining the
existing mismatch error behavior and actionable message.
In `@linopy_yaml/relational/executor.py`:
- Around line 777-789: Update _solution_to_parquet so the destination path used
in the DuckDB COPY statement is safely escaped as a SQL string literal before
interpolation, preserving valid paths containing apostrophes and preventing
injection. Apply the escaping specifically to out while keeping the existing
parquet output and written mapping behavior unchanged.
In `@README.md`:
- Around line 47-49: Add the linopy_yaml compat import to the README example and
explicitly identify m as an existing linopy.Model before the compat.extend call,
keeping the example otherwise unchanged.
In `@tests/test_api.py`:
- Around line 169-182: Update test_solution_context_manager_and_to_parquet to
import pyarrow.parquet through pytest.importorskip, so the test is skipped when
pyarrow is unavailable while preserving the existing parquet assertions when it
is installed.
🪄 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: b7df1535-37ec-4c96-b3d6-44e6b66bc49f
📒 Files selected for processing (37)
.github/workflows/ci.ymlARCHITECTURE.mdCLAUDE.mdGLOSSARY.mdREADME.mdROADMAP.mdSPEC.mdlinopy_yaml/__init__.pylinopy_yaml/_patch.pylinopy_yaml/accessor.pylinopy_yaml/api.pylinopy_yaml/compat.pylinopy_yaml/expansion.pylinopy_yaml/helpers.pylinopy_yaml/loader.pylinopy_yaml/lowering.pylinopy_yaml/piecewise.pylinopy_yaml/relational/__init__.pylinopy_yaml/relational/executor.pylinopy_yaml/validation.pypyproject.tomltests/conftest.pytests/test_accessor.pytests/test_api.pytests/test_architecture.pytests/test_compat.pytests/test_dispatch.pytests/test_error_notes.pytests/test_expansion.pytests/test_group_sum.pytests/test_language_boundary.pytests/test_lowering.pytests/test_milp.pytests/test_piecewise_block.pytests/test_piecewise_convex.pytests/test_roll.pytests/test_validation.py
💤 Files with no reviewable changes (3)
- tests/test_accessor.py
- linopy_yaml/accessor.py
- linopy_yaml/_patch.py
🛑 Comments failed to post (10)
CLAUDE.md (3)
35-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the documentation with the new explicit compat/strict-language architecture.
The changed sections still leave the old behavior documented:
CLAUDE.mddescribes monkey-patching/oracle behavior, whileREADME.mdretains an eager-fallback overview.
CLAUDE.md#L35-L51: remove the duplicatecompat.pyentry and describe the pure-producer compat lane.README.md#L69-L69: update the earlier overview and Mermaid diagram so unsupported constructs fail at load time rather than route to eager fallback.🧰 Tools
🪛 LanguageTool
[style] ~35-~35: ‘In brief’ might be wordy. Consider a shorter alternative.
Context: ...E.md` for the authoritative module map. In brief: ``` linopy_yaml/ ├── api.py ...(EN_WORDINESS_PREMIUM_IN_BRIEF)
🪛 markdownlint-cli2 (0.23.0)
[warning] 37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 2 files
CLAUDE.md#L35-L51(this comment)README.md#L69-L69🤖 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 `@CLAUDE.md` around lines 35 - 51, Update CLAUDE.md’s module map to remove the duplicate compat.py entry and describe the pure-producer compat lane. Update README.md’s overview and Mermaid diagram to document that unsupported constructs fail during loading rather than route to eager fallback.
37-37: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the module-tree fence.
Use
```text(or another suitable language) to satisfy markdownlint MD040 and keep documentation checks clean.🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@CLAUDE.md` at line 37, Update the module-tree fenced code block in CLAUDE.md to include an appropriate language tag, preferably text, immediately after the opening fence while preserving its contents.Source: Linters/SAST tools
55-61: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use the context-managed
Solutionin every public solve example.These examples leave the DuckDB-backed solution open until garbage collection.
CLAUDE.md#L55-L61: wraply.solve(...)in awithblock or callclose().README.md#L37-L41: usewith ly.solve(...) as sol:in the short example.README.md#L100-L113: use the same context-managed form in the quick example.📍 Affects 2 files
CLAUDE.md#L55-L61(this comment)README.md#L37-L41README.md#L100-L113🤖 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 `@CLAUDE.md` around lines 55 - 61, Update every public solve example to use the context-managed Solution: in CLAUDE.md lines 55-61, README.md lines 37-41, and README.md lines 100-113, wrap each ly.solve(...) assignment in a with block and keep the existing solution access inside it.GLOSSARY.md (3)
96-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Track the exponentiation gap in
ROADMAP.md.This section documents
**as a streaming parity gap while explicitly saying it is unlisted in the roadmap. Add the gap to the roadmap or remove this cross-document claim before shipping the language 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 `@GLOSSARY.md` around lines 96 - 100, Update ROADMAP.md to include the exponentiation (`**`) streaming parity gap described in GLOSSARY.md, aligning it with the existing roadmap item for related language-parity gaps; preserve the glossary’s description and classification.
267-271: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Synchronize SPEC §12.4 with the IR map.
The glossary says the specification omits
Div,Shift,Defined, andBool, although those nodes are implemented. Leaving the “full specification” stale makes supported and rejected constructs ambiguous across the documented 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 `@GLOSSARY.md` around lines 267 - 271, Update SPEC §12.4 in GLOSSARY.md to include the implemented IR nodes Div, Shift, Defined, and Bool in its IR listing. Also align the where-AST and IR terminology for equivalent concepts by mapping ExistenceCheck to Defined, Comparison to Cmp, and BoolLiteral to Bool, keeping the documentation consistent with relational/ir.py.
323-329: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate dimension-valued kwargs during load.
sum(p, over=snapshto)can currently be accepted and silently lowered as a no-op: validation skips kwargNameNodes, and the lowering path only checks whether the operand carries that name. Validateover=,into=, and roll/shift dimension keys againstdimensions:before either lane dispatches.As per coding guidelines, all validation should happen at load time with clear, actionable error messages.
🤖 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 `@GLOSSARY.md` around lines 323 - 329, Update load-time validation in validation.py to validate dimension-valued kwargs, including sum’s over=, into=, and roll/shift dimension keys, against the declared dimensions before either execution lane dispatches. Do not skip these kwarg NameNodes; emit clear, actionable errors for unknown dimension names, while preserving existing validation for data references and valid dimensions.Source: Coding guidelines
linopy_yaml/compat.py (1)
125-149: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate declared coordinates against the actual model before applying overrides.
coords=overwritesknownbefore the consistency check, so it can mask a mismatch between YAMLvalues:and existing model coordinates. Preserve inferred coordinates for the comparison, then apply explicit overrides for master-coordinate resolution.Proposed fix
- known = _infer_coords(model) - if coords is not None: - known.update({k: pd.Index(v, name=k) for k, v in coords.items()}) + inferred = _infer_coords(model) # If this YAML declares values: for a dim the model already has, they # must match. Silent override would hide real bugs. for dim_name, dim_def in schema.dimensions.items(): - if dim_def.values is None or dim_name not in known: + if dim_def.values is None or dim_name not in inferred: continue declared = pd.Index(dim_def.values, name=dim_name) - existing = known[dim_name] + existing = inferred[dim_name] if not declared.equals(existing): ... + known = inferred.copy() + if coords is not None: + known.update({k: pd.Index(v, name=k) for k, v in coords.items()}) master_coords = build_master_coords(schema, known)As per coding guidelines, “Perform all validation at load time and provide clear, actionable error messages.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.inferred = _infer_coords(model) # If this YAML declares values: for a dim the model already has, they # must match. Silent override would hide real bugs. for dim_name, dim_def in schema.dimensions.items(): if dim_def.values is None or dim_name not in inferred: continue declared = pd.Index(dim_def.values, name=dim_name) existing = inferred[dim_name] if not declared.equals(existing): msg = ( f"Extension declares dimension '{dim_name}' with values " f'that differ from the existing model.\n' f' Existing: {list(existing)}\n' f' Declared: {list(declared)}\n' f"Either omit 'values:' for '{dim_name}' in the " f'extension, or make them match.' ) raise ValueError(msg) # ``known`` is the override, so any dim it covers beats this YAML's # ``values:``. Dims still missing fall through to ``values:`` or raise. known = inferred.copy() if coords is not None: known.update({k: pd.Index(v, name=k) for k, v in coords.items()}) master_coords = build_master_coords(schema, known)🤖 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 `@linopy_yaml/compat.py` around lines 125 - 149, Update the coordinate validation flow around _infer_coords and build_master_coords so YAML dimension values are compared against the model’s inferred coordinates before applying explicit coords overrides. Preserve the inferred mapping for consistency checks, then merge coords into the mapping used for master-coordinate resolution, retaining the existing mismatch error behavior and actionable message.Source: Coding guidelines
linopy_yaml/relational/executor.py (1)
777-789: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== locate file ==" git ls-files | rg '(^|/)executor\.py$|linopy_yaml/relational' echo "== outline ==" ast-grep outline linopy_yaml/relational/executor.py --match _solution_to_parquet --view expanded || true echo "== relevant lines ==" sed -n '740,805p' linopy_yaml/relational/executor.py echo "== search copy/to usage ==" rg -n "COPY \(SELECT|TO '[^']*'|format parquet|parquet" linopy_yaml/relational/executor.py echo "== duckdb available? show api if ==" python3 - <<'PY' try: import duckdb print("duckdb", getattr(duckdb, "__version__", "<no ver>")) except Exception as e: print("duckdb_import_error:", repr(e)) PYRepository: FBumann/linopy-yaml
Length of output: 4201
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== behavioral probe: single-quoted COPY path with apostrophe and quote-like payloads ==" python3 - <<'PY' import duckdb import tempfile from pathlib import Path con = duckdb.connect(":memory:") con.execute("CREATE TABLE t (x VARCHAR)") con.execute("INSERT INTO t VALUES ('a')") for filename in [ "good.parquet", "path'containing'apostrophe.parquet", "path'; DROP TRIGGER x;--.parquet", 'path"double"quote.parquet', "path\ntab.parquet", ]: with tempfile.TemporaryDirectory() as td: dirp = Path(td) out = dirp / filename try: con.execute("COPY (SELECT x FROM t) TO '{}' (FORMAT parquet) RETURN FILE_SIZE".format(out).replace("'", "''")) print("escaped_ok", str(out), out.exists()) except Exception as e: print("escaped_err", str(out), type(e).__name__, str(e).splitlines()[0]) try: con.execute(f"COPY (SELECT x FROM t) TO '{out}' (FORMAT parquet) RETURN FILE_SIZE") print("literal_ok", str(out), out.exists()) except Exception as e: print("literal_err", str(out), type(e).__name__, str(e).splitlines()[0]) PY echo "== DuckDB parameterized file output/docs ==" python3 - <<'PY' import inspect import duckdb for name in ["DuckDBPyConnection", "DuckDBPyRelation"]: obj = getattr(duckdb, name, None) if obj is None: continue print(f"## {name}") for attr in dir(obj): if attr in ("to_csv_file", "to_csv", "to_parquet", "write_parquet", "write_csv"): m = getattr(obj, attr, None) if m is None: continue try: print(attr, inspect.signature(m)) print(inspect.getdoc(m)) except Exception as e: print(attr, "inspect_err", repr(e)) PYRepository: FBumann/linopy-yaml
Length of output: 1925
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== corrected COPY TO behavior with properly escaped absolute path ==" python3 - <<'PY' import duckdb import tempfile from pathlib import Path con = duckdb.connect(":memory:") con.execute("CREATE TABLE t (x VARCHAR)") con.execute("INSERT INTO t VALUES ('a')") for filename in [ "good.parquet", "path'containing'apostrophe.parquet", "path"; DROP QUOTE x="--.parquet", 'path"double"quote.parquet', ]: with tempfile.TemporaryDirectory() as td: dirp = Path(td) out = dirp / filename sql = f"COPY (SELECT x FROM t) TO '{out}' (FORMAT parquet) RETURN FILE_SIZE" print("sql:", repr(sql)) try: res = con.execute(sql).fetchall() print("success", str(out), "exists", out.exists(), "rows", len(res)) except Exception as e: print("error", type(e).__name__, str(e).splitlines()[0]) print("\n== parameterized/relational write APIs ==") con.execute("CREATE TABLE t (x VARCHAR)") con.execute("INSERT INTO t VALUES ('a')") with tempfile.TemporaryDirectory() as td: out = Path(td) / "rel.parquet" rel = con.execute("SELECT x FROM t") rel.write_parquet(out) print("rel.write_parquet", out.exists()) PYRepository: FBumann/linopy-yaml
Length of output: 331
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import duckdb import tempfile from pathlib import Path con = duckdb.connect(":memory:") con.execute("CREATE TABLE t (x VARCHAR)") con.execute("INSERT INTO t VALUES ('a')") filenames = [ "good.parquet", "path'containing'apostrophe.parquet", "path'; DROP TRIGGER x;--.parquet", 'path"double"quote.parquet', ] for filename in filenames: with tempfile.TemporaryDirectory() as td: dirp = Path(td) out = dirp / filename sql_literal = f"COPY (SELECT x FROM t) TO '{out}' (FORMAT parquet) RETURN FILE_SIZE" sql_escaped = f"COPY (SELECT x FROM t) TO '{str(out).replace(chr(39), chr(39) + chr(39))}' (FORMAT parquet) RETURN FILE_SIZE" print("*", filename) print(" literal sql:", repr(sql_literal)) try: res = con.execute(sql_literal).fetchall() print(" literal success:", out.exists()) except Exception as e: print(" literal error:", type(e).__name__, str(e).splitlines()[0]) print(" escaped sql:", repr(sql_escaped)) try: res = con.execute(sql_escaped).fetchall() print(" escaped success:", out.exists(), "file_size", res[0][0] if res else None) except Exception as e: print(" escaped error:", type(e).__name__, str(e).splitlines()[0]) print("\n== parameterized/write API ==") con.execute("CREATE TABLE t (x VARCHAR)") con.execute("INSERT INTO t VALUES ('a')") with tempfile.TemporaryDirectory() as td: out = Path(td) / "rel.parquet" rel = con.execute("SELECT x FROM t") rel.write_parquet(out) print("rel.write_parquet", out.exists()) PYRepository: FBumann/linopy-yaml
Length of output: 2082
🌐 Web query:
DuckDB PYTHON COPY TO RETURN FILE_SIZE returns file size output option💡 Result:
To obtain information about file sizes after a
COPY TOoperation in DuckDB, you should use theRETURN_STATSoption [1][2]. When you execute aCOPY ... TOstatement with(RETURN_STATS TRUE), DuckDB returns a result set containing detailed information about the files written, including afile_size_bytescolumn [1][2]. Example usage in Python: con.execute("COPY (SELECT * FROM my_table) TO 'output.parquet' (RETURN_STATS TRUE);").fetchall The output of this query will be a result set containing the following columns for each file generated: - filename - count - file_size_bytes - footer_size_bytes - column_statistics - partition_keys - extra_info Note that while there is an option calledFILE_SIZE_BYTES, it is a configuration parameter used to split output into multiple files of a specific size (often used withPER_THREAD_OUTPUT), not an output option to return the file size [1][2][3]. To get the size of the generated files, you must useRETURN_STATS[1][2]. Additionally, if you only need the list of filenames, you can useRETURN_FILES TRUE[4][1].Citations:
- 1: https://duckdb.org/docs/lts/sql/statements/copy.html
- 2: https://raw.githubusercontent.com/duckdb/duckdb-web/refs/heads/main/docs/current/sql/statements/copy.md
- 3: https://raw.githubusercontent.com/duckdb/duckdb-web/refs/heads/main/docs/1.2/sql/statements/copy.md
- 4: duckdb/duckdb#12220
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import duckdb import tempfile from pathlib import Path con = duckdb.connect(":memory:") filenames = [ "good.parquet", "path'containing'apostrophe.parquet", "path'; CREATE TABLE x AS SELECT 1; DROP TRIGGER x;--.parquet", 'path"double"quote.parquet', ] for filename in filenames: with tempfile.TemporaryDirectory() as td: dirp = Path(td) out = dirp / filename sql_escaped = ( "COPY (SELECT 'a') TO '" + str(out).replace("'", "''") + "' (FORMAT parquet) (RETURN_FILES TRUE)" ) print("filename:", filename) print("sql:", sql_escaped) try: res = con.execute(sql_escaped).fetchall() print("success:", out.exists()) except Exception as e: print("error:", type(e).__name__, str(e).splitlines()[0]) print("\n== parameterized write API ==") with tempfile.TemporaryDirectory() as td: out = Path(td) / "rel.parquet" rel = con.sql("SELECT 'a'") rel.write_parquet(out) print("rel.write_parquet", out.exists()) PYRepository: FBumann/linopy-yaml
Length of output: 1834
🌐 Web query:
DuckDB Python write_parquet Path incompatible function arguments Path str💡 Result:
The error "incompatible function arguments" when passing a pathlib.Path object to DuckDB's Python functions typically occurs because the function expects a string [1]. While DuckDB has updated several functions to accept path-like objects (such as pathlib.Path) [2], inconsistencies have existed across different API methods (e.g., read_parquet vs write_parquet) [1][3]. To resolve this, you must explicitly convert your Path object to a string before passing it to the function: import duckdb from pathlib import Path path = Path("output.parquet") # Use str(path) to avoid the TypeError duckdb.sql("SELECT 42").write_parquet(str(path)) Alternatively, you can use the SQL COPY statement, which is a robust alternative for exporting data and is generally less sensitive to Python-side type restrictions: import duckdb from pathlib import Path path = Path("output.parquet") # SQL COPY handles the file path natively duckdb.sql(f"COPY (SELECT 42) TO '{path}'") DuckDB continues to improve its Python API to achieve better parity with standard Python file handling [3], but string conversion remains the most reliable workaround if you encounter this error in current versions.
Citations:
Escape the parquet destination path before embedding it in SQL.
OUTis interpolated directly intoTO '{out}', so a valid path containing an apostrophe breaks theCOPY; if the directory name is externally controlled, this also creates an SQL injection surface. Escape the path string literal or use DuckDB’s Python write-api with the path passed separately.🤖 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 `@linopy_yaml/relational/executor.py` around lines 777 - 789, Update _solution_to_parquet so the destination path used in the DuckDB COPY statement is safely escaped as a SQL string literal before interpolation, preserving valid paths containing apostrophes and preventing injection. Apply the escaping specifically to out while keeping the existing parquet output and written mapping behavior unchanged.README.md (1)
47-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the compat example self-contained.
The snippet calls
compat.extend(...)without importingcompat; it also relies on an implicit existingm. Addfrom linopy_yaml import compatand clearly state thatmis an existinglinopy.Model.🤖 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 `@README.md` around lines 47 - 49, Add the linopy_yaml compat import to the README example and explicitly identify m as an existing linopy.Model before the compat.extend call, keeping the example otherwise unchanged.tests/test_api.py (1)
169-182: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the pyarrow import with
importorskip.
pyarrow.parquetis imported unconditionally inside this test, but per the PR objective pyarrow is no longer a runtime dependency, and this module isn't gated on any dependency inconftest.py(unlike the linopy-gated modules). On an environment without pyarrow installed this test will error rather than skip.🧪 Proposed fix
def test_solution_context_manager_and_to_parquet(dispatch_yaml, dispatch_inputs, tmp_path): - import pyarrow.parquet as pq + pq = pytest.importorskip('pyarrow.parquet')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def test_solution_context_manager_and_to_parquet(dispatch_yaml, dispatch_inputs, tmp_path): pq = pytest.importorskip('pyarrow.parquet') sources, coords = dispatch_inputs with ly.solve(dispatch_yaml, sources, coords=coords) as sol: assert sol.status == 'Optimal' written = sol.to_parquet(tmp_path / 'solution') assert set(written) == {'p'} table = pq.read_table(written['p']) assert set(table.column_names) == {'snapshot', 'generator', 'value'} assert table.num_rows == sol.primal('p').shape[0] # closed by the with-block: the workdir is gone with pytest.raises(Exception): # noqa: B017 — any error is fine, it must not silently work sol.primal('p')🤖 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_api.py` around lines 169 - 182, Update test_solution_context_manager_and_to_parquet to import pyarrow.parquet through pytest.importorskip, so the test is skipped when pyarrow is unavailable while preserving the existing parquet assertions when it is installed.
FBumann
changed the title
…erge `sum(p, over=snapshto)` used to build a model that solved and was wrong. The typo made the sum a no-op: `_helper_sum` returns the array unchanged when it does not carry the named dim, and `lowering.py` deliberately mirrors that for eager parity — so both lanes agreed, and nothing downstream caught it. `validation.py` skipped kwarg NameNodes as "dimension names, not data references" and no other pass checked them. It now checks that they name a *declared* dimension, in both positions the language uses: - value position — `sum(x, over=d)`, `group_sum(x, m, into=d)` - key position — `roll(x, d=1)`, `shift(x, d=1)` Macro formals stay legal in a dim position: the template body is checked against `dimensions | formals`, the same widening already applied to variables and parameters. The error names the consequence, since that is the part that bites: "an unknown dimension makes sum() a silent no-op rather than an error". Also restores four doc fixes that #43 merged without — GLOSSARY.md landed via an earlier commit, but the corrective one did not: - ARCHITECTURE/CLAUDE: pointers to GLOSSARY.md, and the tier-1 diagram no longer calls `piecewise:` a primitive (it expands to declarations and has no IR node — glossary section 0, SPEC §12.4). - SPEC §12.4: the IR listing had drifted from `relational/ir.py` — `Div` and `Shift` missing from expressions, `Defined` and `Bool` from predicates. - ROADMAP: row 5d, `**` evaluated eagerly and rejected by lowering. - SPEC §7: dimension arguments are name-checked at load time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…erge (#48) `sum(p, over=snapshto)` used to build a model that solved and was wrong. The typo made the sum a no-op: `_helper_sum` returns the array unchanged when it does not carry the named dim, and `lowering.py` deliberately mirrors that for eager parity — so both lanes agreed, and nothing downstream caught it. `validation.py` skipped kwarg NameNodes as "dimension names, not data references" and no other pass checked them. It now checks that they name a *declared* dimension, in both positions the language uses: - value position — `sum(x, over=d)`, `group_sum(x, m, into=d)` - key position — `roll(x, d=1)`, `shift(x, d=1)` Macro formals stay legal in a dim position: the template body is checked against `dimensions | formals`, the same widening already applied to variables and parameters. The error names the consequence, since that is the part that bites: "an unknown dimension makes sum() a silent no-op rather than an error". Also restores four doc fixes that #43 merged without — GLOSSARY.md landed via an earlier commit, but the corrective one did not: - ARCHITECTURE/CLAUDE: pointers to GLOSSARY.md, and the tier-1 diagram no longer calls `piecewise:` a primitive (it expands to declarations and has no IR node — glossary section 0, SPEC §12.4). - SPEC §12.4: the IR listing had drifted from `relational/ir.py` — `Div` and `Shift` missing from expressions, `Defined` and `Bool` from predicates. - ROADMAP: row 5d, `**` evaluated eagerly and rejected by lowering. - SPEC §7: dimension arguments are name-checked at load time. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two divergences #43 recorded and left. Both were the same defect: a file meant different things depending on which lane built it, which hard rule 3 forbids. They are fixed in opposite directions, because the language should contain one and not the other. ROADMAP 5b — `where: "snapshot > 0"` now lowers. A dimension comparison needs no join: the dim table is already in the frame, so it is a filter on its own column. New `ir.DimCmp(dim, op, value)` predicate, one executor branch, and the lowering case that used to raise. The README's own ramp example was affected — a time-coupling constraint that skips the first snapshot is the canonical use, and it was eager-only. A dimension outside the component's `foreach` is still rejected, with the reason: the mask would have to be `any`-reduced over an unlisted dim, which mirrors the existing guard for out-of-frame parameters. ROADMAP 5d — `**` is now rejected by the eager evaluator too, matching the lowering that already refused it. It stays in the grammar and out of the language: a variable base breaks degree 1, and a parameters-only power is data prep. The error says which of the two rewrites applies. Adds a differential test over the ramp pattern: same YAML and data through both lanes, matching objectives, with an assertion that the mask actually bites (the first snapshot is dropped per generator) so the test cannot pass by ignoring the where entirely. SPEC: predicate listing gains `DimCmp`; the precedence table and §5.2 now say `**` parses but is refused at load time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…*` (#52) The two divergences #43 recorded and left. Both were the same defect: a file meant different things depending on which lane built it, which hard rule 3 forbids. They are fixed in opposite directions, because the language should contain one and not the other. ROADMAP 5b — `where: "snapshot > 0"` now lowers. A dimension comparison needs no join: the dim table is already in the frame, so it is a filter on its own column. New `ir.DimCmp(dim, op, value)` predicate, one executor branch, and the lowering case that used to raise. The README's own ramp example was affected — a time-coupling constraint that skips the first snapshot is the canonical use, and it was eager-only. A dimension outside the component's `foreach` is still rejected, with the reason: the mask would have to be `any`-reduced over an unlisted dim, which mirrors the existing guard for out-of-frame parameters. ROADMAP 5d — `**` is now rejected by the eager evaluator too, matching the lowering that already refused it. It stays in the grammar and out of the language: a variable base breaks degree 1, and a parameters-only power is data prep. The error says which of the two rewrites applies. Adds a differential test over the ramp pattern: same YAML and data through both lanes, matching objectives, with an assertion that the mask actually bites (the first snapshot is dropped per generator) so the test cannot pass by ignoring the where entirely. SPEC: predicate listing gains `DimCmp`; the precedence table and §5.2 now say `**` parses but is refused at load time. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Three commits, rebased onto main after #36 and #37 landed. Two lanes that
accept different languages cannot be an oracle for each other; this closes
that gap and stops patching linopy.
No helper registry
@registeris gone. The built-in set is closed, so the streaming lane and thecompat lane accept exactly the same language — which is what makes the
differential tests an oracle rather than a comparison of two dialects.
Compositions belong in
macros:; math the language cannot say belongs in adeclared
escape:island (#38), which is visible in the file instead ofreading like a built-in on the page.
No monkey-patch
_patch.pyandYamlAccessorare deleted.compatexposesbuild()andextend(), both pure producers: YAML in, model out, nothing retained. Theresult is a plain
linopy.Modelthat stands for itself. This drops thedependency on
Model.__slots__carrying__weakref__, so "pure consumer oflinopy's public API" is true rather than aspirational.
No accumulated state
The accessor's merged parameter dataset let a YAML file reference parameters
declared by an earlier file — meaning through Python-side state, which hard
rule 5 forbids. Every file now declares what it uses, and the caller supplies
that data per call. State derived from the model argument (its variables, its
coords) stays.
Also:
[oracle]extra →[compat];extend()expandspiecewise:blockslike
build()does; lowering rejections name the construct and its rewriteinstead of pointing at "the eager backend", which hard rule 3 abolished; and
the linopy-free architecture check now sees through module-level
try:guards, which could previously hide a forbidden import.
Dissolves #12 — accessor state cannot be lost across
to_netcdf/pickle/deepcopywhen nothing is attached in the first place.Docs record the decisions, not just the API delta
property the code now enforces. New expressive ceiling section defining
the admissible closure as degree 1 ∩ relational ∩ local, with the
sink-/budget-/design-bounded split that says which exclusions can ever move.
Calliope and PyPSA's constraint modules as corpora (Streaming-subset completeness audit: every lowering rejection is now a product gap #27), scale as the
second axis — plus the primitive backlog, the degree axis (linear stays the
goal, recorded with its revisit conditions), and the deliberate
non-primitives with their rewrites.
every layer. Section 0 separates five things that "primitive" was covering:
only constructs with an IR node and a lowering case carry the two-backend
tax, while
piecewise:is taxed but expands to declarations before eitherlane sees it. The tier-1 diagram is corrected to match.
§7.5 replaces the custom-helper section; non-goals name
parity-with-other-languages and xarray-operation-parity explicitly; §12's
fallback framing is marked superseded; §12.4's IR listing had drifted from
relational/ir.py(Div,Shift,Defined,Boolwere missing).Three known gaps, recorded not fixed
Added to ROADMAP Track 1 rather than patched here, since each needs a decision:
wherecomparisons on dimension coordinates (where: "snapshot > 0")are accepted eagerly and rejected by lowering. The README's own ramp example
is affected.
sum(p, over=snapshto)builds a model that solves and is wrong: the typo makes the sum a no-op,
and lowering deliberately mirrors that no-op for eager parity. Silent on both
lanes, so the most severe of the three.
**is evaluated eagerly and rejected by lowering; membership isalso dubious, since a variable base breaks degree 1 and a parameters-only
power is data prep.
Verification
146 tests pass; ruff, pre-commit and CI (
native,full 3.11,full 3.13)green; mypy unchanged at 8 pre-existing errors.
Deferred packaging work is filed as #42. Note the correction there: pyarrow
is a runtime dependency — the solver_direct sink reaches it through duckdb's
.to_arrow_reader(), so it never appears as animport pyarrow.🤖 Generated with Claude Code