BMI Formulation Serialization and Deserialization implemented via a bmi::protocol - #957
BMI Formulation Serialization and Deserialization implemented via a bmi::protocol#957hellkite500 wants to merge 24 commits into
bmi::protocol#957Conversation
| (restore) classes registered under `NgenBmiProtocols`. | ||
|
|
||
| Conformance is optional. Models that do not implement the protocol run | ||
| unchanged; the engine silently skips checkpointing for those modules. |
There was a problem hiding this comment.
Protocol-wise, sure, conformance is optional.
The engine will need to contemplate what it means to be asked to checkpoint/save-state when it's running with a model that can't. That's a modeling problem.
Maybe the latter phrase's sentiment just doesn't belong in this document at all.
There was a problem hiding this comment.
I think it is relevant to note the protocol and the engine are opt in. If the functionality is required by a particular application, e.g. operational forecasting, it is up to the deployment of that application to ensure conformance and test the modules. There are clear indicators in the runtime to point to modules which aren't capable of that use.
I don't think it is worth preventing the testing and development of modules via the model engine that aren't capable of this specific functionality. I would be open to a strict mode that could be set which complains much more loudly and prevents runs/configurations that aren't fully conformant
There was a problem hiding this comment.
I agree on configurability of the engine to require or ignore protocol conformance.
That's not the point, though. Documentation of the protocol shouldn't speak to the pragmatics of a particular implementation - viz., whether ngen will or won't require conformance, and under what circumstances
There was a problem hiding this comment.
The applicability and audience of this file are somewhat muddled. It initially feels like it's addressing itself to model developers, and how they can expect ngen to interact with them relative to this protocol.
But, then it includes details of how ngen is going to store the data a model provides, which is not salient to the protocol, and how ngen's configuration will control its use of the protocol.
There was a problem hiding this comment.
I conflated these two viewpoints into one document. It definitely makes sense to pull them apart and target documentation to two distinct audiences.
|
I'm only part way through looking over this PR. Broadly, I like the construction, and appreciate the thoroughness of presentation (Thanks, Claude, I guess?). That said, I see a need for a stronger separation of concerns among at least
|
| // Check mass balance if able | ||
| r_c->check_mass_balance(output_time_index, simulation_time.get_total_output_times(), current_timestamp); | ||
| // Checkpoint state via the serialization protocol (no-op when unconfigured) | ||
| r_c->checkpoint_state(output_time_index, simulation_time.get_total_output_times(), current_timestamp); |
There was a problem hiding this comment.
This insertion starts to illustrate a design idea I want to push for in ngen, that's applicable to a lot of simulation engines. Basically, rather than hard-coding these various periodic meta-operations 'about' the simulation, there should be a timer wheel that gets cranked each time step, and dispatches whatever abstract operations are scheduled to occur on that step. Right now, as seen here, we're going to get an explosion of code enumerating every possible thing we might be configured to do on any step.
| // Frequency gate — matches NgenMassBalance. | ||
| bool checkpoint_step = false; | ||
| if (frequency > 0) { | ||
| checkpoint_step = (ctx.current_time_step % frequency) == 0; | ||
| } | ||
| else if (ctx.current_time_step == ctx.total_steps) { | ||
| checkpoint_step = true; | ||
| } | ||
| if (!checkpoint_step) { | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
This is exactly the kind of thing I was talking about in terms of separation of concerns - the protocol should run when it's called to run(), not be responsible for deciding "nah, I'd rather not".
| | `{{rank}}` | MPI rank (0 for non-MPI builds or uninitialized MPI)| | ||
| | `{{pid}}` | POSIX process id | | ||
| | `{{host}}` | First label of the host name (max 63 chars) | | ||
| | `{{date}}` | Local date at startup, `YYYYMMDD` | |
There was a problem hiding this comment.
This feels like a really weird selection of variables to be able to template over. I can see PID, host, and date being useful for intra-unit-test purposes, but not much else.
MPI rank is more reasonable, if one is going to embrace a file-per-rank design. However, my experience says that saved states should ideally be independent of rank/count. Baking in that dependence would seem to be a mis-step.
There was a problem hiding this comment.
To be clear on "date", I think it would be quite sensible to have tokens representing dates significant to the substance of the simulation - e.g. the starting epoch
I can kinda see the execution date being workable in an operational forecasting environment, but it would require a bunch of scaffolding that a date descriptive of the simulation itself would not, in that the simulation epoch is already going to be available in whatever script set up and launched the job.
There was a problem hiding this comment.
This was an attempt to keep the protocol agonistic of the runtime but still allow for some level of flexibility. The other templates seemed like plausible differentiators that may be useful and are straightforward enough to implement/support. I'm definitely open to cutting this down and adding new ones as needed in the future, though.
f91fb6b to
0106cfb
Compare
…/restore Add support for the four reserved ngen::serialization_* BMI variables that allow the ngen engine to checkpoint and restore model state via an opaque byte buffer (see NOAA-OWP/ngen#957). Protocol implementation: - SetValue(create): snapshots state into a versioned byte buffer - GetValue(size): reports buffer size - GetValue(state): copies captured bytes to caller - SetValue(state): restores model state from bytes with validation - SetValue(free): releases internal buffer The buffer layout (version 1) packs soil_storage_m, discrete theta, gw_storage_m, nash_subsurface_storage, and giuh_queue as raw doubles for bit-exact round-trip fidelity. Deserialization validates layout version, DSBM flags, and GIUH ordinate count against the running model config. Shared protocol definitions and helpers added to ngen_utilities.h following the existing mass balance protocol pattern. Tests: - test_serialization_metadata: verifies check_support() probe - test_serialization_round_trip: full save/restore/compare cycle Assisted by Claude Opus 4.6 (1M context)
0106cfb to
de3a93d
Compare
fb5bf64 to
979d40b
Compare
88b813f to
1021dd8
Compare
| 3. Append-only multi-writer safety — because each record is a self- | ||
| contained boost archive, and the length prefix lets a reader locate | ||
| the next record without reconstructing archive state, multiple writers | ||
| can append independently without file-level coordination. Writers | ||
| must still serialize their writes relative to each other — see | ||
| NgenSerializationProtocol for the per-instance mutex policy. |
There was a problem hiding this comment.
This might end up being misleading to callers, if anyone actually reads these comments looking for this sort of insight. Appends work fine between processes on a single system, but I honestly have no idea whether they work at all correctly on Lustre or other distributed filesystems.
It might be best to just back off the claims.
| * since epoch) get a one-to-one mapping; callers with formatted strings | ||
| * that don't parse cleanly get 0, and the record is still written — | ||
| * restore-by-timestamp just won't work against those records unless | ||
| * the caller matches on the same "0" sentinel. This is a deliberate | ||
| * policy: a parse failure is not fatal to save. |
There was a problem hiding this comment.
My gut sense is that this is not the right policy to adopt. If we're trying to save data that won't be usefully readable once it's written, because we don't know its timestamp, we should throw, not return a junk value that's otherwise part of the output range.
1021dd8 to
0987d39
Compare
| strcmp(name, NGEN_SERIALIZATION_FREE) == 0) { | ||
| *size = 0; | ||
| return BMI_FAILURE; | ||
| } |
There was a problem hiding this comment.
This may actually be the wrong approach, vs reporting a size congruent with the item's type, since that's how some of the adapters match up concrete language-level types against the strings reported through BMI.
The actual implementation in the test model really only needs to support the way we're going to use it, but it may also be taken as an example for other BMI models, so I'd prefer to treat it more expansively.
There was a problem hiding this comment.
Same applies to nbytes, too
There was a problem hiding this comment.
The assumption and design here is to treat this as a trigger, which is by design typeless, and contains no semantic size or number of bytes, but instead communicates a semantic action the module should take. the variable NGEN_SERIALIZATION_FREE doesn't actually exist. When I was working through this, the question of "What does it mean to call get_value on this kind trigger made it pretty clear that exposing it as an actual variable was just overhead and noise.
The only real semantic value is "did serialization free succeed when passed to set value" which is communicated by the "success or failure" of the set value call.
|
I'm a bit late to this party, so if I missed a similar discussion on this somewhere, feel free to ignore me ...
I think I see the reasons for wanting to just treat the advertised protocol "variables" as triggers for This doesn't really comply with the BMI documentation. It's one thing to go beyond what BMI intends, but using It may feel slightly clunkier, but I think it would still be better to have these as true state variable that are set, just as with other BMI variables. Then have the logic of Though, to Phil's earlier point:
Perhaps all this only really matters with respect to ngen's documentation, and perhaps we should be cautious about saying too much there about how model developers have to develop their models. Still, if it's being assumed on this end, we should be careful about any unintended consequences of that assumption. |
| /** Save-side / restore-side shared sub-keys. */ | ||
| constexpr const char* const SERIALIZATION_CHECK_KEY = "check"; | ||
| constexpr const char* const SERIALIZATION_FATAL_KEY = "fatal"; | ||
| constexpr const char* const SERIALIZATION_FREQUENCY_KEY = "frequency"; |
There was a problem hiding this comment.
I don't really like that the protocol itself owns responsibility for frequency of its execution, but it needs to be configured somewhere. It should be fine for now.
There was a problem hiding this comment.
The NGWPC work takes a different path on configuration, though I don't know the details.
| * All protocol instances pointed at the same `path` write into the same | ||
| * file; one entity (id) may have at most one record per time step but | ||
| * may have records at multiple steps. | ||
| * | ||
| * Thread safety — three regions to reason about | ||
| * --------------------------------------------- | ||
| * Each region is documented at its point of use inside the .cpp; this | ||
| * header lists them as a map so readers can find the relevant comment. | ||
| * | ||
| * (1) Per-instance I/O state (the persistent ofstream and the | ||
| * reusable payload buffer): guarded by `io_mutex_` because a | ||
| * future threaded driver may invoke `run()` for different ids | ||
| * concurrently against the same protocol instance. Without the | ||
| * mutex, two threads could interleave archive bytes on the | ||
| * stream and corrupt the file. |
There was a problem hiding this comment.
These are concerns about the implementation using a particular file format and writer functionality. They really shouldn't be part of the protocol itself.
Maybe we can pull in the Saver abstraction from the NGWPC work and shift it over there.
| // that mutation safe to do across threads. | ||
| mutable std::mutex io_mutex_; | ||
| mutable std::unique_ptr<std::ofstream> out_; | ||
| mutable std::vector<char> payload_buffer_; |
There was a problem hiding this comment.
The payload_buffer_ member almost makes sense, but I think we can potentially do without it.
Instead of using GetValue("state", payload_buffer_), we could just call GetValuePtr("state"), and pass that along to whatever is doing the output, skipping the intermediate copy.
| // buffer only grows; it is never shrunk, which is fine because each | ||
| // protocol instance is bound to one model whose state size is | ||
| // stationary across timesteps. | ||
| constexpr size_t INITIAL_PAYLOAD_BUFFER_BYTES = 4096; |
There was a problem hiding this comment.
If this comment is an accurate description of the operation, then it's a huge problem. It's talking about maintaining an extra memory footprint equal to each model's state.
When I previously read other comments about the buffer, I understood it to be a single shared instance that would be the maximum of any of the individual models being saved. As long as we're only dealing in catchment column models, and not t-route, SCHISM, or some big gridded model, that's fine and desirable.
There was a problem hiding this comment.
This is a real concern that is better handled with the refactored abstraction and Writer mechanics later (the buffer could more easily be per writer and reused much more in alignment with the original intent.) I think its a pretty small change against this PR head to support that, I'll take a look.
Close out the BMI serialization protocol's cross-language support
story with a pickle-based reference implementation in the Python
test model and end-to-end framework tests. Three pieces:
1. Bmi_Py_Adapter byte-typed path. `get_analogous_cxx_type` now
recognizes numpy `uint8` / `int8` / `byte` (itemsize 1) and
routes them through a new `uint8_t` branch in GetValue /
SetValue. The existing `copy_to_array<T>` and `set_value<T>`
templates instantiate cleanly with `T = uint8_t`, so the adapter
gains opaque-byte support without new pybind11 machinery.
2. Python test model. `bmi_model.py` gains the four reserved
variables (uint8 STATE, int32 triggers + size) with matching unit
strings and a pair of pickle-based `_create_serialization` /
`_deserialize_state` helpers. Location is deliberately not
advertised for the reserved names, matching the C/C++/Fortran
reference models. `get_var_nbytes` gains a dynamic branch for
the STATE variable: when the adapter asks how many bytes STATE
currently occupies (including at restore time before any CREATE
has fired), the model answers by pickling a snapshot of the
current state and returning its length. Pickle output is
size-stable for the fixed schema used here — numpy arrays of
fixed dtype/shape + Python floats, which pickle to fixed-width
BINFLOAT opcodes — so the live-state probe matches the saved
record's length and the adapter wraps the incoming byte buffer
at the right size. Docs updated: the Model Developer Guide
explains this adapter-driven sizing expectation so authors of
future Python (or similarly adapter-wrapped) BMI models have a
template to follow.
3. Framework tests. A new `test/utils/bmi/py_serialization_Test.cpp`
(wired into `test_bmi_python`) exercises three cases against the
live adapter:
* `check_support_passes` — conforming units probe.
* `save_writes_record` — byte-exact comparison between the
record on disk and a second manual CREATE/SIZE/STATE/FREE
capture against the unchanged model, catching any
adapter-layer byte corruption.
* `save_restore_roundtrip` — explicit mutation of the
serialized inputs (sentinel -777/-888) with a pre-restore
divergence assertion, proving that the post-restore equality
checks measure real work done by the restore rather than
inputs that were never perturbed.
Assisted by Claude Opus 4.7 (1M context)
NgenMassBalance::run treated `ctx.current_time_step == ctx.total_steps` as the end-of-run sentinel when frequency == -1. The Context contract puts current_time_step in [0, total_steps - 1] (end-of-step counter), so the comparison gated against an out-of-range value and the negative-frequency branch was dead at every in-range counter value. The original comparison was authored with begin-of-step semantics in mind, where K == total_steps would name the boundary before the would-be Nth step (which is never computed). Mixing the two perspectives made the branch unreachable in practice. Existing frequency_end tests passed against the buggy comparison because they manually constructed contexts with current_time_step == total_steps, matching the bug rather than the contract. They're rewritten to drive in-range; frequency_end_single_step, frequency_end_large_n, and frequency_negative_other_than_minus_one cover the sentinel edges. Assisted by Claude Opus 4.7 (1M context)
BMI_SERIALIZATION_PROTOCOL.md — protocol spec for model developers. Reserved variables, unit conventions, and the model implementation guide. Intro frames conformance as use-case dependent, with the engine's behavior for non-conforming models documented in one place. NGEN_SERIALIZATION.md — ngen-internal documentation. Wire format (SerializationRecord, length-prefixed boost archives), save and restore protocol configuration, identity convention (compound_id()), engine integration points, the worked multi-formulation example, realization-level (global) configuration including path templating, and scaling considerations (CheckpointIndex, id_subset, thread and process safety, when the protocol is not the right tool).
src/utilities/bmi/wire_format.hpp — RecordPrefix struct +
read/write functions for the v0.2 on-disk record header.
Defines the format, its versioning, the timestamp encoding
convention, the two's-complement assumption with its
run-host canary tests, and the cross-host portability
claims.
src/utilities/bmi/byte_io.hpp — internal helper that fronts
`boost::endian`'s per-type `store_little_uXX` /
`load_little_uXX` primitives via using-declarations, adds
the single-byte trivials boost doesn't supply, and wraps
them in iostream-friendly stream adapters that
`write_record_prefix` / `read_record_prefix` call.
Both are internal-only: under src/ (not include/), PRIVATE include
path on ngen_bmi_protocols, opted in by test_bmi_protocols only.
Will replace the boost::serialization-backed record layout in a
follow-up commit; landing the new infrastructure as pure addition
first keeps the format-introduction step independently reviewable
from the format-switch step.
Purely additive: no existing code path uses these helpers yet,
existing protocol tests are unchanged.
Assisted by Claude Opus 4.7 (1M context)
Replaces the boost::serialization-backed record layout (length-prefixed archive body) with the fixed-prefix wire format from the prior commit. write_record / read_next_record / read_record_length / read_record_metadata now drive the byte_io + wire_format primitives directly; BOOST_CLASS_VERSION machinery and the boost archive includes are gone. SerializationRecord mirrors the wire fields directly: id, time_step, simulation_timestamp, checkpoint_epoch, payload. The constructor's 5th parameter defaults to 0 so existing 4-arg test fixtures keep working; the production write site stamps it with std::time(nullptr) (a future Coordinator will provide a single per-event value). CheckpointIndex uses the new read_record_metadata fast walker — prefix + id read, then seekg past the payload. Index-build cost becomes O(prefix + id_size) per record instead of v0.1's O(prefix + payload_size). Internal field names align with the wire format (simulation_timestamp, target_simulation_timestamp). MAX_RECORD_ARCHIVE_BYTES is renamed MAX_RECORD_PAYLOAD_BYTES (it caps payload_length now, not an archive body). The library no longer links Boost::serialization. Error handling: the wire-format read/write helpers return expected<wire_format::Status, std::string> (reads) and expected<void, std::string> (writes). The three-state Status distinguishes "record read" from "clean EOF / torn final record"; the error arm is reserved for malformed records (bad magic, unsupported wire_version, oversized payload). The throw-on-corruption pattern from earlier drafts is gone — serialization.cpp's write loop and deserialization.cpp's index walk both check the expected<> arm directly. The library no longer imposes exception semantics on consumers; the application layer decides whether to escalate. The wire-format-specific tests in SerializationRecordTest are rewritten for the v0.2 byte layout and the expected<>-arm contract. Assisted by Claude Opus 4.7 (1M context)
New engine-agnostic library at include/utilities/serialization/
carrying the abstractions for record-based state checkpointing:
- Record — value type producers and backends interchange
- RecordBackend — abstract storage interface with nested
Writer / Reader sub-handles, returned by
factories writer() / reader(). Scoped
with_writer() / with_reader() wrappers are
the recommended call pattern.
- BackendError — categorized error type (NotFound, Corrupted,
IOError) carried on every expected<> error arm.
- IdPredicate — application-supplied id-string predicate the
library invokes but never parses.
- Durability — caller-chosen contract for commit semantics.
Header-only INTERFACE target; ngen_bmi_protocols PUBLIC-links it
and re-exports Record as the legacy SerializationRecord typedef,
so existing BMI call sites are unchanged.
Initial release v0.1.0. The 0.X series carries no API-compatibility
guarantee — the abstract surface is in active design.
Design rationale and the rules every backend must satisfy are
documented in include/utilities/serialization/README.md.
Fallible methods on the abstract surface carry [[nodiscard]] so
silently dropping an error arm requires an explicit (void) cast.
The abstract surface is covered by tests in test_serialization
against five reusable mock backends (Noop, Tracking, Failing,
Snapshot, SharedState); existing BMI tests continue to pass
unchanged.
Assisted by Claude Opus 4.7 (1M context)
Route both serialization and deserialization through the ngen::serialization::RecordBackend abstraction via a new FileBackend implementation shared across protocol instances by path (process-static weak_ptr registry). The N protocols on a realization configured against the same checkpoint file share one in-memory index, one write fd, and one mutex-serialized write critical section — fixing the latent interleaving hazard from N independent fds and amortizing the index walk to one per path. Two-layer scope filtering: realization-level construction scope bounds the index; per-call exact_id(ctx.id) read scope narrows each Reader to the one feature the engine is asking about. Removes the v0.1 process-global CheckpointIndex cache and its clear_checkpoint_indexes() eviction hook; weak_ptr lifecycle in the registry handles eviction automatically. Assisted by Claude Opus 4.7 (1M context)
Splits the checkpoint-record value type via a `detail::RecordT<PayloadT>` template into: - `Record` — owning, payload = std::vector<char> - `RecordView` — non-owning, payload = boost::span<const char> Writer API: - `RecordBackend::Writer::write(const RecordView&)` replaces the prior owning `write(const Record&)` signature. The writer streams the caller-owned payload bytes to storage and does not retain the pointer after returning. - `bmi::write_record` gains an overload taking `RecordView` as the canonical entry point; the owning `Record` overload is retained alongside it. - `FileBackend::Writer` consumes the view directly on the POSIX path (zero copy); the non-POSIX fallback calls the new RecordView `write_record` overload. Producer side (NgenSerializationProtocol::run): - Drops the `mutable payload_buffer_` member and the move-in / move-out dance with `Record`. Each save allocates a function-scoped `vector<char>`, fills it via `GetValue`, and hands the writer a view over it. Tests and mocks updated for the new write signature. Single review-checkpoint commit; may be squashed back into the earlier serialization commits on this branch once reviewed.
…mers adopt it
The original `parse_timestamp` guarded only against "std::stoll
consumed zero characters" — any input with at least one leading
digit (including ngen's "2025-12-19 14:30:00" format from
`Simulation_Time::get_timestamp`) parsed as the leading integer
(2025) and silently landed in the record's `simulation_timestamp`
field. The doc claim "non-numeric strings map to 0" did not match
the implementation.
Parser (`include/utilities/serialization/record.hpp`)
-----------------------------------------------------
Rewrite into a two-helper split with a uniform contract — all
three return `int64_t`, never throw, and resolve failures to a
documented out-of-band sentinel:
* `parse_epoch_string` — whole-string signed integer interpreted
as Unix epoch seconds. Tolerates surrounding whitespace;
rejects any non-numeric trailing content (including the
partial-numeric case that was the bug). Catches std::stoll
exceptions internally.
* `parse_formatted_time` — whole-string strptime match against
`TIMESTAMP_STRPTIME_FORMAT` (`"%Y-%m-%d %T"`, matching ngen's
Simulation_Time format). Tolerates trailing whitespace.
* `parse_timestamp` — orchestrator. Tries the epoch parser
first; cascades to the formatted parser on sentinel.
`UNPARSEABLE_TIMESTAMP_SENTINEL = INT64_MIN` is documented at the
type and pinned by tests. INT64_MIN interpreted as Unix epoch is
~9.2e18 seconds before 1970 — guaranteed unreachable as a real
timestamp, including paleo-hydrology simulations.
Producer adoption (`src/utilities/bmi/serialization.cpp`)
---------------------------------------------------------
On an unparseable `ctx.timestamp`, stamp the sentinel on the
record and emit a `PROTOCOL_WARNING` naming the offending input
and pointing to `parse_timestamp` for accepted forms. The save
itself succeeds — callers that only restore-by-step are
unaffected; restore-by-timestamp simply cannot match the
sentinel.
Consumer adoption (`src/utilities/bmi/deserialization.cpp`)
-----------------------------------------------------------
On an unparseable `restore.timestamp` config value, refuse to
enable restore — returns `PROTOCOL_ERROR` (when `fatal: true`)
or `PROTOCOL_WARNING` + `check = false` (when `fatal: false`),
rather than silently looking up the leading integer.
Tests
-----
New `test/utils/serialization/parse_timestamp_Test.cpp` (20
tests, 4 suites) pins each helper's success and rejection
contract, the sentinel's value and non-collision with any
parseable input, the cascade behavior, and the
`TIMESTAMP_STRPTIME_FORMAT` convention against the format
produced by ngen's `Simulation_Time::get_timestamp`.
New protocol-level tests cover the warning + sentinel behavior
end-to-end: `unparseable_timestamp_warns_and_uses_sentinel` on
the save side; `unparseable_timestamp_warns_when_non_fatal` and
`unparseable_timestamp_throws_when_fatal` on the restore side.
Existing tests across both serialization and deserialization
suites used `"t0"` / `"t1"` / `"t2"` as placeholder
`ctx.timestamp` strings; under the new contract each of those
saves emits a `PROTOCOL_WARNING` to stderr. Replaced 14 call
sites in `serialization_Test.cpp` and 1 in
`deserialization_Test.cpp` with parseable 1-hour-cadence epoch
strings (`"0"`, `"3600"`, `"7200"`, …) — same test semantics,
no warning noise. The stale comment in `check_writes_record`
claiming non-numeric inputs "land as 0" is updated to point at
the new dedicated warning-path test.
Single review-checkpoint commit; intended to be folded back into
the introducing commits — parser body + sentinel + helpers + test
file into the original SerializationRecord-on-disk-format commit;
producer/consumer adoption and test-suite noise cleanup into the
respective NgenSerializationProtocol / NgenDeserializationProtocol
introduction commits — once reviewed.
34928ba to
dc2173e
Compare
|
The most recent force push layers on top of #1008 #1009 and #1011 |
…ization_size in models A 32 bit compatible path is proposed in this commit, allowing models to use a narrower value for the serialized byte size up to INT32_MAX. Overflow becomes a model concern in this case. The outlier here is the Fortran ABI and the iso C interface, which *can* support a 64 bit c int declaration on the model side, but requires marshalling the size across the set_value_int function as an array of 4 byte ints, and using `transfer` to correction assemble them. Test coverage for each model expands to round trip into both a 32 bit and 64 bit integer in the test models.
`run()` had accumulated three pieces of unreachable code that
predated the DESERIALIZATION case being added:
- `expected<void, ProtocolError> result_or_err;` — a local
that was never assigned or referenced;
- `break;` after each `return` inside the switch arms — dead
because the return exits the function before the break can
fire;
- `return {};` after the switch — dead because every arm
(including `default:`) returns.
The `default:` case itself stays as defensive coverage for
enum-abuse-via-cast, with a comment explaining what triggers it.
No behavior change; just clearer control flow.
`FileBackend::create(path, ...)` is a path-keyed cache — whichever protocol's `initialize()` fires first fixes the shared backend's construction-time scope for every subsequent caller on that path. Because save-side previously ran first with no scope, the shared in-memory index defaulted to "all records in the file" and the restore-side `id_scope` (from `serialization.restore.id_subset`) was silently ignored at cache-fill time. Reordering so deserialization initializes first lets its `id_scope` size the shared index. Save-side writes append independently of the index scope, so tightening it here does not affect save behavior. Correctness is unchanged either way — per-Reader `exact_id(ctx.id)` scoping still filters results — but memory footprint of the in-memory index now honors the restore-side scope as intended.
`save.path` and `restore.path` now override the top-level `serialization.path` for their respective directions. The top-level `path` remains a shared default when neither sub-block overrides it, so existing configs keep working unchanged. Because `FileBackend::create` is path-keyed, split paths resolve to two independent backend instances — restore reads the source file and save appends to the destination without either seeing the other. Enables the write-forward pattern (warm-start from an old checkpoint, land new records in a fresh file) that a single shared path can't express. Warn-and-disable behavior is unchanged: an enabled direction with no resolved path (neither top-level nor sub-block) still emits a PROTOCOL_WARNING and disables. The warning text now mentions both possible sources so the operator can pick where to add the key. Tests: - save.path overrides top-level path (writes land in sub, not top) - save.path only (no top-level) is sufficient to enable save - restore.path overrides top-level path (reads sub, not top) - restore.path only is sufficient to enable restore - save + restore round-trip with distinct paths, verifying the restore file is not appended to and the save file is created Docs: schema updated for both directions; new "Split save/restore paths" subsection documents the resolution rule and the FileBackend consequence. Assisted by Claude Opus 4.7 (1M context)
0966ff0 to
a4558ec
Compare
|
@hellkite500, something occurred to me as I was working on getting NOM ready for this. I don’t see a way via the protocol to distinguish between what I'll call a hotstart restore (i.e., bootstrap a new simulation) versus a resume restore (i.e., resume an interrupted simulation after it failed or was stopped). The only way ngen sends info to a model is via the serialization payload, which doesn’t carry anything about the type of restore. I can imagine scenarios (and I think NOM might be a tangible example with some time-related counter variables) when a model might want to behave differently depending on which of those restore scenarios was happening, and I'm pretty sure (at least eventually) ngen will want to support the distinction. |
|
@hellkite500, to add weight to my previous comment, I am pretty sure Noah-OWP-Modular - after updates to support the protocol - would have some significant issues, unless it is also updated/hacked to ignore or leave out certain parts of the state (or we do more changes on the ngen side). For one, it would cause ngen to throw an exception on the first time step when used for a hotstart, unless More seriously, the time step value ( |
|
Hi Guys, I just wanted to remind you that I implemented a model state serialization strategy a few years ago (for OWP via Lynker contract). I gave several presentations on it at conferences, and wrote a report or two, etc. It doesn't appear that you are making use of that prior work. I'm happy to send details and links, etc., if it is of interest. We serialized several models as test cases, including Topmodel, and models in Fortran and C. Several others worked on this with me. |
The fundamental difference in this work specifically is the ability of a calling engine ( e.g. ngen ) to collect and mange the serialized representation via BMI. |
|
In the work we did, BMI was extended with three new functions including "get_var_role()", and C code was written with the intent of it being called by the NextGen framework. Jessica and I modified topmodel and others with these new BMI functions in a branch, and then demonstrated the serialization by this BMI extension mechanism. In other work we tested 4 different approaches to do the same for Fortran. Just hoping this work was helpful to the current effort. |
|
My concern over:
Is the extension aspect. The official BMI is the surface that we have the ability to work with as a lowest common denominator. Have your extensions been evaluated, approved, and added to the official BMI spec? If so, you may have made my week. Unless we pivot as a product, if it's not in the official spec, it's essentially off limits. |
|
The extension we proposed is a pretty elegant approach to this and many related problems, like model calibration, but is not part of the core BMI spec. The BMI Council has been working on what it means to be a BMI extension, and how a framework can tell which extensions a model supports. The way model state variables (which go beyond input and output variables) and information about them are acquired for the approach you are using goes beyond what a BMI implementor is expected to provide. So someone (e.g. you guys) then needs to modify a model's BMI functions to make this work for variables other than input and output variables. So while technically conforming to the BMI version 2.0 specs, you cannot apply this to a user-contributed, BMI v2 compliant model without further modifying their BMI implementation to a significant degree. So this isn't an optimal approach either. With our extension approach, you wouldn't need to do this additional work. |
|
|
||
| /** Reserved BMI variable names driving the protocol's capture sequence. */ | ||
| constexpr const char* const SERIALIZATION_CREATE_NAME = "ngen::serialization_create"; | ||
| constexpr const char* const SERIALIZATION_FREE_NAME = "ngen::serialization_free"; |
There was a problem hiding this comment.
The more I get my hands dirty with serialization code, the more squeamish about serialization_create and serialization_free BMI variables that semi-directly call internal logic within BMI objects I get. If the point of BMI is to just act as a data exchange and progression api, sneaking in a required RPC interface runs counter to that.
It's needed for the RAII approach, which is a relatively safe approach, but it may be forcing NGen to be responsible for things it shouldn't be. If BMI is supposed to be a pretty firm barrier between logic and language, the BMI implementation should probably be the one responsible for getting the BMI implementation initialized and ready to go (and destructed).
There was a problem hiding this comment.
There's no way in the BMI 2.0 ABI to avoid this, which is why these are "triggers" by type in the protocol, a loose indication between caller and receiver that when get/set value see this variable, it has expected semantics (e.g. rpc like semantics vs typical read/write a state variable).
| // The reserved-variable table is defined in serialization.hpp and | ||
| // shared with the restore protocol so the two agree on the exact | ||
| // set of variables a conforming model must expose. | ||
| for (const auto& ev : RESERVED_VARS) { |
There was a problem hiding this comment.
This may be a bit too restrictive. Under this logic, if a BMI implementation doesn't expose a create and free function, they aren't valid for serialization, but create and free may not be required for a BMI implementation to be adequately serialized and deserialized. Size and state? Needed without a doubt. Free and create? Not so much, especially since the assumption they are available hints at abstraction leakage.
There was a problem hiding this comment.
These are required if the protocol expects to always call these in a specific order and expect them to result in particular semantics.
This is why protocol compliance/support is documented and expected in addition. Regardless of the technique applied, someone is going to have to modify, annotate, and manage the serialization and deserialization of the computational states the model requires. The ability to classify a named variable as a Also, one benefit of treating state as an opaque buffer though is that 4 BMI calls are needed (as this protocol exists today) to capture any number of state variables from the model, vs having to enumerate and iterate, one after the other, N variables and make 4 * N BMI function calls to get the same collection. |
7 participants
BMI State Serialization and Deserialization Protocol.
This is an opt-in convention that lets a BMI model participate in ngen's state checkpoint/restore workflow by exposing four reserved BMI variables (
ngen::serialization_create,_free,_size,_state). The engine side is a pair of sibling protocolsNgenSerializationProtocol(save)NgenDeserializationProtocol(restore)registered under
NgenBmiProtocolson every BMI formulation. Records share a single length-prefixed archive file keyed by a new engine-levelcompound_id()per formulation, so a multi-feature, multi-submodule simulation can checkpoint into one file and restore any entity by id. Scale work, MPI-agnostic path templating, and full cross-language test-model support (C / C++ / Fortran / Python) are included.See
doc/BMI_SERIALIZATION_PROTOCOL.mdfor the model-author-facing spec andtest/utils/bmi/for end-to-end coverage.Additions
SerializationRecordon-disk format (length-prefixed boost archives)with truncation tolerance and a
MAX_RECORD_ARCHIVE_BYTESsanity capNgenSerializationProtocol(save) andNgenDeserializationProtocol(restore), with metadata-only
check_support,is_fatal-drivenescalation at
initialize(), and the usual frequency gatingCheckpointIndex— process-global per-path lazy-built index turningrestore from O(features × records) into O(records + features); exposed
clear_checkpoint_indexes()for memory-bounded callerscompound_id()accessor onBmi_Formulationwith multi-submodule3-part key injection (
<feature>.<n>:<submodule-mtn>:<multi-mtn>)realization::config::GlobalConfigKey+apply_config—realization-level inheritable config blocks with per-formulation override
utilities::resolve_path_tokens+{{rank}}/{{pid}}/{{host}}/{{date}}template support so per-rank/host/date paths stay an engine concern
(the protocol itself is MPI-agnostic)
Bmi_Py_Adapteruint8/int8/bytedispatch branch for opaque-bytemarshaling across the Python language boundary
(test_bmi_c, test_bmi_cpp, test_bmi_fortran, test_bmi_py — pickle-based)
Formulation::checkpoint_state()(per-step, called fromLayer.hpp) and restore call at the end ofBmi_Module_Formulation::inner_create_formulation(one-shot)formulations have been constructed — restores are init-phase-only, so holding
the index longer is dead weight for memory-bounded runs
doc/BMI_SERIALIZATION_PROTOCOL.mdspec — reserved variables, unitconventions, wire format, model-developer guide, engine integration,
scaling considerations, MPI guidance
Removals
Changes
NgenBmiProtocolsgainsProtocol::SERIALIZATIONandProtocol::DESERIALIZATIONenum entries + dispatch wiringFormulation_Managerparses a realization-levelserializationblock,resolves path tokens, auto-derives
restore.id_subsetfrom thehydrofabric, and injects the block into both construction paths
Bmi_Multi_Formulation::init_nested_modulepeeks the submodule'smodel_type_namefrom its params and injects the 3-part compound idvia
set_compound_id()beforecreate_formulation()runsLayer::update_modelscallscheckpoint_state()alongside theexisting
check_mass_balance()Bmi_Py_Adapter::{GetValue,SetValue}dispatch gains auint8_tbranch; existing numeric paths unchanged
Testing
cd cmake_build && make test_bmi_protocols && ./test/test_bmi_protocols→ 53 tests (record format + both protocols)
make test_bmi_c && ./test/test_bmi_c→ 57 testsmake test_bmi_cpp && ./test/test_bmi_cpp→ 45 testsmake test_bmi_fortran && ./test/test_bmi_fortran→ 93 testsmake test_path_tokens && ./test/test_path_tokens→ 9 testsNGEN_WITH_PYTHON=ON:make test_bmi_python && PYTHONPATH=<site-packages> ./test/test_bmi_python→ 54 tests (includes Python byte-path + save/restore round-trip)
make ngento confirm downstream linker integrityAll green on macOS/arm64 with gcc-14 / clang.
Notes
Format version stamped as v0.1 in file headers. Two independent
version fields (
BOOST_CLASS_VERSIONfor boost wire format,SerializationRecord::CURRENT_VERSIONfor application schema) supportforward evolution without one forcing the other.
The Meyers-singleton-style
CheckpointIndexcache holds memory untilclear_checkpoint_indexes()is called or the process exits.Intentional: init-time restores should share the index across all
formulations. Memory-bounded callers should invoke the clear hook
once initialization-phase restores complete.
This PR ended up a little larger than initially anticipated. Each commit in the PR is
logically contained and ordered in a meaningful fashion. I recommend reviewing by commit
if the overall change set seems a bit daunting, it should help the review process a fair bit!
Todos
Checklist
Target Environment support