Feat/deterministic network audit final - #79
qian-harvard merged 10 commits into
Conversation
qian-harvard
left a comment
There was a problem hiding this comment.
This addresses all three blockers from #68, and I checked each one against the network that disproved the original rather than taking the description's word for it. Thank you for the rework — the diff is noticeably tighter than the first attempt.
The three blockers, verified
DISCONNECTED_BUS now fires. unsupplied_buses(net) & in_service_buses is the right primitive. Re-run of the exact case that killed the first version, plus the ones around it:
two islands, slack at b0 status=error DISCONNECTED_BUS: [2, 3]
orphan bus, no lines at all status=error DISCONNECTED_BUS: [1]
no source anywhere status=error DISCONNECTED_BUS: [0, 1]
ext_grid out of service status=error DISCONNECTED_BUS: [0, 1]
The last one is a nice property you may not have aimed for: a source that exists but is out of service correctly supplies nothing. And no false positives where it matters — case9, case118 and mv_oberrhein all come back ok.
The module is in the right package. pandapower/audit.py, next to the server that uses it. I checked the import hazard that comes with that, since the repo directory shares a name with the installed library: with only the server directory on sys.path (how powermcp/runner.py launches it), import pandapower inside panda_mcp.py still resolves to site-packages, not the repo folder. No shadowing.
It is reachable. audit_network registers as a tool and works end to end through the server — case9 returns ok, the islanded network returns two DISCONNECTED_BUS findings, and the payload round-trips through json.dumps. The non-mutation claim holds at the tool boundary too: bus table, line table and converged all unchanged after a call.
Also good: except ImportError instead of the catch-all, and the arbitrary 0.0..1.1 / 0.9..2.0 voltage windows are gone in favour of finite-and-signed checks, which is a much more defensible thing to assert.
Merged onto current main it is clean — no conflicts, 455 passed, and the server entrypoint tests still pass.
One thing worth fixing: status is overloaded
status: "error" means two different things, and a caller cannot tell them apart:
A) {"status": "error", "message": "No pandapower network is currently loaded. ..."}
B) {"status": "error", "counts": {"errors": 2, "warnings": 0, "info": 0}, "findings": [...]}A is "the audit could not run". B is "the audit ran and the network has two unsupplied buses". A model branching on status will report a structural fault to the user when in fact nothing was audited — the opposite of what a pre-flight check is for, since the failure mode is silent and confident.
The AuditReport half of the contract is good precisely because it is machine-readable; the error half throws that away. Something like status: "failed" for the could-not-run case, or an explicit "ok" | "warning" | "error" | "failed" set, would keep the two separable. Either way it is worth stating the full set in the tool docstring, since that is what the model reads.
This is the same shape as the issue on #78, where a native failure dropped the structured fields and left only prose — worth a look at how that one landed.
Smaller notes, none blocking
counts["info"]is always 0. No check emits"info", so the key is decorative. Fine to keep for shape stability, just noting it carried over from the first version.- The test bypasses the server's import path.
tests/test_audit.pyloadsaudit.pythroughspec_from_file_location, which is a reasonable way to avoid thesys.pathquestion — but it means thefrom audit import audit_networkline inpanda_mcp.pyis not covered by any test. I verified it by hand and it works; a test that imports the server module would keep it that way. auditis a very generic top-level module name. Nothing in the current dependency tree provides one, so there is no collision today. The existingfrom core import ...in OpenDSS is the same pattern, so this is consistent with the repo rather than new risk.- Cost is fine.
iterrowshad me expecting trouble on a large case; 1354 buses audits in 54 ms, so it is not worth restructuring.
Happy to merge once status distinguishes "could not run" from "ran and found faults" — everything else here is in good shape.
Checked against pandapower's bundled networks, the audit graded eight solvable benchmark grids as structurally broken -- case145, case1888rte, case2848rte, case3120sp, case6470rte, case6495rte, case6515rte and case9241pegase all audited as "error" while run_power_flow converged on each. They carry negative line resistance or negative transformer vk_percent, as converted network equivalents often do. Audit changes: - Negative line r and negative trafo vk_percent are warnings. Non-finite r, and vk of zero or non-finite, stay errors: runpp raises FloatingPointError on those. - NaN voltage limits count as unset, which is how pandapower stores them, rather than as invalid. - Parameter faults on out-of-service elements are capped at warning, since they cannot break a solve. - An area fed only through a dcline is now flagged DISCONNECTED_BUS. A dcline is two PV generators, not a slack, so it supplies nothing; runpp leaves NaN voltages there. Every in-service bus is added to the graph so a dangling element reference cannot hide an isolated bus. Across all 62 zero-argument pandapower.networks constructors, exactly the eight grids above change, from error to warning. Tool contract: audit_network now reports like its sibling tools -- status "success" when the audit ran, "error" with a message when it could not -- and carries the verdict in audit_status (ok, warning or error). Across the pandapower server "error" means "could not run" nineteen times; a verdict in the top-level status gave it the opposite meaning in one tool. The server also puts its own directory on sys.path before importing audit, so a direct launch under PYTHONSAFEPATH no longer crashes; main starts fine there and this branch did not. Tests were written first and cover each change, the envelope, tool registration and the safe-path launch: 10 failed at the previous head, 19 pass now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I pushed a round of fixes as The status vocabulary. I suggested The audit against real networks. Across all 62 zero-argument Three smaller corrections:
Packaging. The server now puts its own directory on Tests were written first and cover every change, the envelope, tool registration and the safe-path launch: 10 failed at your previous head and 19 pass now. Merged with current Left as follow-ups, none blocking: Your #86 is next. It needs the same envelope and has one packaging blocker; I'll comment there. |
Summary
Adds a deterministic, solver-independent structural audit layer for the pandapower MCP integration.
Changes
pandapower/audit.pywith deterministic network pre-flight checks.audit_network.Scope
This PR intentionally contains only the audit implementation, MCP exposure, and tests.