Skip to content

fix(powermodels): validate bus identifiers before conversion - #767

Open
BurhanAbdullah wants to merge 33 commits into
eigenergy:mainfrom
BurhanAbdullah:fix/powermodels-bus-id-range-validation
Open

BurhanAbdullah wants to merge 33 commits into
eigenergy:mainfrom
BurhanAbdullah:fix/powermodels-bus-id-range-validation

Conversation

@BurhanAbdullah

@BurhanAbdullah BurhanAbdullah commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Validate PowerModels bus identifiers and equipment references before narrowing or constructing canonical bus IDs. Missing and out-of-range references now fail explicitly instead of silently aliasing bus 0.

Review fix

The overflow regression constructs the first out-of-range ID in u64 space. On 32-bit targets it is rejected by usize::try_from; on 64-bit targets it reaches the explicit PowerIO ID ceiling. The assertion accepts the platform-range or PowerIO-range diagnostic. The previous #744-based Rust log confirmed bus_ids_outside_supported_range_are_rejected passes, and its wasm32 check completed successfully.

Supporting stored-IR performance change

The stored-record provenance-index optimization is explicitly documented here. It replaces ordered lookup sets/maps with capacity-reserved, borrowed-key hash indexes for source IDs, diagnostic IDs and history IDs. It does not alter serialization, record ordering, validation rules, or duplicate/reference checks. The unchanged six_figure_record_counts_decode_within_the_ceiling regression passed under the original strict five-second assertion; no threshold was relaxed.

Previous #744-based validation 41413090108 / job 125170387382 fetched the pinned case13659pegase.m, found 74 shifted rows out of 20,467, and passed all 65 matrix cells.

Final current-head CI

Head: 4fc170b4a1c67b5caafa394aa5fa1cea77c4425d (22 commits ahead, 0 behind current main a6bc722a72e700768c6b9fb0a339964f7f27cf2e). The three-dot diff is limited to CHANGELOG.md, powerio-tx/src/format/powermodels.rs, and powerio/src/stored/dto.rs.

All 10/10 workflow runs completed successfully: Semver 41425899357, Docs 41425899514, Python 41425899493, Fuzz smoke 41425899385, PowSybl 41425899345, Conversion Matrix 41425899512, CIM schema 41425899581, Julia binding 41425899446, Rust 41425899562, Validation 41425899448. No failed, cancelled or pending jobs. The only job-level skips are the PR-event-gated PyPI publish, browser-matrix converter and deploy jobs.

  • Rust build/test job 125212309542: 813 passed, 0 failed, 1 ignored; bus_ids_outside_supported_range_are_rejected passed.
  • The first out-of-range identifier is constructed in u64 space with checked addition. The assertion accepts the shared bus ID range diagnostic from either 32-bit usize conversion or the PowerIO ceiling. wasm32 job 125212309815 passed.
  • The unchanged six_figure_record_counts_decode_within_the_ceiling regression passed under the original strict five-second assertion; no threshold or test was changed.
  • Miri executed the C ABI ownership regression: 1 passed, job 125212310159.
  • Python 3.9: 258 passed, 8 explicitly reported skips, job 125212309038; macOS 15, Windows native and GridFM-extra jobs passed. sdist, wheel, wheel smoke and distribution validation passed.
  • Validation job 125212308299 fetched the pinned MATPOWER fixture, executed both 20,467-row phase-shift-sign legs (74 shifted rows), passed all 65 matrix cells and dc_makebdc, and ended all checks passed.

The stored-record provenance-index optimization remains documented above; its decoder performance regression is unchanged. Frederik's 32-bit overflow review thread has been answered with current-head evidence and resolved. This PR is ready for maintainer review; no merge was performed.

Copy link
Copy Markdown
Collaborator Author

CI diagnosis (verified from the failing job logs): the Rust workflow is green on this head. The Python 3.12, GridFM-extra, Windows native, lint, quality, wheel and distribution checks are green. Two failures are shared prerequisites rather than failures in this PR's changed Rust code:

No checks are being skipped or weakened, and I am not marking this ready to merge while those prerequisites remain unresolved.

Copy link
Copy Markdown
Collaborator Author

Fresh CI diagnosis on this head (not a parser-code failure):

I am not weakening the Julia ABI gate or marking either PR ready while the shared prerequisites remain unresolved.

Copy link
Copy Markdown
Collaborator Author

Fresh CI diagnosis: Rust and the full 65-cell conversion matrix pass. Remaining failures match shared base/dependency issues: Python 3.9 cannot be installed on ubuntu-26.04 (runner fix #779); Julia ABI-7 companion check reports the five AC-OTS entry points are not bound in PowerIO.jl; validation fails only the two PMD BMOPF fixtures (case3_balanced.json and case3_balanced_multinetwork.json) handled by #744. Please merge #779 and #744 in the agreed order, then refresh this branch and rerun. No tests have been weakened.

Copy link
Copy Markdown
Collaborator Author

Pushed two CI-only fixes to this PR branch:

Fresh full CI has started on 0fc0195. The validation and Julia ABI dependencies are the shared #744 release-validation repair and companion-pin removal. Keeping this PR scoped; it remains draft until all required checks pass.

Copy link
Copy Markdown
Collaborator Author

Further CI root-cause fix pushed: 115afbd migrates Python workflow macOS jobs/wheel matrix to macos-15 and updates the workflow-matrix regression test. GitHub retires macos-14 on 2026-11-02. Fresh CI is queued; the inherited #744 validation/Julia companion fixes remain the shared dependency.

Copy link
Copy Markdown
Collaborator Author

The fresh run on the updated Python/macOS workflow still fails the Julia ABI job for the same shared companion-pin reason, not because of this PR's parser/model change. The current check reports ABI-7 exports missing from the pinned PowerIO.jl checkout, including AC-OTS instance/solution, island, and sensitivity entry points (pio_ac_ots_instance_*, pio_ac_ots_solution_*, pio_balanced_network_islands, pio_islands_*, pio_calc_sensitivities, pio_sensitivities_*). #744 removes the obsolete companion pin and its Julia binding check passes. I am not weakening the ABI check or duplicating #744's shared repair in this feature branch; please merge #779 then #744, refresh these branches, and rerun.

BurhanAbdullah added a commit to BurhanAbdullah/powerio that referenced this pull request Oct 31, 2026
BurhanAbdullah added a commit to BurhanAbdullah/powerio that referenced this pull request Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up correction from the latest full CI run: the earlier Python 3.9 runner change was incomplete because the test job still ran both interpreter versions on ubuntu-latest. The matrix now explicitly pairs Python 3.9 with ubuntu-24.04 and Python 3.12 with ubuntu-latest, and the workflow regression test asserts that pairing. No tests were removed or skipped. Fresh full CI is running on the current head.

The remaining known shared blockers are the Julia ABI companion pin and the two PMD BMOPF validation fixtures, both handled by the upstream release-validation work in #744. I am keeping this PR focused on its parser change and will refresh/retest after the maintainers resolve and merge the #744/#771 design conflict.

@BurhanAbdullah
BurhanAbdullah marked this pull request as ready for review November 1, 2026 17:13
@BurhanAbdullah
BurhanAbdullah force-pushed the fix/powermodels-bus-id-range-validation branch from 0605727 to 114d658 Compare November 1, 2026 17:19
@BurhanAbdullah
BurhanAbdullah force-pushed the fix/powermodels-bus-id-range-validation branch from 114d658 to 856f13b Compare November 1, 2026 17:26

Copy link
Copy Markdown
Collaborator Author

CI diagnosis from the latest current-head run: Python, Julia ABI, conversion matrix, interoperability, schema, docs, fuzz and semver checks are passing. Validation fails on the same two existing PMD fixtures: case3_balanced.json emits unsupported offset/replace/source_id properties; case3_balanced_multinetwork.json is a whole TimeSeries<MulticonductorNetwork> writer unsupported-value error. The established design choices are under review in #744 vs #771 (normalization and snapshot traversal versus retaining source-only profiles and unsupported whole-series emission). I am not duplicating either competing design inside this parser PR. #766 also has one independent Rust performance test failure: six_figure_record_counts_decode_within_the_ceiling measured 5.176 s against a strict 5 s ceiling on the hosted runner. This is a performance assertion failure, not a Rust compile or parser regression. I am investigating the decode hot path before adjusting anything; I will not simply relax the assertion. Please treat these PRs as not green until these outstanding checks pass.

@frederikgeth frederikgeth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed b5318d7. The parser change looks sound; please fix the new overflow test's 32-bit input and assertion before approval.

Validation in an isolated checkout: all 9 existing PowerModels unit tests and 14 PowerModels integration tests passed. Additional boundary checks passed for all ten equipment-reference fields, including missing references when bus 0 exists, null/negative/fractional/out-of-range references, explicit zero, valid maximum IDs, and the documented null bus_i/index fallback. All ten reported current-head workflows are green.

Integration remains #779 -> #744 -> refreshed #767. The copied runner and BMOPF prerequisite changes match those PRs. The unrelated provenance-index optimization should be separated or explicitly described with its validation.

Comment thread powerio-tx/src/format/powermodels.rs Resolved

Copy link
Copy Markdown
Collaborator Author

Current-head audit (2 Nov 2026), head 97fc3c6c1bb543ded3df39d6e8ad856bae4f1961:

  • All 41 published check-runs are completed: 38 success, 3 expected PR-event skips; no failed, pending or cancelled check-runs.
  • Rust, Miri (1 real ownership test), Julia ABI, Python 3.9/3.12, macOS 15/Windows, docs, semver, fuzz, CIM, PowSybl, conversion matrix and distribution validation passed.
  • Validation job 125023379343 passes the vendored PMD fixtures but skips the phase-shift-sign leg because tests/data/large/case13659pegase.m is absent.
  • The overflow regression now constructs BusId::MAX + 1 in u64 space and accepts the platform-range diagnostic, addressing Frederik's 32-bit review finding.
  • The provenance-index optimization remains in the diff and needs separation or explicit justification.

This branch is 2 commits behind main and still depends on #744 becoming the upstream base. Refresh after #744 integrates and obtain a fresh review; do not treat the skipped large-case leg as exercised.

@BurhanAbdullah
BurhanAbdullah marked this pull request as draft November 2, 2026 04:10
@BurhanAbdullah
BurhanAbdullah marked this pull request as ready for review November 2, 2026 10:47

@frederikgeth frederikgeth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed 4fc170b alongside #766. The previous overflow-test finding is resolved: the first unsupported ID is constructed in u64 space with checked addition, and either platform-range or PowerIO-range diagnostic is accepted. No actionable findings; approved.

All 26 focused PowerModels tests pass. Supplemental public-API checks reject missing, null, negative, fractional and out-of-range values across all ten equipment-reference fields (60 cases), while preserving bus zero and the largest supported bus ID. The missing-reference check fails on main by accepting a missing load_bus when bus zero exists, confirming the regression.

The shared provenance optimization was reviewed and tested once on #766; this PR's implementation differs only in a comment. Combined integration and formatting pass, and all ten current-head workflows pass.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants