capi: the ABI v5 design, and the header check that will police it - #323
Open
samtalki wants to merge 14 commits into
Open
capi: the ABI v5 design, and the header check that will police it#323samtalki wants to merge 14 commits into
samtalki wants to merge 14 commits into
Conversation
Every one of the 84 v4 symbols as kept, renamed, re-signatured, or removed, with the six rules that decide each one. Written before the code so the one break powerio cannot revisit is reviewed as a design rather than as a diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The v4 surface is 84 functions, 5 handles, and 24 macros, and the audit accounted for the functions alone. That left rule 2 unfinished in its own signatures: pio_dist_network_free became pio_multiconductor_free while still taking a PioDistNetwork, so the word the rule retires survived in every call it touched. Handles and macros are now audited on the same rules, and the counts cover all 113 names. Three entries change what a payload means while the call still works, which is the v4 pio_convert_file bug in another place. They get a mechanism each rather than a sentence: bus_ids now returns one id per row, so a length assert separates v4 from v5; the SCOPF document already carries index_base, so v5 makes reading it required and moves the shared writer, keeping C and Python on one index base instead of splitting them. Rule 7 collapses reading to one verb. The file/directory split does not survive the formats: an OpenDSS .dss redirects a tree and goes through parse_file, and parse_file already dispatches a PyPSA directory before it looks at an extension, so pio_read_dir was a second door to a job the first one did.
An audit of the v5 design found one deleted capability, several underspecified mechanisms, and section arithmetic that does not reconcile against the code. Rule 7 deleted `pio_read_dir`, which is the only read symbol carrying `scenario: i64`, on the grounds that `parse_file` already handles directories. It does, for PyPSA CSV folders, but a gridfm dataset directory is N networks over one shared topology rather than one network in an unusual storage shape, and `read_gridfm_scenarios` on the Julia side loops that symbol once per id. The rule is rewritten around the distinction that survives: storage shape is the format's business and the verb names cardinality. `pio_parse_file` becomes `pio_balanced_parse`, since one file is not one case for OpenDSS `Redirect` trees or PyPSA folders, and `pio_read_dir` becomes `pio_balanced_parse_scenario`, pairing by name with the `pio_balanced_scenario_ids` already in the table. The package and multiconductor verbs follow. Rule 5 said what an absent tail field means and nothing else, so a zeroed options struct — what a Julia or Python caller gets for free — would have silently discarded every field the caller set. It now specifies the read bound in both directions, a minimum below which `struct_size` is an error rather than a request for defaults, the caller's zero-initialization duty, and `NULL` for defaults. `PioNormalizeOptions` gains its field list, with the boolean crossing as `int32_t` like every other boolean in the ABI rather than as the Rust `bool` its source field uses. `pio_warnings` was absent from a document titled "every symbol, and why", while its multiconductor twin was migrated explicitly. Four section headers disagreed with their own tables, and the disposal path count was wrong in both directions: six in, six out, since `pio_string_free` retires and `pio_conversion_free` arrives. The rename count is replaced by a rule naming the twelve functions that keep their names, because a count drifts and a rule does not. `pio_scopf_to_json` flips to 0-based on the wire, and the design named `index_base` as the migration mechanism. A field is only a mechanism if something reads it, and nothing does: both bindings forward the document unchanged and both test suites assert the value is 1. Since a 0-based index is still a valid 1-based index, a missed conversion reads the wrong element rather than failing. PowerIO.jl is now required to normalize to 1-based at the boundary. Separately, `capi-header-parity.sh` compares symbol names only, so a reordered argument, a changed type, or a struct field all pass it — which is how `pio_convert_file` once shipped with two arguments reversed. `capi-header-regen.sh` regenerates with cbindgen and diffs. cbindgen gates optional features behind `#ifdef`, so one run covers every feature configuration and it belongs in one job. The committed header already matches, so the check lands green. The header preamble named one of the two entry points that take a handle non-const. It now names both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rule 3's two-call shape means a size query and the fill run the serialization twice unless something keeps the result, and the obvious fix — cache it on the handle, since handles are immutable after construction — is false for the one handle where it matters most. `PioPackage` has two mutating entry points, `pio_package_validate` and `pio_package_set_operating_points`, and they rewrite exactly the fields that `to_json`, `validation`, `diagnostics`, and `operating_points` read back out. A cached read taken before `validate` and served again after it returns pre-validation text from a const accessor, immediately after the call whose purpose was to produce the new text. The header already documents `PioPackage` as the exception to the immutability rule, in the same paragraph the justification quotes. Outside the five free functions, lines 1718 and 1815 are the only non-const handle parameters in the crate, so the safe set is exactly the other five handles. Those cache each payload in a `OnceLock<Result<String, String>>`; `OnceLock<T>` is `Sync` when `T` is `Send + Sync`, so the existing `assert_send_sync` pins hold and the header's concurrent-read promise survives unchanged. `get_or_try_init` is unstable on the pinned toolchain, so the stored value is the `Result` and a failed first attempt retries rather than freezing, which is what happens per call today. The three symbols that take no handle carry build-time facts and cache in a static. Rule 3 also gained the invariants it relied on without stating: the return excludes the NUL, the buffer is always terminated, and a short buffer truncates on a UTF-8 boundary. `copy_to_buf` already does all three and `pio_warnings` already documents them, but only five symbols inherited that wording and v5 moves twenty-seven more onto the same idiom. Fallible symbols now clear `errbuf` on entry, since several can legitimately return nothing and a bare `0` otherwise means both "empty" and "failed". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A fifteen-agent adversarial pass over the v5 design, the symbol table, and the eleven-PR stack: five design topics each paired with a critic that tried to break its recommendations, plus mechanical passes on Simplified Technical English, table verification, the stack as one change, and security. Forty-two recommendations, nine sound, twenty-seven weak, six overturned. This is a working document. Its grammar section and symbol table replace what abi-v5-audit.md carries today, once the open decisions at the end are settled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three Opus tracks with adversarial critics, answering the questions left open after the last pass: whether PioSource should be universal or gridfm-only, whether powerio's geo family can absorb tellegen's layer outright, and what the Arrow generator-cost tables must carry for ExaModelsPower. Twenty-two recommendations, one sound, nineteen weak, two overturned. The critic broke the container track's central claim, and the break matters: PowerModels JSON with multinetwork=true carries N networks and the reader takes the top snapshot and warns (powerio/src/format/powermodels.rs:468-473), so gridfm was never the only format with more than one case per path. A second one already ships, and it currently drops data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three documents described the C surface and they disagreed. `pio_balanced_parse` took a path in one and bytes in the other, which is the same symbol with two incompatible signatures sitting in one directory. That happened because each round of design landed as a new file instead of replacing the one before it. `abi-v5.md` is the settled result. The other three are deleted rather than kept as history: a superseded design that is still readable is a design someone implements. The read/parse split is stated once and it decides the ingest surface: `read` takes a path, `parse` takes bytes. That removes `_file`, `_str`, `_bytes`, `_dir`, `_dataset` and `_scenario` from the surface, because the argument type already says what the suffix was trying to say. `.pwb` gains an in-memory entry point it never had, since a binary case cannot be a C string. Every path now opens to a source, so the answer to "why only gridfm" is that gridfm is not the only container: a PowerModels document with `multinetwork=true` also carries N networks, and the reader takes the first and warns about the rest. Entries are named rather than numbered, because `int64_t` is gridfm's Parquet key type and not a property of containers. `read` survives as a documented composition of open plus entry zero, the way SQLite documents `sqlite3_exec`, and it fails on a multi-entry source rather than silently disagreeing with the container form. The rest of the design is unchanged from the reviews: the handshake collapses to four symbols, the acopf trio goes for want of a consumer, geo keeps `parse` and gains a report struct, caching is scoped to the handles with no mutator, and the options structs take the extensible-syscall rules including a hard failure on a nonzero unknown tail. Written to be read: shorter sentences, active voice, one idea each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…stem The read/parse split repeated the mistake rule 7 exists to stop. Rule 7 says a verb must not encode where the data lives; `read` for a path and `parse` for bytes encodes exactly that, on a different axis. And the words do not carry it: nobody reads `read` as a path or `parse` as memory, so the reader checks the header every time and the principle does no work. There is one ingest verb now. `parse` takes a path. `parse_bytes` takes memory, and the suffix appears only for that. The distinction the suffix names is not storage shape, which is the format's business. It is who touches the filesystem. A path means the library opens files and may follow an OpenDSS Redirect tree; a buffer means it opens nothing. That is a security property rather than a convenience, and it is the mechanism behind the 0.7.3 advisory fix. `_bytes` rather than `_str` because a PowerWorld .pwb is binary and a NUL truncates it, which is a format v4 named and could not accept. libxml2 is the precedent: one verb, and a suffix for where the bytes came from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rule 6 deletes pio_dist_abi_version. The migration note said to delete the constant from PowerIO.jl and claimed BMOPFTools sees nothing. Both understate it: _ensure_dist_compatible resolves the symbol and gates 13 distribution call sites, and BMOPFTools.from_dss reaches it through parse_file. schema_versions, dist_capabilities and matrix_available degrade silently the same way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The constructor verb list omitted the verb its own package section uses, so the grammar did not describe the symbol table. Extraction is as_, because to_ already means emitter (to_json returns bytes) and a verb cannot mean both 'returns a handle you free' and 'fills your buffer'. as_ is also what Rust and Python already ship. The _bytes rationale claimed a capability the Rust does not have: parse_str takes &str and no case-ingest parse_bytes exists, so .pwb still needs a path. Named as new Rust work rather than left inside a rename estimate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Conversion MatrixLegendCells show
Transmission7 cases.
Distribution7 cases.
Full warning details: download |
mdbook compiles an unannotated fence as a Rust doctest, so the naming pattern `pio_<subject>_<operation>[_<qualifier>]` was handed to rustc and failed the docs build. It is text. scripts/ci-mirror.sh never ran mdbook, so the one job that catches this class was invisible locally. It runs both build and test now, and skips with a message when mdbook is not installed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every ABI visible JSON document changed shape while the integer stayed 4: pio_schema_versions_json dropped four keys, pio_dist_capabilities_json renamed schema_version to powerio_version, the Arrow metadata key became powerio.version. A binding built against 4 passes the handshake and reads null for keys it mirrors, which is the failure the integer exists to prevent. pio_acopf_* goes. Three symbols, no callers in PowerIO.jl or BMOPFTools, and an AC OPF surface is better re-cut additively when a consumer asks for one than frozen now on a guess. pio_build_info is the curl_version_info shape: one call, one report, new keys without new symbols. It carries what schema_versions_json, dist_capabilities_json and matrix_available each answered one at a time, plus the error category tokens, so a consumer that wants to branch on the kind of failure has a closed set to match instead of parsing prose. PIO_DIST_ABI_VERSION is frozen at 1 and documented vestigial rather than removed. Rule 6 of the v5 design is right that one shared object should not carry two compatibility promises, and wrong that the fix is deleting the symbol: PowerIO.jl gates thirteen distribution call sites on resolving it, so removal breaks every distribution call, and BMOPFTools.from_dss with them. Revoking the promise costs nobody anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The seven convert and write entry points took a caller buffer and cut the joined warning text to fit it. Warnings are unbounded in a way an error message is not: one per lossy element, so a large case always beats whatever size the caller guessed. PowerIO.jl guessed 64 KiB and appended a "may be truncated" marker when the fill came near the cap, which is a way of saying the channel does not work rather than a way of making it work. They now publish through an out pointer as one owned C string, freed with pio_string_free, NULL when the conversion lost nothing. It is set before the body runs, so a stale value from an earlier call cannot be read as this one's. This also fixes the dropped OpenDSS sidecar report. warn_dropped_sidecars names each file the written .dss refers to and did not get, and those names were riding the same truncated channel. pio_warnings and pio_dist_warnings keep their caller buffer: they return the byte length needed, so a caller sizes and fills without guessing, and nothing truncates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A case with an in-service 3-winding transformer star-lowers before the dense extractors run, adding one bus per transformer. pio_bus_demand, pio_bus_shunt and pio_n_islands counted that bus; pio_n_buses and pio_bus_ids counted the unexpanded table. A caller sizing a per-bus buffer from pio_n_buses read short, and the trailing entries it did get had no id to key them by. The shipped v4 header documented this and told callers to size from each extractor's own count query, ending "Aligning the two is a v5 change". This is that change: both now report the star-lowered space, so one count sizes every per-bus column and every dense index has an id. The regression test builds the case rather than vendoring one, because none of the MATPOWER fixtures carries a 3-winding transformer. Reverting the fix makes it fail at 9 against 10, so it is testing the thing it names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Two separable things, neither of which renames a symbol.
The header check.
scripts/capi-header-parity.shcompares symbol names, so a reordered argument, a changed type, or a new struct field passes it.scripts/capi-header-regen.shregenerates with cbindgen and diffs, which catches those. Wired into thec-abijob and intoscripts/ci-mirror.sh. cbindgen gates optional features behind#ifdef, so one run covers every feature set and the step belongs in exactly one job.Regenerating corrected the header's own thread safety note, which named
pio_package_validateas the only entry point taking a handle non-const.pio_package_set_operating_pointsdoes too: it replaces the operating points and revalidates.The design.
docs/src/abi-v5.mdis the v5 proposal in full: the naming grammar as numbered rules, every one of the 84 symbols as kept / renamed / re-signatured / removed with its reason, and what each change costs PowerIO.jl. None of it is implemented.pio_balanced_*,pio_multiconductor_*,PioSourceandPioConversionappear zero times inpowerio-capi.The document is under review and has been corrected four times. Most consequentially: ingest is one verb rather than one verb per input type;
_ensure_dist_compatiblegates thirteen distribution call sites onpio_dist_abi_version, which v5 deletes, soBMOPFTools.from_dssbreaks unless that gate moves topio_has_featurein the same change as the artifact repin; the constructor verb list omittedas_, which its own package section uses; andpio_balanced_parse_bytesneeds a Rustparse_bytestaking&[u8]that does not exist today, so v5 is not a rename-only pass.Merging this commits no one to implementing v5 in 0.9.0.
Twelfth in the v0.9.0 stack, on top of #321.