refactor: the package moves under src/, so CI tests the artifact - #118
Conversation
|
Warning Review limit reached
Next review available in: 44 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 change introduces a src-layout package with YAML parsing, schema validation, expression lowering, compatibility builders, a relational DuckDB executor, LP/HiGHS sinks, public runner APIs, and updated packaging, documentation, CI, and source-tree tests. ChangesYAML relational engine
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant API
participant Lowering
participant DuckdbExecutor
participant HiGHS
Caller->>API: load, build, solve, or write model
API->>Lowering: validate and lower MathSchema
API->>DuckdbExecutor: build relational model from sources
DuckdbExecutor->>DuckdbExecutor: assemble dimensions, variables, constraints, and objective
DuckdbExecutor->>HiGHS: stream columns and constraint rows
HiGHS-->>DuckdbExecutor: return status, objective, and primal values
DuckdbExecutor-->>Caller: return Solution or LP output
Possibly related PRs
🚥 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 |
2257bcd to
6e3cea5
Compare
The `native` CI job exists to prove a packaging claim — bare install, no linopy on disk, the streaming lane still works. Under flat layout it proved that about the *source tree*: `tests/__init__.py` makes pytest's rootdir the repo root and prepend it to `sys.path`, so `linopy_yaml/` in the working directory shadowed site-packages whether or not the install worked at all. Demonstrated by installing the built wheel into a clean venv and running the suite from the repo root: 308 passed, and the package it imported was `<repo>/linopy_yaml/__init__.py`. The wheel was never executed. A subpackage missing an `__init__.py`, or a data file the wheel target does not pick up, would keep CI green and reach a user first. With no importable `linopy_yaml/` at the root there is nothing to shadow, so `uv sync --no-editable` in that job now runs the suite against the wheel. Same probe after the move resolves to site-packages. The sdist becomes the build input and nothing more: `include = ["/src"]`, 996 files down to 33. Two bugs were hiding in the old list. The patterns were gitignore-style and unanchored, so bare `tests/` and `README.md` swept in every nested copy under a build tree or a local worktree. And `tests/` shipped without `examples/`, which `conftest.py` and `test_walkthrough.py` both read — the shipped suite failed 38 tests on FileNotFound before it could check anything. Verification belongs in the repo and CI, not in a tarball where it rots unnoticed. README.md and LICENSE need no entry; hatchling adds the `readme` target and the detected license on top of the list, and the wheel is unchanged at 32 files with its long description and dist-info/licenses/LICENSE intact. `tests/test_architecture.py` keeps reading `src/linopy_yaml` — it lints the source a PR wrote, which is now a different directory from the one the test run imports. That distinction used to hold only by accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6e3cea5 to
bec0f6f
Compare
`license = { text = "MIT" }` and the `License ::` classifier are both
deprecated. The SPDX expression plus `license-files` says the same thing in
the fields that own it, which is also the honest answer to "should the sdist
include list name LICENSE explicitly" — it should not; this is where that
declaration belongs.
Wheel metadata goes from `License: MIT` to `License-Expression: MIT`, the
classifier is gone, `License-File: LICENSE` and dist-info/licenses/LICENSE are
unchanged. `twine check` passes on both artifacts.
hatchling gets a >=1.27 floor: that is the first release with the PEP 639
fields, and without it the new keys fail at build time rather than at parse.
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: 15
🧹 Nitpick comments (7)
src/linopy_yaml/relational/executor.py (1)
469-477: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNULL-bound check rescans the whole
colstable per variable.The count is unfiltered, so building n variables costs n full scans of a table that grows with every one of them. Restrict it to the rows just inserted (
col >= previous _n_cols), which also makes the error message provably aboutv.🤖 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/linopy_yaml/relational/executor.py` around lines 469 - 477, The NULL-bound validation after inserting each variable currently scans all rows in cols, causing repeated full-table scans and potentially attributing earlier rows to the current variable. In the variable-building flow around self._con.execute and self._scalar, capture the prior _n_cols boundary and restrict the count query to rows with col >= that boundary, preserving the existing DataError behavior for missing bounds in v’s newly inserted rows.src/linopy_yaml/piecewise.py (1)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring: mark
curve_adjacencyas emitted only for the non-convex form.
curve_pickcarries "(when not convex)" butcurve_adjacencydoes not, even thoughexpand_piecewiseemits both only underif not pw.convex(Lines 89-92). Reads as if adjacency is always emitted, which contradicts the paragraph right below.📝 Proposed docstring fix
- curve_adjacency(F, bp): curve_lam <= curve_seg + shift(curve_seg, bp=1) + curve_adjacency(F, bp): curve_lam <= curve_seg + shift(curve_seg, bp=1) + (when not convex)🤖 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/linopy_yaml/piecewise.py` around lines 26 - 29, Update the piecewise formulation docstring entry for curve_adjacency to explicitly indicate it is emitted only when the piecewise form is not convex, matching curve_pick and the conditional behavior in expand_piecewise. Leave the adjacency equation unchanged.src/linopy_yaml/compat/loader.py (1)
229-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
DataFrame.stack()withoutfuture_stack=Trueis version-sensitive.Pandas 2.1+ emits a
FutureWarningfor.stack()withoutfuture_stack=True, and pandas 3.0 flipped the default (nowfuture_stack=Falseis what warns). Pinning the kwarg explicitly avoids warnings and pins the intended (no-NaN-dropping) semantics regardless of which pandas version this package resolves to.♻️ Proposed fix
- stacked = cast('pd.Series', raw.stack()) + stacked = cast('pd.Series', raw.stack(future_stack=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/linopy_yaml/compat/loader.py` around lines 229 - 244, Update the DataFrame conversion in the parameter-loading branch to call DataFrame.stack with future_stack explicitly enabled, preserving the existing Series cast and xarray reconstruction flow while avoiding version-dependent behavior and warnings.src/linopy_yaml/expansion.py (1)
197-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
strict=Truewould make the arity invariant self-enforcing.The length check above already guarantees equal lengths, so
strict=Truecosts nothing and turns a future regression into a loud error instead of silently dropped bindings.♻️ Proposed change
- **{formal: _expand(arg, schema, context, stack) for formal, arg in zip(macro.args, call.args, strict=False)}, + **{formal: _expand(arg, schema, context, stack) for formal, arg in zip(macro.args, call.args, strict=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/linopy_yaml/expansion.py` around lines 197 - 200, Update the positional binding comprehension in the macro expansion logic to use strict zipping, changing the visible zip call in the bindings construction to enforce equal lengths. Keep the existing length validation and keyword binding behavior unchanged.src/linopy_yaml/helpers.py (1)
56-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider freezing
BUILTINS.The set is documented as closed, but a plain module-level dict can be mutated at import time by any consumer — exactly the registry the design forbids.
MappingProxyTypemakes the rule enforced rather than stated.As per coding guidelines "Built-in helpers form a closed set; do not add a helper registry."
♻️ Proposed change
-BUILTINS: dict[str, Builtin] = { +_BUILTINS: dict[str, Builtin] = { 'sum': Builtin(1, 'sum(<expr>, over=<dim>)', dimension_kwargs=('over',)), @@ 'shift': Builtin(1, 'shift(<expr>, <dim>=<n>)', dimension_is_key=True), } +BUILTINS: Mapping[str, Builtin] = MappingProxyType(_BUILTINS)🤖 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/linopy_yaml/helpers.py` around lines 56 - 68, Wrap the BUILTINS registry in MappingProxyType so consumers cannot mutate the documented closed set, while preserving its existing Builtin entries and BUILTIN_NAMES derivation. Add the required import and update the BUILTINS declaration without changing lookup behavior.Source: Coding guidelines
src/linopy_yaml/resolution.py (1)
95-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
kind()as aLiteralunion.Both call sites
matchon the returned string; aLiteral['variable', 'parameter', 'dimension'] | Nonereturn lets Pyrefly catch a mistypedcasearm instead of silently routing it tocase _.As per coding guidelines "Keep Pyrefly strict checking at zero errors; fix types rather than widening them."
🤖 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/linopy_yaml/resolution.py` around lines 95 - 103, Update the return annotation of kind() to use Literal['variable', 'parameter', 'dimension'] | None, importing Literal from typing as needed. Keep the existing membership checks and return values unchanged so both match call sites receive the precise union and Pyrefly can validate case arms.Source: Coding guidelines
src/linopy_yaml/where_parser.py (1)
128-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGrammar keywords (
AND/OR/NOT/True/False) are not reserved identifiers.
name(used for both comparison operands and bare-name existence) doesn't exclude the grammar's own keywords. A dimension/parameter named e.g.andortruewould get swallowed bytrue_lit/existencebeforeAND/ORfolding runs, producing a confusing raw pyparsing trace viaSchemaErrorinstead of a clear "reserved name" message. Consider reserving these keywords alongsideBUILTIN_NAMESinschema.py's_validate_references, so the failure is a clear load-time error naming the collision.Also applies to: 137-137
🤖 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/linopy_yaml/where_parser.py` at line 128, Update schema.py’s _validate_references to treat the parser keywords AND, OR, NOT, True, and False as reserved names alongside BUILTIN_NAMES. Ensure dimension and parameter references using any of these names fail during load-time validation with the existing clear collision error, rather than reaching where_parser.py’s name, true_lit, or existence parsing.
🤖 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/linopy_yaml/compat/__init__.py`:
- Around line 138-152: Update the dimension-conflict message in the schema
validation loop around known and existing to identify the actual source of the
comparison: distinguish caller-supplied coords= overrides from coordinates
inferred from the model, and report the corresponding source instead of always
saying “existing model.”
In `@src/linopy_yaml/compat/loader.py`:
- Around line 133-193: Preserve boolean dtype for scalar parameters throughout
`_coerce_to_dataarray` and `load_parameters`: handle bool values before numeric
scalar coercion, and broadcast scalar arrays using their original dtype instead
of always converting through `float`. Ensure scalar boolean parameters reach
`_eval_node` for `ParameterDefinedNode` with `arr.dtype == bool`, while numeric
scalar behavior remains unchanged.
In `@src/linopy_yaml/dimensions.py`:
- Around line 231-233: Update objective handling across the dimensions check,
builder, and relational lowering to avoid silently ignoring equations beyond
odef.equations[0]. Either explicitly reject objectives whose equations length is
not exactly one, or consistently iterate and process every objective equation in
all three paths; apply the same behavior across validation and lowering.
In `@src/linopy_yaml/expression_parser.py`:
- Around line 149-153: The number grammar in real and number must accept
exponent-only numerals such as 1e-3 and 1e3. Update the real-number pattern or
add a dedicated exponent form while preserving NumberNode(float(...)) conversion
and existing integer, decimal, and infinity parsing.
In `@src/linopy_yaml/piecewise.py`:
- Around line 157-161: Update the link-expression validation flow around
parse_expression and resolve_expression to call expansion.parse_and_expand(text,
schema, ctx) first, matching the pipeline used by active and other emitted
expressions. Apply the existing ComparisonNode/core-subset checks to the
expanded AST, while preserving the current error collection and context
handling.
- Around line 104-120: Make frame construction in the piecewise expansion flow
deterministic: when processing dimensions returned by _expr_dims, order them
according to schema.dimensions rather than iterating the frozenset directly.
Preserve the existing validation and deduplication behavior in the surrounding
piecewise expansion logic, including exclusion of pw.over.
In `@src/linopy_yaml/relational/arrow.py`:
- Around line 79-81: Add a concise rationale to the existing Pyrefly suppression
on the `pa.array(list(values))` expression, explaining why the reported
`bad-argument-type` finding is incorrect or unavoidable. Keep the suppression
scoped to that line and follow the explanatory style used in `compiler.py`.
- Around line 56-65: Update the pandas Series handling in as_table so an index
with unnamed levels returns None when dims is absent or does not provide
matching dimension names, avoiding rename_axis with an empty or insufficient
dims sequence. Preserve the existing rename_axis and reset_index behavior when
valid matching dims are supplied, allowing the caller to emit its documented
error.
In `@src/linopy_yaml/relational/compiler.py`:
- Around line 341-349: The _comparison_sql function must escape apostrophes in
string literals before wrapping them in SQL quotes. Update its literal
construction to replace each single quote in string values with two single
quotes, while preserving numeric formatting and the existing operator mapping.
- Around line 75-89: Update SqlCompiler.frame to handle an empty dims tuple
before accessing dims[0]. Either return a valid one-row scalar frame equivalent
to SELECT 1, or raise an actionable LanguageError; do not allow an IndexError to
escape. Preserve the existing FROM/WHERE/order-key behavior for non-empty
dimensions.
In `@src/linopy_yaml/relational/executor.py`:
- Around line 519-525: The objective constant accumulation in the executor loop
must not silently discard rows from dimensionless parameter fragments. Update
the `_scalar` handling used by the `comp.consts` path to either aggregate all
`cval` rows with SUM or validate that exactly one row exists and raise a clear
error otherwise, while preserving the existing scalar fallback for empty
results.
- Around line 267-269: Add one shared SQL string-literal escaping helper and use
it for every filesystem path interpolated into SQL: update _source_relation and
the executor sites at lines 182, 355, and 574, plus COPY targets at lines 40,
58, 69, and 79 in lp_file.py. Ensure paths such as sources, temp_directory,
workdir, and output targets escape single quotes before interpolation, while
leaving identifier handling unchanged.
In `@src/linopy_yaml/relational/sinks/highs.py`:
- Around line 82-89: Guard the solution-table construction in the HiGHS solve
flow before creating `primal`/`sol`: compare `h.getSolution().col_value` length
with `model.column_count`, and skip building the table when no primal solution
is available. Preserve the documented `(status, objective)` return path for
infeasible or unbounded solves while retaining normal table construction for
valid primal solutions.
In `@src/linopy_yaml/relational/sinks/lp_file.py`:
- Line 40: Update the COPY target construction in the relevant export functions
around the object, constraint, bound, and coefficient writes to escape
caller-supplied paths before embedding them in SQL. Reuse the existing
path-escaping helper from executor.py, ensuring every COPY TO target handles
quotes in model.workdir consistently.
In `@src/linopy_yaml/relational/sinks/README.md`:
- Around line 3-4: Update the relative links in the sinks README to point four
levels up: change the ARCHITECTURE.md reference to ../../../../ARCHITECTURE.md
and the ROADMAP.md reference to
../../../../ROADMAP.md#track-4--sink-capabilities. Preserve the surrounding
documentation text.
---
Nitpick comments:
In `@src/linopy_yaml/compat/loader.py`:
- Around line 229-244: Update the DataFrame conversion in the parameter-loading
branch to call DataFrame.stack with future_stack explicitly enabled, preserving
the existing Series cast and xarray reconstruction flow while avoiding
version-dependent behavior and warnings.
In `@src/linopy_yaml/expansion.py`:
- Around line 197-200: Update the positional binding comprehension in the macro
expansion logic to use strict zipping, changing the visible zip call in the
bindings construction to enforce equal lengths. Keep the existing length
validation and keyword binding behavior unchanged.
In `@src/linopy_yaml/helpers.py`:
- Around line 56-68: Wrap the BUILTINS registry in MappingProxyType so consumers
cannot mutate the documented closed set, while preserving its existing Builtin
entries and BUILTIN_NAMES derivation. Add the required import and update the
BUILTINS declaration without changing lookup behavior.
In `@src/linopy_yaml/piecewise.py`:
- Around line 26-29: Update the piecewise formulation docstring entry for
curve_adjacency to explicitly indicate it is emitted only when the piecewise
form is not convex, matching curve_pick and the conditional behavior in
expand_piecewise. Leave the adjacency equation unchanged.
In `@src/linopy_yaml/relational/executor.py`:
- Around line 469-477: The NULL-bound validation after inserting each variable
currently scans all rows in cols, causing repeated full-table scans and
potentially attributing earlier rows to the current variable. In the
variable-building flow around self._con.execute and self._scalar, capture the
prior _n_cols boundary and restrict the count query to rows with col >= that
boundary, preserving the existing DataError behavior for missing bounds in v’s
newly inserted rows.
In `@src/linopy_yaml/resolution.py`:
- Around line 95-103: Update the return annotation of kind() to use
Literal['variable', 'parameter', 'dimension'] | None, importing Literal from
typing as needed. Keep the existing membership checks and return values
unchanged so both match call sites receive the precise union and Pyrefly can
validate case arms.
In `@src/linopy_yaml/where_parser.py`:
- Line 128: Update schema.py’s _validate_references to treat the parser keywords
AND, OR, NOT, True, and False as reserved names alongside BUILTIN_NAMES. Ensure
dimension and parameter references using any of these names fail during
load-time validation with the existing clear collision error, rather than
reaching where_parser.py’s name, true_lit, or existence parsing.
🪄 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: 46ce7ffa-9854-47b7-9abb-21a46902cbbe
📒 Files selected for processing (34)
.github/workflows/ci.ymlARCHITECTURE.mdCLAUDE.mdpyproject.tomlsrc/linopy_yaml/__init__.pysrc/linopy_yaml/_notes.pysrc/linopy_yaml/_yaml.pysrc/linopy_yaml/api.pysrc/linopy_yaml/compat/__init__.pysrc/linopy_yaml/compat/builder.pysrc/linopy_yaml/compat/loader.pysrc/linopy_yaml/dimensions.pysrc/linopy_yaml/errors.pysrc/linopy_yaml/expansion.pysrc/linopy_yaml/expression_parser.pysrc/linopy_yaml/helpers.pysrc/linopy_yaml/lowering.pysrc/linopy_yaml/piecewise.pysrc/linopy_yaml/relational/__init__.pysrc/linopy_yaml/relational/arrow.pysrc/linopy_yaml/relational/compiler.pysrc/linopy_yaml/relational/executor.pysrc/linopy_yaml/relational/plan.pysrc/linopy_yaml/relational/sinks/README.mdsrc/linopy_yaml/relational/sinks/__init__.pysrc/linopy_yaml/relational/sinks/highs.pysrc/linopy_yaml/relational/sinks/lp_file.pysrc/linopy_yaml/relational/sinks/tables.pysrc/linopy_yaml/resolution.pysrc/linopy_yaml/schema.pysrc/linopy_yaml/validation.pysrc/linopy_yaml/where_parser.pytests/test_architecture.pytests/test_doc_examples.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.
🛑 Comments failed to post (15)
src/linopy_yaml/compat/__init__.py (1)
138-152: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Error message says "existing model" even when the conflict is against the
coords=override, not the model.
knownis built from_infer_coords(model)and then overwritten by anycoords=entries (lines 132-134) before this check runs. If the mismatch is actually against a caller-suppliedcoords=value rather than the model's real coordinates, the raised message ("differ from the existing model") misattributes the source of truth and will mislead whoever debugs it.✏️ Proposed message fix
+ source = 'coords= override' if coords is not None and dim_name in coords else 'existing model' msg = ( f"Extension declares dimension '{dim_name}' with values " f'that differ from the existing model.\n' - f' Existing: {list(existing)}\n' + f' {source.capitalize()}: {list(existing)}\n' f' Declared: {list(declared)}\n'🤖 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/linopy_yaml/compat/__init__.py` around lines 138 - 152, Update the dimension-conflict message in the schema validation loop around known and existing to identify the actual source of the comparison: distinguish caller-supplied coords= overrides from coordinates inferred from the model, and report the corresponding source instead of always saying “existing model.”src/linopy_yaml/compat/loader.py (1)
133-193: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scalar boolean parameters silently become floats, breaking
ParameterDefinedNodemask semantics.Two compounding bugs:
_coerce_to_dataarray(line 203) checksisinstance(raw, (int, float, ...))before any bool-specific handling. Sinceboolsubclassesintin Python, a scalarTrue/Falsematches this branch and becomesxr.DataArray(float(raw))— dtype float, not bool.- Even if that were fixed,
load_parameters's scalar-broadcast branch (lines 176-184) unconditionally doesscalar_val = float(arr.values)and builds the broadcast array as float, regardless of the original dtype — so theelif arr.dtype == bool:branch right below (line 185) can never fire for a scalar.Downstream,
builder.py's_eval_nodeforParameterDefinedNode(evaluates a "parameter defines a mask" where-clause) special-casesarr.dtype == boolto read it as a literal True/False mask; otherwise it falls back tonotnull() & isfinite(), which is alwaysTruefor both0.0and1.0. A scalar boolean parameter passed throughdata=therefore can never evaluate toFalsein a where-mask — it always "counts."🐛 Proposed fix
def _coerce_to_dataarray( name: str, raw: Any, dims: list[str], master_coords: dict[str, pd.Index], ) -> xr.DataArray: """Coerce a user-provided value into an xr.DataArray.""" + # bool is a subclass of int in Python — check it first, or a boolean + # scalar silently becomes 1.0/0.0 below. + if isinstance(raw, bool): + return xr.DataArray(raw) # Scalar if isinstance(raw, (int, float, np.integer, np.floating)): return xr.DataArray(float(raw))if arr.ndim == 0: # Broadcast scalar to full shape over declared dims - scalar_val = float(arr.values) shape = tuple(len(master_coords[d]) for d in pdef.dims) arr = xr.DataArray( - np.full(shape, scalar_val), + np.full(shape, arr.values, dtype=arr.dtype), dims=pdef.dims, coords=reindex_coords, )Also applies to: 195-276
🤖 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/linopy_yaml/compat/loader.py` around lines 133 - 193, Preserve boolean dtype for scalar parameters throughout `_coerce_to_dataarray` and `load_parameters`: handle bool values before numeric scalar coercion, and broadcast scalar arrays using their original dtype instead of always converting through `float`. Ensure scalar boolean parameters reach `_eval_node` for `ParameterDefinedNode` with `arr.dtype == bool`, while numeric scalar behavior remains unchanged.src/linopy_yaml/dimensions.py (1)
231-233: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check whether the objective-building/lowering path consumes more than equations[0]. rg -n 'equations\[0\]|equations\b' src/linopy_yaml/compat/builder.py src/linopy_yaml/lowering.py 2>/dev/nullRepository: FBumann/linopy-yaml
Length of output: 878
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== files ==" git ls-files src/linopy_yaml | sed -n '1,200p' echo "== schema relevant sections ==" fd -a 'schema.py|dimensions.py|builder.py|lowering.py' src/linopy_yaml | while read -r f; do echo "--- $f ($(wc -l < "$f") lines) ---" done echo "== schema.py objective/constraint definitions ==" rg -n "class ObjectiveBlock|class ConstraintBlock|equations|_at_least_one|Field|Validator|validate" src/linopy_yaml/schema.py -C 4 echo "== dimensions.py relevant section ==" sed -n '190,250p' src/linopy_yaml/dimensions.py | nl -ba -v190 echo "== builder.py objective section ==" sed -n '160,235p' src/linopy_yaml/compat/builder.py | nl -ba -v160 echo "== lowering.py objective and doc ==" sed -n '1,35p;90,140p' src/linopy_yaml/lowering.py | nl -ba -v1Repository: FBumann/linopy-yaml
Length of output: 3806
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== dimensions.py relevant section ==" sed -n '190,250p' src/linopy_yaml/dimensions.py sed -n '220,235p' src/linopy_yaml/dimensions.py echo "== builder.py objective section ==" sed -n '160,235p' src/linopy_yaml/compat/builder.py echo "== lowering.py objective section and comments ==" sed -n '1,35p' src/linopy_yaml/lowering.py sed -n '90,140p' src/linopy_yaml/lowering.py echo "== all objective equation references ==" rg -n "odef\\.equations|equations\\[0\\]|equations\\]" src -SRepository: FBumann/linopy-yaml
Length of output: 11304
Validate and reject/multi-target objective equations in all lanes
schema.pyacceptsObjectiveBlock.equationsas a non-empty list, and the constraint path processes every equation. The dims-check, builder, and relational lowering each read onlyodef.equations[0], so declared additional objective equations can pass validation and then be silently dropped by both lanes. Either reject objectives withlen(odef.equations) != 1, or process/build/lower every equation consistently like constraints.🤖 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/linopy_yaml/dimensions.py` around lines 231 - 233, Update objective handling across the dimensions check, builder, and relational lowering to avoid silently ignoring equations beyond odef.equations[0]. Either explicitly reject objectives whose equations length is not exactly one, or consistently iterate and process every objective equation in all three paths; apply the same behavior across validation and lowering.src/linopy_yaml/expression_parser.py (1)
149-153: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Locate expression_parser.py" fd -a 'expression_parser.py' . || true echo echo "Relevant parser section" if [ -f src/linopy_yaml/expression_parser.py ]; then sed -n '1,260p' src/linopy_yaml/expression_parser.py | cat -n fi echo echo "Search parse_expression and callers for parse_all/SchemaError" rg -n "parse_all|parse_expression|SchemaError|expression_parser" src tests 2>/dev/null || trueRepository: FBumann/linopy-yaml
Length of output: 14705
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Inspect test_parser.py" sed -n '1,200p' tests/test_parser.py | cat -n echo echo "Check runtime behavior without importing repository packages" python3 - <<'PY' import re regexes = { "integer": r"-?\d+", "real": r"-?\d+\.\d*([eE][+-]?\d+)?", "inf_literal": r"[-]?inf", # approximate from Literal('.inf') | Literal('inf') } samples = ["1e-3", "1e3", "0.5e-3", "1.0e-3", "1e-3 + 2", "1.0"] for s in samples: print(f"\nINPUT: {s!r}") for name, pat in regexes.items(): for m in re.finditer(pat, s): print(f" {name}: matched {m.group()!r} at {m.start()}-{m.end()}") try: print(f" float: {float(m.group())}") except Exception as e: print(f" float: {type(e).__name__}: {e}") PY echo echo "Check installed pyparsing version" python3 - <<'PY' try: import pyparsing as pp print(pp.__version__) grammar = ( pp.Regex(r"-?\d+\.\d*([eE][+-]?\d+)?") | pp.Literal('.inf') | pp.Literal('inf') | pp.Regex(r"-?\d+") ) for sample in ["1e-3", "1e3", "1.0", "sum(x, over=1e-3)"]: try: result = grammar.parse_string(sample, parse_all=True) print(repr(sample), "=>", result) except Exception as e: print(repr(sample), "=>", type(e).__name__, str(e).splitlines()[0][:160].replace('\n','\\n')) except Exception as e: print("pyparsing probe failed:", type(e).__name__, e) PYRepository: FBumann/linopy-yaml
Length of output: 6544
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Read parser error path and tests with full context" sed -n '240,255p' src/linopy_yaml/expression_parser.py | cat -n sed -n '78,84p' tests/test_parser.py | cat -n echo echo "Check pyparsing exception base classes" python3 - <<'PY' import pyparsing as pp print("ParseException is ValueError?", issubclass(pp.ParseException, ValueError)) PYRepository: FBumann/linopy-yaml
Length of output: 1239
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' import pyparsing as pp try: print("SchemaError available as ValueError?", issubclass(pp.ParseException, ValueError)) except Exception as e3: print("subclass check failed:", e3) print([e for e in pp.ParseException.__mro__ if e.__module__ in {"exceptions", "builtins"}]) # Demonstrate parser result and exception text shape; no repository code executed. grammar = ( pp.Regex(r"-?\d+\.\d*([eE][+-]?\d+)?") | pp.Literal('.inf') | pp.Literal('inf') | pp.Regex(r"-?\d+") ) for sample in ["1e-3", "sum(x, over=1e-3)"]: try: result = grammar.parse_string(sample, parse_all=True) print("sample", repr(sample), "success", result) except Exception as e: print("sample", repr(sample), "class", e.__class__.__module__ + "." + e.__class__.__qualname__, "text", repr(str(e))) PYRepository: FBumann/linopy-yaml
Length of output: 590
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Inspect errors.py exception hierarchy" sed -n '1,130p' src/linopy_yaml/errors.py | cat -n echo echo "Inspect parser failure tests to see whether current SchemaError is expected" rg -n "Failed to parse|SchemaError|pytest.raises" tests src/linopy_yaml -g '*.py' | head -80Repository: FBumann/linopy-yaml
Length of output: 11799
Accept exponent-only numerals or document them as unsupported.
The current
realregex only accepts bare floating-point mantissas and mantissa-plus-exponent forms such as1.0e-3; inputs like1e-3or1e3failparse_alland return invalid remaining text, e.g.e3. Add the missing exponent-only case to the number grammar if it should be available.🤖 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/linopy_yaml/expression_parser.py` around lines 149 - 153, The number grammar in real and number must accept exponent-only numerals such as 1e-3 and 1e3. Update the real-number pattern or add a dedicated exponent form while preserving NumberNode(float(...)) conversion and existing integer, decimal, and infinity parsing.src/linopy_yaml/piecewise.py (2)
104-120: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
frameorder is nondeterministic — it is built by iterating afrozenset.
_expr_dimsreturnsfrozenset[str](Line 145) and Line 114 iterates it directly, so the order dims land inframevaries between processes (string hashing is randomized). That order becomes theforeachlist of every emitted λ/seg variable and constraint (Lines 64, 69, 76, 81, 86, 90), which in turn drives the CROSS JOIN order and therefore LP row/column numbering — the same YAML can produce byte-different LP files across runs, undermining the differential-testing goal stated in the module docstring.Suggest ordering deterministically, e.g. by declaration order in
schema.dimensions.🔧 Proposed deterministic ordering
- for d in _expr_dims(schema, expr_text, f'{ctx} link {i}'): + for d in sorted(_expr_dims(schema, expr_text, f'{ctx} link {i}'), key=_dim_order(schema)):(or collect all dims first and emit
tuple(d for d in schema.dimensions if d in collected))🤖 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/linopy_yaml/piecewise.py` around lines 104 - 120, Make frame construction in the piecewise expansion flow deterministic: when processing dimensions returned by _expr_dims, order them according to schema.dimensions rather than iterating the frozenset directly. Preserve the existing validation and deduplication behavior in the surrounding piecewise expansion logic, including exclusion of pw.over.
157-161: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Do any tests or docs use a macro / named expression inside a piecewise link? rg -nP -C4 'piecewise' --iglob '*.yml' --iglob '*.yaml' --iglob '*.py' --iglob '*.md' | rg -n -C4 'links' ast-grep run --pattern 'parse_and_expand($$$)' --lang python srcRepository: FBumann/linopy-yaml
Length of output: 157
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== files ==" git ls-files | rg '(^|/)piecewise\.py$|(^|/)expansion|(^|/)resolution|(^|/)validation|(^|/)lowering' echo "== piecewise outline ==" ast-grep outline src/linopy_yaml/piecewise.py --view expanded || true echo "== piecewise relevant lines ==" nl -ba src/linopy_yaml/piecewise.py | sed -n '1,240p' echo "== parse_and_expand definition/usages ==" rg -n 'def parse_and_expand|parse_and_expand\(' src tests docs -S || true rg -n 'def expression_of|def _parse_expand|_parse_expand|expand' src/linopy_yaml -S || trueRepository: FBumann/linopy-yaml
Length of output: 868
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== piecewise.py ==" python3 - <<'PY' from pathlib import Path p = Path('src/linopy_yaml/piecewise.py') for i, line in enumerate(p.read_text().splitlines(), 1): if 1 <= i <= 260: print(f"{i:4}: {line}") PY echo "== expansion.py outline/relevant ==" python3 - <<'PY' from pathlib import Path p = Path('src/linopy_yaml/expansion.py') lines = p.read_text().splitlines() print("outline-like function/class defs:") for i, line in enumerate(lines, 1): if any(s in line for s in ('def ', 'class ')): print(f"{i:4}: {line}") print("\nfirst 260 lines:") for i, line in enumerate(lines, 1): if 1 <= i <= 260: print(f"{i:4}: {line}") PY echo "== resolution.py relevant ==" python3 - <<'PY' from pathlib import Path p = Path('src/linopy_yaml/resolution.py') for i, line in enumerate(p.read_text().splitlines(), 1): if 1 <= i <= 260: print(f"{i:4}: {line}") PY echo "== validation.py relevant ==" python3 - <<'PY' from pathlib import Path p = Path('src/linopy_yaml/validation.py') for i, line in enumerate(p.read_text().splitlines(), 1): if 1 <= i <= 280: print(f"{i:4}: {line}") PY echo "== lowering.py relevant ==" python3 - <<'PY' from pathlib import Path p = Path('src/linopy_yaml/lowering.py') for i, line in enumerate(p.read_text().splitlines(), 1): if 1 <= i <= 260: print(f"{i:4}: {line}") PY echo "== macro/named expr usages ==" rg -n "macros|expressions:|parse_and_expand\(|resolve_expression\(|parse_expression\(" src tests -S || trueRepository: FBumann/linopy-yaml
Length of output: 50375
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== parse_and_expand usages ==" rg -n 'parse_and_expand\(' src tests -S || true echo "== tests mentioning piecewise links/macro/named expression ==" rg -n -C3 'piecewise|links|macro|macros|expressions:|expand_piecewise|expansion' tests src/linopy_yaml --iglob '*.py' --iglob '*.yml' --iglob '*.yaml' --iglob '*.md' || true echo "== deterministic AST probe: macro/named expansion is skipped during piecewise _expr_dims ==" python3 - <<'PY' from pathlib import Path import ast src = Path('src/linopy_yaml/piecewise.py').read_text() ns = {} exec(src, ns) fn = ns['_expr_dims'] import ast as ast_mod print('_expr_dims source:') lines = src.splitlines() for i in range(144, 172): print(f"{i+1}: {lines[i]}") # Minimal semantic check: _expr_dims imports expression_parser.parse_expression first tree = ast_mod.parse(src) for node in ast_mod.walk(tree): if isinstance(node, ast_mod.FunctionDef) and node.name == '_expr_dims': imports = [call for call in ast_mod.walk(node) if isinstance(call, ast_mod.Call) and isinstance(call.func, ast_mod.Attribute) and call.func.attr == 'from_import'] print("imports inside _expr_dims:", [(type(call.func.value.id).__name__ if hasattr(call.func, 'value') else None, call.func.attr) for call in nodes :=[]]) PYRepository: FBumann/linopy-yaml
Length of output: 50375
Align piecewise link validation with the expansion flow.
_expr_dims()resolvesparse_expression()AST directly, so a link text that is a macro or named expression is rejected here as “not found” before expansion would translate it into core math. Useexpansion.parse_and_expand(text, schema, ctx)before the existing core-subset checks so links follow the same pipeline asactiveand other emitted expressions.🤖 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/linopy_yaml/piecewise.py` around lines 157 - 161, Update the link-expression validation flow around parse_expression and resolve_expression to call expansion.parse_and_expand(text, schema, ctx) first, matching the pipeline used by active and other emitted expressions. Apply the existing ComparisonNode/core-subset checks to the expanded AST, while preserving the current error collection and context handling.src/linopy_yaml/relational/arrow.py (2)
56-65: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Confirm pandas raises when rename_axis is given a name list shorter than nlevels pip install --quiet pandas >/dev/null 2>&1 python - <<'PY' import pandas as pd s = pd.Series([1.0, 2.0]) for names in [(), ('a','b')]: try: s.rename_axis(names) print(names, 'ok') except Exception as e: print(names, type(e).__name__, e) PYRepository: FBumann/linopy-yaml
Length of output: 267
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate and inspect the relevant implementation and call site without executing repository code. printf '--- arrow.py outline ---\n' ast-grep outline src/linopy_yaml/relational/arrow.py || true printf '\n--- arrow.py around as_table ---\n' sed -n '1,120p' src/linopy_yaml/relational/arrow.py printf '\n--- executor.py call site around 250-285 ---\n' sed -n '250,285p' src/linopy_yaml/relational/executor.py printf '\n--- any as_table callers in relational ---\n' rg -n "as_table\(" src/linopy_yaml/relational -g '*.py'Repository: FBumann/linopy-yaml
Length of output: 248
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- arrow.py outline ---' ast-grep outline src/linopy_yaml/relational/arrow.py || true echo echo '--- arrow.py around as_table ---' sed -n '1,120p' src/linopy_yaml/relational/arrow.py echo echo '--- executor.py call site around 250-285 ---' sed -n '250,285p' src/linopy_yaml/relational/executor.py echo echo '--- as_table callers in relational ---' rg -n "as_table\(" src/linopy_yaml/relational -g '*.py'Repository: FBumann/linopy-yaml
Length of output: 6028
Handle unnamed Series indexes without calling
rename_axiswith the default emptydims.
executor.pycallsas_table(source)with nodims, so apandas.Serieswith one unnamed index level reachesobj.rename_axis(()), which raisesValueError. ReturnNonehere unless matching dimension names are supplied so the caller can produce the documented error message instead of this internal-boundary crash.🛡️ Proposed guard
- if any(n is None for n in obj.index.names): + if dims and len(dims) == obj.index.nlevels and any(n is None for n in obj.index.names): obj = obj.rename_axis(dims) + elif any(n is None for n in obj.index.names): + return None📝 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.pd = sys.modules.get('pandas') if pd is not None and isinstance(obj, pd.Series): # a Series exposes a capsule too, but it describes the values alone — # the index, which holds the dims, has to be promoted to columns first. # Levels the caller named are left alone and bind by name: overwriting # them with *dims* transposes the data silently when two dims share a # label space, and nothing downstream can catch that. if dims and len(dims) == obj.index.nlevels and any(n is None for n in obj.index.names): obj = obj.rename_axis(dims) elif any(n is None for n in obj.index.names): return None obj = obj.rename('value').reset_index()🤖 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/linopy_yaml/relational/arrow.py` around lines 56 - 65, Update the pandas Series handling in as_table so an index with unnamed levels returns None when dims is absent or does not provide matching dimension names, avoiding rename_axis with an empty or insufficient dims sequence. Preserve the existing rename_axis and reset_index behavior when valid matching dims are supplied, allowing the caller to emit its documented error.
79-81: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Explain the Pyrefly suppression.
# pyrefly: ignore[bad-argument-type]carries no rationale here, unlike the suppression insrc/linopy_yaml/relational/compiler.pyLines 319-321 which states why. As per coding guidelines: "If a finding is genuinely wrong, suppress only that line with# pyrefly: ignore[rule-name]and explain why".📝 Proposed comment
+ # `values` is typed `object` on purpose — the caller may hand over any + # sequence of labels, and `list()` is exactly the widening check here. try: return pa.table({dname: pa.array(list(values))}) # pyrefly: ignore[bad-argument-type]📝 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.# `values` is typed `object` on purpose — the caller may hand over any # sequence of labels, and `list()` is exactly the widening check here. try: return pa.table({dname: pa.array(list(values))}) # pyrefly: ignore[bad-argument-type] except (TypeError, pa.ArrowInvalid) as exc:🤖 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/linopy_yaml/relational/arrow.py` around lines 79 - 81, Add a concise rationale to the existing Pyrefly suppression on the `pa.array(list(values))` expression, explaining why the reported `bad-argument-type` finding is incorrect or unavoidable. Keep the suppression scoped to that line and follow the explanatory style used in `compiler.py`.Source: Coding guidelines
src/linopy_yaml/relational/compiler.py (2)
75-89: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Is a scalar (empty foreach) declaration exercised anywhere, and is it rejected earlier? rg -nP -C3 'foreach:\s*\[\s*\]' --iglob '*.yml' --iglob '*.yaml' --iglob '*.py' . rg -nP -C4 'foreach' src/linopy_yaml/schema.py rg -nP -C4 'def frame\(' -A20 src/linopy_yaml/relational/executor.pyRepository: FBumann/linopy-yaml
Length of output: 2404
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== compiler frame/predicate snippets ==" sed -n '1,140p' src/linopy_yaml/relational/compiler.py echo echo "== piecewise expand snippets ==" rg -n -C 5 "expand_piecewise|foreach.*list|frame" src/linopy_yaml/piecewise.py src/linopy_yaml echo echo "== schema foreach validator snippets ==" sed -n '300,370p' src/linopy_yaml/schema.py echo echo "== tests around foreach empty ==" sed -n '60,100p' tests/test_yaml_loading.py sed -n '70,105p' tests/test_error_notes.pyRepository: FBumann/linopy-yaml
Length of output: 50375
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== dimension validation exact sections ==" sed -n '170,235p' src/linopy_yaml/dimensions.py echo echo "== schema field_validator exact sections ==" sed -n '120,185p' src/linopy_yaml/schema.py sed -n '338,360p' src/linopy_yaml/schema.py echo echo "== lower_program declaration lowering ==" sed -n '68,145p' src/linopy_yaml/lowering.py echo echo "== compiler usages of frame ==" rg -n -C3 "\.frame\(" src/linopy_yaml/relational/executor.py src/linopy_yaml/relational/compiler.py echo echo "== relevant tests around empty foreach / piecewise ==" sed -n '80,98p' tests/test_error_notes.py rg -n -C4 "piecewise|empty|dimensionless|scalar|foreach.*\[\s*\]" tests src/linopy_yaml --iglob '*.py' | sed -n '1,240p'Repository: FBumann/linopy-yaml
Length of output: 26148
Handle empty dimension frames explicitly.
SqlCompiler.frame()crashes here whendims=(), and relationalframe()is a valid compiler concern even if the current dim checker rejects the generated schema-levelforeach: []cases first. Either produce a one-row scalar frame (SELECT 1) or raise an actionableLanguageErrorinstead of letting anIndexErrorescape.🤖 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/linopy_yaml/relational/compiler.py` around lines 75 - 89, Update SqlCompiler.frame to handle an empty dims tuple before accessing dims[0]. Either return a valid one-row scalar frame equivalent to SELECT 1, or raise an actionable LanguageError; do not allow an IndexError to escape. Preserve the existing FROM/WHERE/order-key behavior for non-empty dimensions.
341-349: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Single quotes in string literals are not escaped.
f"'{value}'"breaks (or lets the caller alter) the generated SQL for any label containing an apostrophe — e.g.where: "bus == 'O'Brien'". Since the value comes straight from user YAML, double the quote as SQL requires.🔒️ Proposed fix
- literal = f"'{value}'" if isinstance(value, str) else repr(value) + literal = "'" + value.replace("'", "''") + "'" if isinstance(value, str) else repr(value)📝 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 _comparison_sql(column: str, op: plan.ComparisonOperator, value: float | str) -> str: """One where-comparison: ``(<column> <op> <literal>)``. The language's ``==`` is SQL's ``=``, and a string literal needs quoting — stated once, since the parameter and dimension cases differ only in which column they test. """ literal = "'" + value.replace("'", "''") + "'" if isinstance(value, str) else repr(value) return f'({column} {"=" if op == "==" else op} {literal})'🤖 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/linopy_yaml/relational/compiler.py` around lines 341 - 349, The _comparison_sql function must escape apostrophes in string literals before wrapping them in SQL quotes. Update its literal construction to replace each single quote in string values with two single quotes, while preserving numeric formatting and the existing operator mapping.src/linopy_yaml/relational/executor.py (2)
267-269: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Filesystem paths are interpolated into SQL literals without escaping. Declared names go through
_IDENT, but paths (sources[...],workdir, output targets) do not — a single quote in any of them terminates the literal and injects SQL. One shared escaping helper fixes every site.
src/linopy_yaml/relational/executor.py#L267-L269: escape the parquet path in_source_relation, and apply the same helper at Line 182 (temp_directory), Line 355 (read_parquet(..., file_row_number=true)) and Line 574 (COPY … TO).src/linopy_yaml/relational/sinks/lp_file.py#L40-L40: escape theCOPY … TOtargets at Lines 40, 58, 69 and 79 with the same helper.📍 Affects 2 files
src/linopy_yaml/relational/executor.py#L267-L269(this comment)src/linopy_yaml/relational/sinks/lp_file.py#L40-L40🤖 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/linopy_yaml/relational/executor.py` around lines 267 - 269, Add one shared SQL string-literal escaping helper and use it for every filesystem path interpolated into SQL: update _source_relation and the executor sites at lines 182, 355, and 574, plus COPY targets at lines 40, 58, 69, and 79 in lp_file.py. Ensure paths such as sources, temp_directory, workdir, and output targets escape single quotes before interpolation, while leaving identifier handling unchanged.Source: Linters/SAST tools
519-525: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_scalarsilently takes the first row of a dimensionless const fragment.For a dimensionless
Parameter,p.sqlisSELECT value AS cval FROM p_<name>; if the bound source has more than one row, only the first contributes to_obj_constwith no diagnostic. ConsiderSELECT SUM(cval) FROM (…)or asserting single-rowness.🤖 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/linopy_yaml/relational/executor.py` around lines 519 - 525, The objective constant accumulation in the executor loop must not silently discard rows from dimensionless parameter fragments. Update the `_scalar` handling used by the `comp.consts` path to either aggregate all `cval` rows with SUM or validate that exactly one row exists and raise a clear error otherwise, while preserving the existing scalar fallback for empty results.src/linopy_yaml/relational/sinks/highs.py (1)
82-89: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Infeasible/unbounded solves can crash instead of returning a status.
h.getSolution().col_valueis empty when HiGHS has no primal solution, while thecolarray is alwayscolumn_countlong —pa.tablethen raises on mismatched lengths, so the documented(status, objective)return is unreachable exactly when the caller most needs the status. Guard on the length before buildingsol.🛡️ Suggested guard
- primal = pa.table( - { - 'col': pa.array(np.arange(model.column_count, dtype=np.int64)), - 'value': pa.array(np.asarray(h.getSolution().col_value, dtype=np.float64)), - } - ) + values = np.asarray(h.getSolution().col_value, dtype=np.float64) + if len(values) != model.column_count: # no primal solution (infeasible/unbounded) + values = np.full(model.column_count, np.nan) + primal = pa.table( + { + 'col': pa.array(np.arange(model.column_count, dtype=np.int64)), + 'value': pa.array(values), + } + )📝 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.import pyarrow as pa values = np.asarray(h.getSolution().col_value, dtype=np.float64) if len(values) != model.column_count: # no primal solution (infeasible/unbounded) values = np.full(model.column_count, np.nan) primal = pa.table( { 'col': pa.array(np.arange(model.column_count, dtype=np.int64)), 'value': pa.array(values), } )🤖 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/linopy_yaml/relational/sinks/highs.py` around lines 82 - 89, Guard the solution-table construction in the HiGHS solve flow before creating `primal`/`sol`: compare `h.getSolution().col_value` length with `model.column_count`, and skip building the table when no primal solution is available. Preserve the documented `(status, objective)` return path for infeasible or unbounded solves while retaining normal table construction for valid primal solutions.src/linopy_yaml/relational/sinks/lp_file.py (1)
40-40: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
COPY … TO '{path}'inherits the unescaped-path problem.
model.workdiris caller-supplied (build(workdir=...)), so a quote anywhere in it breaks everyCOPYtarget here (Lines 40, 58, 69, 79). Same escaping helper as suggested insrc/linopy_yaml/relational/executor.py.🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 40-40: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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/linopy_yaml/relational/sinks/lp_file.py` at line 40, Update the COPY target construction in the relevant export functions around the object, constraint, bound, and coefficient writes to escape caller-supplied paths before embedding them in SQL. Reuse the existing path-escaping helper from executor.py, ensuring every COPY TO target handles quotes in model.workdir consistently.src/linopy_yaml/relational/sinks/README.md (1)
3-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash fd -H -t f 'ARCHITECTURE.md|ROADMAP.md' -d 3 rg -n '\]\(\.\./' src/linopy_yaml --glob '*.md'Repository: FBumann/linopy-yaml
Length of output: 419
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import PurePosixPath paths = [ "src/linopy_yaml/relational/sinks/README.md", "../../../ARCHITECTURE.md", "../../../ROADMAP.md", "../../../../ARCHITECTURE.md", "../../../../ROADMAP.md", ] base = PurePosixPath(paths[0]) links = {p: str((base.parent / p).relative_to(base.anchor)) for p in paths[1:]} print("current relative links resolve to:") for p, resolved in zip(paths[1:], links.values()): print(f"{p} -> {resolved}") print("\nrepository files:") for p in paths[1:]: resolved = base.parent / p print(f"{p}: exists={resolved.exists()}, is_file={resolved.is_file()}") PYRepository: FBumann/linopy-yaml
Length of output: 686
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path base = Path("src/linopy_yaml/relational/sinks/README.md") links = [ "../../../ARCHITECTURE.md", "../../../ROADMAP.md", "../../../../ARCHITECTURE.md", "../../../../ROADMAP.md#track-4--sink-capabilities", ] for link in links: resolved = (base.parent / link).resolve(strict=False) print(f"{base}: {link} -> {resolved}") PY printf '\nRepository files:\n' for f in ARCHITECTURE.md ROADMAP.md src/linopy_yaml/relational/sinks/README.md; do if [ -f "$f" ]; then printf '%s exists\n' "$f" else printf '%s missing\n' "$f" fi doneRepository: FBumann/linopy-yaml
Length of output: 743
Correct the relative doc links in the sinks README.
From
src/linopy_yaml/relational/sinks/README.md,../../../ARCHITECTURE.mdand../../../ROADMAP.mdresolve tosrc/ARCHITECTURE.mdandsrc/ROADMAP.md, which do not exist. Use../../../../ARCHITECTURE.mdand../../../../ROADMAP.md#track-4--sink-capabilitiesinstead.🔗 Proposed fix
-executor in [ARCHITECTURE.md](../../../ARCHITECTURE.md)'s pipeline. +executor in [ARCHITECTURE.md](../../../../ARCHITECTURE.md)'s pipeline.-[Track 4](../../../ROADMAP.md#track-4--sink-capabilities) gives each sink a +[Track 4](../../../../ROADMAP.md#track-4--sink-capabilities) gives each sink a🤖 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/linopy_yaml/relational/sinks/README.md` around lines 3 - 4, Update the relative links in the sinks README to point four levels up: change the ARCHITECTURE.md reference to ../../../../ARCHITECTURE.md and the ROADMAP.md reference to ../../../../ROADMAP.md#track-4--sink-capabilities. Preserve the surrounding documentation text.
1 participant
Two commits: the layout move, and the license metadata it surfaced.
1. src layout
The
nativeCI job exists to prove a packaging claim: bare install, no linopy on disk, the streaming lane still works. Under flat layout it proved that about the source tree.tests/__init__.pymakes pytest's rootdir the repo root and prepend it tosys.path, solinopy_yaml/in the working directory shadowed site-packages — whether or not the install worked, or happened at all.Demonstrated before the change: built the wheel, installed it into a clean venv with no editable install, ran the suite from the repo root.
Green, and the wheel was never executed. A subpackage missing an
__init__.py, or a data file the wheel target fails to pick up, keeps CI green and reaches a user first.Same probe after the move:
With nothing importable at the root there is nothing to shadow, so
uv sync --no-editablein that job runs the suite against the built wheel. Full suite passes that way.The sdist is now the build input, and nothing more
include = ["/src"]— 996 files down to 33. Two bugs were hiding in the old list:tests/andREADME.mdmatched at any depth and swept in every nested copy under a build tree or local worktree.tests/shipped withoutexamples/, whichtests/conftest.py:15andtests/test_walkthrough.py:31both read. Running the shipped suite from an unpacked sdist:38 failed, 262 passed, 8 errors— allFileNotFoundErroronexamples/. It carried a test suite that could not run.Rather than add
examples/to make it runnable, the sdist drops the pretence — and--no-editableabove now covers from inside CI the case a downstream sdist test run would have caught.README.mdandLICENSEneed no entry: hatchling adds thereadmetarget and the declared license on top of the include list. Verified identical file lists with and without them.2. PEP 639 license metadata
license = { text = "MIT" }and theLicense ::classifier are both deprecated. The SPDX expression pluslicense-filessays the same thing in the fields that own it — which is also why LICENSE does not belong in the sdist include list.License-File: LICENSEanddist-info/licenses/LICENSEunchanged.hatchling>=1.27floor added — the first release with these fields.Verification
pytest308 passed, 1 xfailed ·ruff checkclean ·ruff format --checkclean ·pyrefly check0 errors ·twine checkPASSED on wheel and sdist · suite re-run against the installed wheel · wheel 32 files, sdist 33.No source file changed — the diff is a
git mvplus path references and metadata.🤖 Generated with Claude Code