Repository navigation
Conversation
… up CI
Verified against the pre-fix code: all three tests fail with the original
ModuleNotFoundError('pandas') when model_runner is imported eagerly, and
pass once it's deferred.
Two CI jobs: 'test' runs pytest with the full [run] extras installed;
'test-minimal-install' installs the bare package (no pandas, no pywr) and
runs create-synthetic-model as a real subprocess smoke test — the closest
possible reproduction of the original bug, independent of the mocked
unit tests.
…onfig Makes the test tooling a real reusable foundation rather than a one-off: pytest was only ever installed ad hoc in CI, with no declaration in the package manifest for local development. Added a 'dev' extra alongside the existing 'run' extra (both pip-install-able the same, standard way), plus explicit testpaths/coverage config in pyproject.toml so 'pytest' on its own does the right thing without CLI flags. Also installs libglpk-dev and sets PYWR_BUILD_LPSOLVE=false in the 'test' CI job, needed for the 'run' extra's pywr build to succeed on a bare Ubuntu runner. Both jobs verified end-to-end against a real containerised GitHub Actions environment via 'act', not just reasoned about from the YAML.
Generates a Cobertura XML report (pytest-cov's --cov-report=xml, already Cobertura-compatible with no extra conversion step) and uploads it with actions/upload-code-coverage@v1, per docs.github.com/en/code-security/ how-tos/maintain-quality-code/set-up-code-coverage. Requires Code Quality to be enabled for this repo first (Settings > Security > Code quality) — that's an org/repo admin action neither CI nor a push-only collaborator can do; this wiring is inert until someone with admin rights on Nexsys-Analytics flips it on.
… enabled actions/upload-code-coverage@v1 403s outright while the repo hasn't had Code Quality turned on yet (Settings > Security > Code quality) — an admin-only action, confirmed by a real CI run. fail-on-error: false is the action's own documented escape hatch for this; temporary until an admin enables it, at which point this step starts succeeding for real and the flag can come out.
…n gate coverage.py's own --format=markdown plus GitHub Actions' own $GITHUB_STEP_SUMMARY needs no repo settings, no admin action, and no third-party service — it just works, on every run, visible on the run's summary page. Complements (doesn't replace) the Code-Quality upload above, which will start actually working once an admin enables it.
The empty dict literal had no annotation, and pywr itself ships no type stubs or py.typed marker, so mypy would otherwise skip-warn on every model_runner.py import of it. Scoped ignore_missing_imports to pywr.* specifically rather than disabling import checking broadly. Also declares mypy and pandas-stubs as dev dependencies.
Runs against the full pywr_utils package (only needs the dev extras, no glpk build) rather than as a step in the heavier 'test' job.
pull_request: branches: [main] restricts the workflow to PRs whose base is main - confirmed elsewhere (clusterapi-slurm-contract) that a PR based on any other branch produces zero check suites under this config, no error, just silent non-triggering. This repo's own fix PR is stacked on the test/lazy-import-regression-coverage base, not main, so it's relying on undocumented/inconsistent behaviour rather than a guaranteed trigger. Dropping the branches filter under pull_request runs the workflow for a PR against any base; push stays scoped to main since that only matters for direct pushes/merges.
create-synthetic-model needs nothing beyond stdlib json/typing, but cli.py imported run_file/run_incremental_sizes from model_runner at module load time, and neither pandas nor pywr was declared as a dependency anywhere. Every CLI invocation, including model creation, failed with ModuleNotFoundError: No module named 'pandas' on a fresh install before even reaching the intended pywr build step. Defer the model_runner import to inside the two commands that actually need it, and declare pandas/pywr as an optional 'run' extra so a plain 'pip install pywr-utils' installs cleanly and create-synthetic-model works standalone.
The base branch's CI only installed [dev] since [run] didn't exist yet. Now that this commit declares it, install both extras and the glpk headers pywr's build needs, so CI exercises the actual fixed behaviour end to end rather than just the mocked unit tests.
…ed PRs Configures pip and github-actions version updates on a weekly schedule. Both use a 7-day default cooldown (a newly published version isn't eligible for an update PR until it's been out for 7 days), matching the project's existing min-release-age policy for npm/pnpm. Each ecosystem's updates are grouped into a single PR per run instead of one PR per dependency.
Adds [tool.coverage.report] fail_under=27, just below today's measured 29.05%, so pytest-cov fails the build if coverage regresses further without pretending an aspirational target is already met.
Scans only new commits per push/PR via the gitleaks binary directly (same approach as cluster-api-v2, no third-party Action). Uses plain default rules, no repo-specific patterns needed since there's no known historical-secret backlog here. Verified locally: config parses, scans clean.
…effect The method accepted a solver kwarg but ignored it, hardcoding Model.load(pywr_data, solver='glpk') regardless of what was passed in. Found by vulture (unused variable 'solver') while adding it to CI. No current caller passes a non-default solver, so this changes no existing behaviour, it just makes the already-public parameter do what its signature promises.
Adds it as a dev dependency and a new step in the typecheck job, after mypy, at --min-confidence 80. Gated against a committed whitelist covering the one remaining finding (an unused tempfile import), now that the solver-parameter finding is fixed rather than whitelisted.
Local safety net ahead of the gitleaks CI check just added: scans the commit range about to be pushed (upstream..HEAD, or just the tip commit if no upstream is tracked yet) before it leaves the machine. One-time setup per clone: pip install pre-commit && pre-commit install (default_install_hook_types installs it as a pre-push hook without needing --hook-type). Requires the gitleaks binary on PATH locally.
Both clean today (after upgrading the local venv's own stale pip, which had 5 known CVEs itself but isn't a declared project dependency - the CI step upgrades it explicitly so a stale bundled pip doesn't produce a false finding there).
Unit layer. Exercises SyntheticModelCreator's node/edge topology (including the modulo wrap-around when transfers outnumber inputs), get_model_summary, save_model round-tripping, and the create_synthetic_model helper's optional write. Node and edge counts are derived from the input/transfer counts rather than hardcoded, so the assertions stay meaningful if the generated layout changes.
Unit layer. _display_terminal_graph and _draw_line are pure functions over a DataFrame and a character grid, so they are tested without a solver or a model. Covers the correlation classification bands, suppression of the undefined correlation when setup times have zero variance, per-zone legend markers, the degenerate single-point case where there is no range to scale against, and Bresenham line drawing including grid clipping and not overwriting plotted data points.
Integration layer. Drives PywrFileRunner, run_file and run_incremental_sizes as Python APIs against genuine PyWr models solved by the real GLPK solver, with no mocking of the solve path. Confirms an explicit solver argument reaches PyWr rather than being discarded in favour of a hardcoded glpk: loading with glpk-edge must leave that solver on the model, and an unknown solver name must be rejected instead of silently falling back. Also covers recorder harvesting, the recorder whose values() is unimplemented, create_csv=False, the error dictionary returned when no model is loaded, output filename derivation, and the sweep's transfer capping, cleanup, empty-range and per-model error-row behaviour.
E2E layer. Invokes create-synthetic-model and run-incremental-sizes as a user would, via CliRunner over argv-style arguments rather than by calling the underlying functions. Covers the default models/ output path, creation of a missing output directory, --show-summary, --run solving the model it just wrote, --show-graph, and the abort paths for an unwritable output and a failing sweep. A final test shells out to the installed pywr-utils console script in a subprocess, which is the only check that the [project.scripts] entry actually resolves to a working command.
Integration layer. Injects failures at the filesystem boundary of run_incremental_sizes rather than stubbing the sweep, covering an undeletable model file, a model that fails after its file has been written, charting of an error row, and the reporting around a summary that fails to write or silently fails to appear. Includes a strict xfail for the output path bug: because the output directory is joined onto a path that already contains it, an output_csv with any directory component is written to <dir>/<dir>/<name>, so the sweep raises and loses every result after all the models have already been run. The test asserts the intended behaviour and will fail as an unexpected pass once the join is corrected.
Adds an e2e check that `python -m pywr_utils.cli` dispatches to the Click group, and an integration check that a model which fails to load and whose leftover file also refuses to delete still yields its error row rather than letting the cleanup's own exception discard every earlier result.
fail_under 27 -> 97, following the unit/integration/e2e tests just merged in (99.23% measured, model_runner.py 1% -> 99%).
Runs ruff's linter with autofix against staged files on every commit, alongside the existing gitleaks pre-push scan. default_install_hook_types now covers both pre-commit and pre-push so a fresh pre-commit installed at /Users/joe/Developer/ExaDev/Clients/Nexsys/repositories/misc/pywr-engine/pywr-utils/.git/hooks/pre-commit pre-commit installed at /Users/joe/Developer/ExaDev/Clients/Nexsys/repositories/misc/pywr-engine/pywr-utils/.git/hooks/pre-push activates both.
Adds ruff as a dev dependency and a lint (ruff) CI step running ruff check . with no fixes and no rule ignores applied -- this deliberately surfaces the codebase's current lint state as-is so it can be triaged as its own piece of work, rather than an agent unilaterally deciding what to autofix or suppress across the existing code.
A plain Requirement already satisfied: ruff in /Users/joe/.local/share/mise/installs/python/3.14.2/lib/python3.14/site-packages (0.15.6) (what CI and the pre-commit hook's isolated tool-install both use) resolves a much broader default rule set than the documented quickstart default (E4/E7/E9/F) that a locally-installed Homebrew ruff binary resolves to, despite both self-reporting the same version -- confirmed by hashing the two binaries, they are genuinely different builds. Pinning select explicitly makes the enforced ruleset deterministic regardless of how ruff was installed, rather than depending on which build happens to be on someone's PATH.
--exit-zero means the pre-commit hook auto-fixes what it can on staged files but never blocks a commit over findings it can't fix -- that's what CI's ruff check . (no --fix, no --exit-zero) is for. Without this, the hook was blocking any commit that touched a file with a pre-existing, not-yet-triaged finding, which made incrementally landing the safe-fixes follow-up work impractical.
Removes unused imports and empty f-string-missing-placeholder cleanups in model_creation.py and model_runner.py -- no behaviour change, verified by the full suite (72 passed, 1 xfailed) and mypy (clean) both before and after. cli.py's fix is deferred to the follow-up PR alongside its remaining F841 finding, so this PR only touches files that end up fully ruff-clean (the local pre-commit hook blocks any commit that leaves a touched file with unresolved findings).
Removes 5 unused imports/f-string issues -- no behaviour change, verified by the full suite (72 passed, 1 xfailed) and mypy (clean). Leaves 1 finding in place that --fix can't auto-resolve (an unused variable assignment) for separate, deliberate follow-up.
…rsion Drop the explicit select = ["E4", "E7", "E9", "F"] override in [tool.ruff.lint], letting ruff fall back to its own real default rule set (bugbear, pylint, security/bandit, pyupgrade, simplify, isort, and more) instead of the narrow quickstart subset. Determinism no longer comes from pinning the rule list by hand; it comes from pinning the ruff version itself exactly everywhere it runs. Pin the dev extra's ruff dependency to ==0.16.2, matching .pre-commit-config.yaml's ruff-check hook rev exactly. Previously the dev extra allowed ruff>=0.16, so a plain pip install could silently resolve a newer ruff than the one pre-commit and CI enforce, reintroducing the same class of nondeterminism the original E4/E7/E9/F pin was meant to close, just at the CI-vs-pre-commit layer instead of the Homebrew-vs-PyPI layer. Apply ruff's safe autofixes across the widened ruleset: import sorting, str(e) to e!s conversion, dropping a redundant pass after a docstring-only function body, and removing a redundant empty-parens class base. Findings ruff cannot fix safely (blind except, undefined name in the vulture whitelist stub, unused variable, and others) are left untouched for a later, separately reviewed pass.
`vulture_whitelist.py` is parsed as plain Python by both vulture (an AST walk that registers names as "used") and ruff (full name resolution), so every name it references has to be genuinely defined there. The bare `tempfile` entry whitelisted an `import tempfile` inside run_incremental_sizes() in model_runner.py, but that import was itself dead code and got removed by ruff's own safe autofix -- the whitelist entry was never cleaned up alongside it, leaving a name with no import of its own. Executing the module now raises NameError, which is exactly what ruff's F821 rule caught. Confirmed vulture's dead-code scan stays clean with the entry removed: nothing in the codebase still needs it whitelisted, since the only thing it ever covered for is already gone. Adds tests/test_vulture_whitelist.py: one test executes the whitelist file directly and asserts it doesn't raise NameError, the other runs `ruff check --select F821` across the repo and asserts a clean exit, so future undefined-name regressions fail the suite rather than only showing up in a separate lint step.
Both create-synthetic-model and run-incremental-sizes already report the caught exception to the user via click.echo before aborting, but the raise click.Abort() that follows discarded the original exception and its traceback rather than chaining it. Any caller or debugger inspecting the resulting Abort saw only a generic abort with no link back to what actually failed.
…f print run_incremental_sizes silently discarded the traceback of any model failure during the sweep, printing only the exception's string form before continuing to the next size combination. That mirrors the existing log-then-continue pattern already used in run_pywr_model (logging.exception, no re-raise), so this brings the sweep's own per-model catch into line with it, using a module-level logger rather than the root logger the sibling call reaches for.
…mmand run_incremental_sizes() already writes the output CSV, prints the summary, and renders the terminal graph internally before returning, so the CLI command never needed to read its return value. Keep the call for its side effects and drop the unused local.
Add from __future__ import annotations and replace typing.List, typing.Dict and typing.Optional with the builtin list, dict and X | None forms. With the future import, annotations are never evaluated at runtime, so this is a pure typing simplification with no behavioural change (the repo's minimum supported Python is 3.7, which does not support subscripting list/dict directly without the future import). Mechanical cleanup, not a bug fix.
…l_runner Replace list(x.values())[0] with next(iter(x.values())) in the recorder 'total' print branch and in the matching integration test assertion, and collapse the nested bounds/overwrite check in _draw_line into a single if with a combined condition. Both changes preserve the existing behaviour exactly. Mechanical cleanup, not a bug fix.
run_pywr_model's exception handler called logging.exception(e) directly on the logging module, which logs through the root logger rather than PywrFileRunner's own self.log logger for pywr_utils.model_runner. Any caller filtering, routing or silencing logs by module name (as the test suite already does elsewhere with caplog.at_level(..., logger=...)) would never see these failures. Passing the exception object itself as the log message was also redundant: exception() already attaches the traceback via exc_info, so the caught exception's repr was being logged twice. Use self.log.exception with a plain description instead, so the failure is attributed to the correct logger and the traceback is only recorded once. Extends the existing 'no loaded model' regression test to assert the failure is captured by the pywr_utils.model_runner logger with exc_info attached; confirmed the extended assertions fail against the old logging.exception(e) call and pass with this fix.
The codebase reached zero ruff findings under the real default ruleset across the fix/remaining-ruff-findings PR chain. The pre-commit hook's --exit-zero flag was a deliberate interim measure so incremental fix commits weren't blocked by pre-existing findings elsewhere in the repo -- with none left, the hook can enforce the same ruleset locally that CI already enforces, catching a new finding at commit time instead of only in CI.
…r compatibility Split the CI workflow into reusable composite actions (typecheck, test, test-minimal-install, gitleaks) so mono's synced copy of this repo can call the identical steps locally, and fix the workflow to run correctly against the self-hosted runner fleet.
Remove the committed .pyc files, which are build output regenerated on every interpreter run and shouldn't have been tracked.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This was referenced Sep 17, 2026
Mearman
enabled auto-merge (rebase)
September 17, 2026 07:24
knoxsp
previously approved these changes
Sep 17, 2026
knoxsp
left a comment
knoxsp
left a comment
Contributor
There was a problem hiding this comment.
Why are pycache files ending up in a commit? Pycache shouldn't be in git.
Mearman
dismissed
knoxsp’s stale review
September 17, 2026 08:56
The merge-base changed after approval.
Mearman
force-pushed
the
derived/ci-self-hosted-runners-v2
branch
from
September 17, 2026 08:56
0906b6a to
dedaa77
Compare
Collaborator
Author
auto-merge was automatically disabled
September 17, 2026 08:56
Pull request was closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #21. Second link in the stacked chain; based on #29.