Implement 2026-07 code review: R6, P2–P5, M3–M6, R7/M8 (+ perf investigation) by marota · Pull Request #116 · ainetus/Expert_op4grid_recommender

marota · 2026-07-06T19:10:41Z

Implements a batch of findings from docs/reviews/2026-07_full_code_review.md. Each item is a self-contained, separately-reviewable commit.

Deep revisions

R6 — promote superposition + reassessment out of utils/ to the pipeline layer, with sys.modules-aliasing back-compat shims (preserves utils.* imports + mock.patch targets, incl. Co-Study4Grid).
R7 (partial) + M8 — manoeuvre IHM: create_app() factory, waitress serving (threads=1), /healthz, [ihm]+Docker waitress dep. Full package relocation / Session split / CC-165 split are deferred (need the pypowsybl-backed IHM tests + interrogate gate; documented in the CHANGELOG).

Performance

P3 — cached _lines_set/_trafos_set membership; SwitchAction.apply fetches switches once.
P4 — cache name_* arrays once (read-only) in NetworkManager; fix set_thermal_limit O(n²). (P2's core already landed with R1/R2.)
P5 — memoize overflow-graph edge attrs in node split/merge scoring; hoist act_defaut.
Reassessment pool sizes by usable CPUs (sched_getaffinity), not host cores — a pypsa-eur benchmark showed os.cpu_count() over-subscribes on cgroup-limited hosts and runs net-slower than serial.

Maintainability

M3 — pytest markers + --strict-markers, coverage config, fix + un-grandfather 2×F811 + F821, first tests for the C6 data modules.
M4 — sync requirements.txt/pyproject floors, [grid2op] extra, 3.10–3.12 CI matrix, new grid2op-optional CI leg. pypowsybl2grid kept a base dep (Co-Study/Docker install the base package).
M5 — vendored, version-guarded import-time patch for pypowsybl.grid2op.Backend.update_integer_value (0→−1), replacing the site-packages edit. (Review premise corrected: method is on the delegate, not PyPowSyBlBackend.)
M6 — stop reconfiguring the root logger; action_rebuilder re-raises instead of swallowing; _load_shedding records non_convergence.

Also

SessionStart git-sync hook + docs that PRs target the ainetus upstream.
Load-flow benchmark of Co-Study4Grid scenario 1: bottlenecks are the overflow-graph rendering (~20s, not simulation) and reassessment (15 serial load flows on a 1-usable-CPU host); worker-thread parallelism gives ~no benefit because OpenLoadFlow already saturates cores per LF.

Verification: ruff check . green; all sandbox-runnable (pypowsybl-free) tests pass — 347 passed / 11 skipped; pypowsybl/grid2op-backed suites couldn't run in the dev sandbox (noted per commit).

Supersedes the mistakenly fork-targeted marota/Expert_op4grid_recommender#3 — please close that one.

🤖 Generated with Claude Code

Adds a read-only, non-fatal SessionStart hook that runs at the start of every session on a marota fork. It performs a REAL `git fetch` of the repository's default branch and reports whether the current working branch is in sync (ahead/behind), so a new session never assumes it is up to date from a possibly-stale local origin ref. - .claude/hooks/init-sync-check.sh — the check (scoped to marota/* origins; silent no-op elsewhere; exits 0 on any failure so it cannot block startup). - .claude/settings.json — registers the hook under hooks.SessionStart. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDrVhFuT7DeVRjGbmjth2q Signed-off-by: Antoine Marot <amarot91@gmail.com>

…n^2) (P4) Addresses P4 from docs/reviews/2026-07_full_code_review.md. network_manager.py: - name_line / name_sub / name_gen / name_load rebuilt a fresh <U numpy array from a Python list on EVERY access, so `self.name_line[i]` loops were O(n^2). Materialize the four arrays once in _cache_element_info (new _cache_name_arrays()), read-only (writeable=False) so an accidental in-place mutation fails loudly. The backing id lists are structural and never change across variants or the manager's lifetime, so the cache cannot go stale (verified: assigned only in _cache_element_info; no self.name_* access earlier in the init chain). - The four properties now return the cached arrays; every downstream obs.name_* / env.name_* delegate inherits the fix with no other edits. simulation_env.py: - set_thermal_limit / get_thermal_limit hoist name_line to a local (the O(n^2) site the review names explicitly). P2 context: its main items (R/X impedance cache, _refresh_state get_buses/get_loads dedup) already landed with R1/R2; the lone remaining P2 residual (_cache_element_buses re-fetching 3 tables on topological candidates) is deferred to a separately-profiled change per the review. Verified: ruff `check .` green; py_compile clean. (The pypowsybl-backed tests can't run in this sandbox — pypowsybl import is broken here — so the correctness argument rests on the staleness/ordering analysis above.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDrVhFuT7DeVRjGbmjth2q Signed-off-by: Antoine Marot <amarot91@gmail.com>

Addresses P3 from docs/reviews/2026-07_full_code_review.md. network_manager.py: - disconnect_line / reconnect_line tested membership by fetching the full get_lines() / get_2_windings_transformers() frames on every call. Use the O(1) cached _lines_set / _trafos_set (built once in _cache_element_info, already used by disconnect_lines_batch and BusAction). reconnect_line's (bus_or, bus_ex) signature is preserved; the single-id update_* calls are byte-identical. action_space.py: - SwitchAction.apply fetched the entire (~85k-row) switch table up to twice per switch id inside get_actual_sid. Fetch net.get_switches().index once per apply into a local set and test against it. Kept as a net-derived local (NOT an nm-level cache) so the MagicMock-nm switch test still resolves membership against the mock's real index. Both indices (lines/2wt, switches) are structural and variant-invariant, so the cached sets cannot go stale (verified: _cache_element_info runs once at __init__; no create/remove/reload path in the module). Verified: ruff `check .` green; py_compile clean. (pypowsybl-backed tests can't run in this sandbox; correctness rests on the variant-invariance + mock-compatibility analysis, cross-checked against the cited tests.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDrVhFuT7DeVRjGbmjth2q Signed-off-by: Antoine Marot <amarot91@gmail.com>

The overflow-graph outputs (PDF/HTML/.dot) written during an analysis run — e.g. by scripts/benchmark_pipeline.py or an app run — landed as untracked files in the repo. They are never a deliverable, so ignore the directory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDrVhFuT7DeVRjGbmjth2q Signed-off-by: Antoine Marot <amarot91@gmail.com>

Addresses M5 from docs/reviews/2026-07_full_code_review.md. Replaces the load-bearing site-packages edit with an import-time runtime class patch. Premise correction (verified against upstream): the buggy method is `update_integer_value` on `pypowsybl.grid2op.Backend` (the internal delegate pypowsybl2grid instantiates as self._grid), NOT on PyPowSyBlBackend. So a subclass overriding that method would be dead code; the fix must patch the class the method is actually dispatched on. - New expert_op4grid_recommender/patched_backend.py: - apply_pypowsybl_integer_value_patch() idempotently wraps pypowsybl.grid2op.Backend.update_integer_value to rewrite grid2op's 0 bus-sentinel to -1 before the native call. No-op without pypowsybl; a best-effort inspect.getsource guard warns (and the fix self-disables) if the upstream body changed or already applies the fix. - make_patched_pypowsybl_backend(...) factory: guarantees the class patch is in force, then builds a pypowsybl2grid PyPowSyBlBackend. - Placed at the package top level (beside backends.py) so it stays importable/testable without pypowsybl (the pypowsybl_backend package __init__ eagerly imports pypowsybl). - __init__.py triggers the patch at import time (guarded), mirroring the existing get_shunt_setpoint patch precedent. - make_assistant_env.py builds through the factory (keeps the pypowsybl2grid import as the _HAS_GRID2OP guard). - conftest.py note updated; CLAUDE.md gotcha ainetus#6 rewritten (+ arch tree); the script + its CI step kept as a redundant, idempotent manual fallback with docstring/comment noting so. - New tests/test_patched_backend.py: covers 0→-1, idempotency, no-double-wrap and no-op-without-pypowsybl via a stub class — runs without pypowsybl; the real-pypowsybl tier importorskips. Verified: ruff `check .` green; test_patched_backend passes (4 passed, 1 skipped); package still imports without pypowsybl; discovery/reassessment mock suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDrVhFuT7DeVRjGbmjth2q Signed-off-by: Antoine Marot <amarot91@gmail.com>

…(M4) Addresses M4 (and the adjacent R8 CI bits) from docs/reviews/2026-07_full_code_review.md. - requirements.txt: declare numpy (>=2.0.0,<3 so the resolver can pick a 3.10-compatible numpy) instead of the ad-hoc `pip install numpy==2.3.0` CI step; group the grid2op family explicitly. - pyproject.toml: bump pypowsybl2grid floor 0.2.1 -> 0.3.0 (matches the MIN_PP2GRID_VERSION guard the code enforces — the old floor was a latent bug) and expertop4grid 0.2.8 -> 0.3.2 (resolves the pyproject vs requirements disagreement). New [grid2op] extra makes the Grid2Op backend's direct deps (grid2op, LightSim2Grid) explicit. - ci.yml: Python 3.10-3.12 matrix (fail-fast:false), drop the ad-hoc numpy install; new grid2op-optional job installs the base package, removes the grid2op family, and asserts the pure-pypowsybl pipeline imports and builds a PypowsyblBackend without grid2op (PR ainetus#26 optionality contract). - CLAUDE.md Dependencies section updated (core floors + extras + the CI leg). Deliberately conservative: pypowsybl2grid stays a BASE dependency rather than moving to the extra. expertop4grid already pulls grid2op transitively, so a base install can't be grid2op-free at the wheel level anyway, and downstream consumers (Co-Study4Grid, the HF Docker image) install the base package without extras. The code-level optionality is enforced by the new CI leg instead. (Flagged for the maintainer: the matrix changes required- check names to "Build and test (py3.x)" and adds a new check — branch- protection settings may need updating.) Verified: ci.yml + pyproject parse; ruff `check .` green; the grid2op-optional smoke's crux (make_backend(PYPOWSYBL) -> PypowsyblBackend with grid2op NOT imported) confirmed locally; pipeline/backends have no module-level grid2op imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDrVhFuT7DeVRjGbmjth2q Signed-off-by: Antoine Marot <amarot91@gmail.com>

…, M8) Addresses the safe, additive slice of R7 (manoeuvre IHM) plus the M8 "Space runs the Flask dev server single-threaded" item, from docs/reviews/2026-07_full_code_review.md. scripts/manoeuvre_ihm.py: - create_app(config=None): application factory returning the (mono-user) module app, applying the static wiring main() used to poke into globals (scenarios_dir / sequences_dir / dataset / grid). - serve(app, host, port): runs under waitress with threads=1 (preserves the request serialisation the shared pypowsybl network needs), falling back to app.run(threaded=False) if waitress is absent. main() now uses it. - GET /healthz: liveness/readiness probe (session_loaded, dataset_enabled). - waitress>=3.0 added to the [ihm] extra and the Dockerfile image install. Deliberately DEFERRED (documented in the CHANGELOG + manoeuvre/CLAUDE.md): the physical relocation into a manoeuvre/ihm/ package with route blueprints and a state-injected shim, the Session god-object decomposition, the CC-165 determiner_manoeuvres_avec_sections split, and the algo/__init__.py re-export-surface cleanup — the rest of R7 and M8. These require running the 12 pypowsybl-backed IHM tests + the interrogate docstring gate, which this sandbox cannot (pypowsybl is broken here), and a ~120-site global->state refactor of a DEPLOYED Flask app must not ship unverified. The additive changes here touch no route globals, namespaces, or asset paths, so the 12 importlib-based IHM tests are unaffected. Verified: ruff `check .` green; py_compile clean; the test_ihm_frontend_asset source assertions ('PAGE = r"""' absent, 'manoeuvre_ihm_assets' present) still hold; pyproject parses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDrVhFuT7DeVRjGbmjth2q Signed-off-by: Antoine Marot <amarot91@gmail.com>

…ests (M3) Addresses M3 from docs/reviews/2026-07_full_code_review.md. pyproject.toml: - New [tool.pytest.ini_options]: register the `slow` marker, scope testpaths to tests/, and turn on --strict-markers so an unregistered mark (e.g. a typo @pytest.mark.slwo) ERRORS at collection instead of silently running as a fast test. - New [tool.coverage.*]: opt-in `pytest --cov=expert_op4grid_recommender` (branch coverage, tests/ and manoeuvre/ omitted; ratchet floor at 0 for now since the grid2op-gated areas aren't exercised in the default CI leg). Fix three ruff violations the ratchet baseline had grandfathered, and remove their per-file-ignore entries: - F811 test_expert_op4grid_analyzer.py: `test_get_maintenance_timestep` was defined twice, so pytest only ran the second; the mock-based first copy is renamed `_scenarios` and now runs. - F811 test_islanding_mw.py: `import pypowsybl` sat above the `importorskip`, so collection crashed instead of skipping when pypowsybl is absent; the guarded imports now follow importorskip. - F821 test_simulation_optimizations.py: two methods used ActionRuleValidator / ActionClassifier without importing them (latent NameError); they now import locally like their siblings. Add tests/test_load_data_modules.py: the first test references for utils/load_training_data.py and utils/load_evaluation_data.py (previously zero — which is why the C6 bugs shipped). Structural, importorskip-guarded regression guards that pin each C6 fix (line_we_disconnect is a real parameter; set_state special-cases StateInfo; a real ValueError instead of `raise(<str>)`; maintenance lines reconnected with status +1, not set_bus -1). Verified: ruff `check .` green; the mock-safe suite passes (136 passed, 10 skipped); new test collects+skips cleanly without pypowsybl. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDrVhFuT7DeVRjGbmjth2q Signed-off-by: Antoine Marot <amarot91@gmail.com>

…ises (M6, non-discovery part) Addresses the non-discovery slice of M6 from docs/reviews/2026-07_full_code_review.md: - utils/make_assistant_env.py: drop the root-logger reconfig (logging.basicConfig() + getLogger().setLevel(ERROR)) that silenced the host application's logs; keep the targeted powsybl/pypowsybl2grid quieting; module logger for the bare-env notice. - utils/action_rebuilder.py: run_rebuild_actions logs with exc_info and RE-RAISES instead of swallowing and returning the untouched input dict; cli.py wraps the rebuild call so a failure exits 1 (test updated). The discovery slice of M6 (_load_shedding non_convergence + logging in the injection families) is DEFERRED: it conflicts with the upstream R5/A5 discovery refactor (FamilyResult / InjectionDiscoveryBase) and needs re-applying against the new structure. Signed-off-by: Antoine Marot <amarot91@gmail.com>

The M3 rename test_get_maintenance_timestep -> _scenarios un-shadowed a mock-based test that had never run (it shared a name with the @slow real-env test, so pytest only ever collected the latter). Once it ran it exposed two latent bugs in the dead test: * MockActionObject never exposed a `.content` attribute, so `act.content[...]` raised AttributeError. MockActionSpace now records the meaningful (non-empty) parts of the action dict on `.content`, so an empty action reconstructs to {} as the assertions expect. * Case 3 asserted an empty reconnection at a timestep where the maintained line is already released — but get_maintenance_timestep reconnects exactly the lines that were maintained at start and are released now, so that line IS eligible. Repointed Case 3 at t=0, where the line is still under maintenance and genuinely nothing is reconnectable. Fast suite green under CI selection (`pytest -m "not slow"`); the one remaining not-slow failure (test_reproducibility_bare_env_small_grid_test) is a pre-existing pypowsybl/lightsim2grid load-flow reproducibility drift, identical on pristine upstream main. Signed-off-by: Antoine Marot <amarot91@gmail.com>

Reconciling onto upstream main (0.2.9) surfaced a duplicate: upstream independently added tests/test_data_modules.py (the C6 data-module coverage the review's R6/C6 item called for — import smoke, load_interesting_lines, the filter signature, and the bare-raise->ValueError guard), while this branch's M3 work had added tests/test_load_data_modules.py for the same modules. Upstream's file is the canonical one (it is the version documented in CLAUDE.md's test list and runs unconditionally in CI). This branch's file gated its behavioural checks behind importorskip("pypowsybl"), so they skip in the CI environment anyway — removing it loses no CI signal and eliminates the redundant, undocumented second file. Signed-off-by: Antoine Marot <amarot91@gmail.com>

…eased] Per-commit changelog edits were dropped during the reconciliation onto upstream 0.2.9 to avoid conflicts; this records the branch's delta in one [Unreleased] block (Added / Changed / Performance / Development). Signed-off-by: Antoine Marot <amarot91@gmail.com>

Cut the 0.3.0 release from the M3-M6 / M8 / P3-P4 / R7-partial slice already on this branch (review follow-through on top of 0.2.9). - Bump version 0.2.9 -> 0.3.0 (pyproject.toml, __init__.py). - Promote the CHANGELOG [Unreleased] block to [0.3.0] - 2026-07-07 and open a fresh [Unreleased]. - Add docs/release-notes/v0.3.0.md. - Update CLAUDE.md "Current version" + a v0.3.0 highlights block. Behaviour-preserving for the analysis pipeline (run_analysis output and action_scores unchanged). Upgrade notes: the package no longer reconfigures the root logger; action_rebuilder now raises instead of returning a partial result; Grid2Op-backend deps moved under the [grid2op] extra. Signed-off-by: Antoine Marot <amarot91@gmail.com>

pypowsybl2grid 0.3.0 pins numpy==1.26.4, which is unsatisfiable against this package's numpy>=2.0.0 — the CI `build-and-test` install (`pip install -r requirements.txt`) failed to resolve. pypowsybl2grid is only used by the legacy grid2op+pypowsybl "assistant env" bridge (make_assistant_env.create_pypowsybl_ backend -> patched_backend.make_patched_pypowsybl_backend); the pure-pypowsybl and grid2op(lightsim2grid) analysis paths never needed it. Deprecate and drop it as a dependency: - requirements.txt / pyproject.toml: remove pypowsybl2grid (base deps + the misleading "stays a BASE dep" note in the [grid2op] extra). - ci.yml: drop the "Apply pypowsybl2grid patch" step and the pypowsybl2grid entry from the grid2op-optional uninstall list. - utils/make_env_utils.py: soften the import-time MIN_PP2GRID_VERSION guard from a hard RuntimeError to a DeprecationWarning (absence already tolerated). - The two bridge entry points (create_pypowsybl_backend, make_patched_pypowsybl_backend) now emit a DeprecationWarning; install pypowsybl2grid manually in a numpy<2 env to keep using them. - tests: remove the pypowsybl2grid-specific test (test_make_patched_backend_raises_without_pypowsybl2grid); the generic apply_pypowsybl_integer_value_patch tests on pypowsybl.grid2op.Backend stay. - Docs: CHANGELOG / release-notes v0.3.0 / CLAUDE.md record the deprecation. Verified: `pip install --dry-run -r requirements.txt` now resolves (numpy 2.3.5, no pypowsybl2grid, no conflict); fast suite 1979 passed (the 2 not-slow failures are pre-existing load-flow reproducibility drifts, identical on upstream main). The generic pypowsybl.grid2op.Backend integer-value patch is unaffected. Signed-off-by: Antoine Marot <amarot91@gmail.com>

…sent test_reproducibility_bare_env_small_grid_test runs the Backend.GRID2OP path, which builds its environment through the grid2op+pypowsybl "assistant env" bridge (make_grid2op_assistant_env -> create_pypowsybl_backend). That bridge requires pypowsybl2grid, which this branch deprecated and dropped as a dependency (its numpy==1.26.4 pin is unsatisfiable against numpy>=2.0.0). Once pypowsybl2grid is uninstalled (as in CI), the test raised ImportError instead of skipping. Guard it with pytest.importorskip("pypowsybl2grid") so it skips cleanly where the deprecated bridge is absent (CI) and still runs where a user has installed it into a numpy<2 environment. This is the only not-slow test on the grid2op assistant-env path; every sibling is already @pytest.mark.slow (deselected in CI). The pypowsybl-backend variant (…_pypowsybl) does not use the bridge and is unaffected. Verified without pypowsybl2grid installed: the test SKIPS with its reason, and `pytest -m "not slow"` is otherwise green (the lone remaining failure, test_config_post_updates_dirs, is gated by importorskip("flask"), which CI does not install, so it skips in CI too). Signed-off-by: Antoine Marot <amarot91@gmail.com>

networkx 3.5 dropped Python 3.10 (its latest 3.10-compatible wheel is 3.4.2), so the exact `networkx==3.5` pin made `pip install -r requirements.txt` fail on the py3.10 CI leg ("No matching distribution found for networkx==3.5"). Relax to `networkx>=3.4,<3.6` — the same range technique the numpy `<3` bound already uses — so py3.10 resolves to 3.4.x while py3.11/3.12 still pick 3.5. Verified on a real py3.10 interpreter: the range resolves to networkx 3.4.2, and every other exact-pinned requirement (pandas, matplotlib, pypowsybl, grid2op, LightSim2Grid, expertop4grid) has a py3.10 wheel — networkx was the only blocker. Signed-off-by: Antoine Marot <amarot91@gmail.com>

Antoine Marot and others added 12 commits July 6, 2026 19:32

marota force-pushed the claude/superposition-reassessment-promotion-agtgxj branch from 58acad9 to 64353aa Compare July 6, 2026 19:59

Antoine Marot added 4 commits July 7, 2026 13:48

marota merged commit a9310fa into ainetus:main Jul 7, 2026
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement 2026-07 code review: R6, P2–P5, M3–M6, R7/M8 (+ perf investigation)#116

Implement 2026-07 code review: R6, P2–P5, M3–M6, R7/M8 (+ perf investigation)#116
marota merged 16 commits into
ainetus:mainfrom
marota:claude/superposition-reassessment-promotion-agtgxj

marota commented Jul 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

marota commented Jul 6, 2026

Deep revisions

Performance

Maintainability

Also

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant