First implementation of ngen BMI serialization protocol - #63
Open
robertbartel wants to merge 17 commits into
Open
First implementation of ngen BMI serialization protocol#63robertbartel wants to merge 17 commits into
robertbartel wants to merge 17 commits into
Conversation
Cherry picking commit 92e10c3 from ngwpc/development, with modifications: - Dropped Makefile work, now that project uses CMake; added Boost and C++17 to CMakeLists.txt instead - Resolved conflicts in src/bmi_topmodel.c unrelated to serialization - Replaced Log(...) calls with fprintf(stderr, ...), since EWTS logging is not carried over
Cherry picking commit a064751 from ngwpc/development. Splits serialization_size out as its own variable and moves the capture action from Get_value_ptr to Set_value, matching protocol semantics.
Cherry picking the serialization parts of commit 6259302 from ngwpc/development. Only include/vecbuf.hpp, include/bmi_serialization.h and src/bmi_serialization.cpp are taken; the reset_time signal and the topmodel.c/h signature refactor in that commit are left behind. Despite its name, this commit carries most of the serialization correctness work: - Read the archive through membuf and an explicit length; the previous istringstream truncated binary payloads at the first NUL byte - Archive time_delay_histogram, which was omitted entirely - Size contrib_area by nstep and Q by num_delay plus the histogram ordinates; both counts were wrong - Archive the array counts and reallocate on load when they change - Refuse to serialize when running stand alone - Prefix the payload with its length Also drops includes left unused once istringstream went away.
Fixing conformance with ngen protocol: - add include/ngen_utilities.h holding the reserved name, type, unit and item count tables, indexed by a shared enum - rename involved variables to have `ngen::` prefix - resolve the variables in Get_var_units, which is the engine's entire support probe and previously could never find them - tweak types of serialization_size and serialization_create to int - keep the variables out of output_var_names and input_var_names, and leave Get_var_grid and Get_var_location failing for them - don't fail Get_var_nbytes or Get_value_ptr during restore, when they are needed but no buffer exists yet
Declare a layout version through BOOST_CLASS_VERSION and check it before archiving anything, so a payload written by an incompatible build is rejected rather than misread. Boost stores the version in the archive itself and rejects newer ones on its own; this catches older ones. Also construct the archives inside the try blocks. The binary_iarchive constructor validates the header, so a foreign or corrupt payload was escaping deserialize_topmodel as an exception and terminating the process instead of returning BMI_FAILURE.
New serialization_unit_tests target covering the engine's support probe (the exact reserved unit strings), the other introspection calls, the save/restore sequence, free being safe whenever issued, and rejection of corrupt or stale payloads. The expected names, types and units are spelled out in the test rather than read from ngen_utilities.h, so a wrong table is caught instead of agreeing with itself.
New serialization_integration_tests target, kept separate so it does not slow the other targets. Runs the full 950 step forcing record from inputs.dat three ways -- straight through, with one restart, and with three -- and requires every output variable to match exactly at every step. Each restart writes the state to a file, finalizes and frees the model, then builds and initializes a new one to restore into, so state left dangling in the previous instance is caught rather than read by luck. The test was checked against a deliberately broken serializer: omitting sbar is caught at the first step after each restart. The header notes what the current fixture cannot reach, since it has no channel delay.
Writing all model state to snapshots but restoring differently based on designated restore mode, corresponding to either a resume (full restore of all state including config and clock) or a hotstart (restore of state of model properties appropriate to seed a new, separate simulation)
Contributor
Author
|
I've been developing this against a stale copy of the ngen serialization protocol branch and will need to reassess. I doubt it'll need a complete re-do, but moving to Draft while I sort that out. |
Steps back to applying every archived value, as upstream did, keeping only the exclusion of current_time_step. May not be right, but figuring out will be done later; for now, follow NGWPC implementation.
Follow updated protocol which makes ngen::serialization_size 64 bit, so no longer carries a separate length value.
Make sure CI actually uses the new tests.
Block and fail if trying to set serialization size in the middle of a restore, when there exists a buffer and so, by definition, already must exist a size value that is more correct and should not be changes (for as long as said buffer exists).
Avoid issues with (g,s)et_value_at_indices and use of backing pointer with special serialization protocol variables by disallowing their use with the indices-specific functions.
robertbartel
force-pushed
the
serial_dev
branch
from
August 17, 2026 16:00
337eb08 to
51fc861
Compare
robertbartel
marked this pull request as ready for review
August 17, 2026 16:53
Contributor
Author
|
Ok, I've adjusted the implementation to be against the correct target ( 🤦 ). Worth noting: I left the commit history intact, rather than folding my corrections into prior commits (I tweaked the commit messages a bit though). |
PhilMiller
reviewed
Aug 17, 2026
PhilMiller
reviewed
Aug 17, 2026
PhilMiller
previously approved these changes
Aug 17, 2026
PhilMiller
left a comment
PhilMiller
left a comment
Contributor
There was a problem hiding this comment.
Modulo the concerns around the todos, the code looks good to me. I'll approve, given that this is not in Draft mode, but I'll let you decide whether to merge or not.
Create shareable action for building Boost serialization dependency and using it rather than explicit steps across workflows (though moving location in ngen_integration due to some workspace hacking).
Avoid side effects of workspace-replacement trickery, which was causing cache saves to fail, by manually splitting.
PhilMiller
approved these changes
Aug 18, 2026
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
3 participants
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.
Implements the (pending) ngen BMI serialization protocol. Initial implementation geared primarily to supporting hotstart scenarios that should not restore full state (i.e., resume restores of full checkpoints). Designed to prepare for future use of resume restores as well, once mechanism is in place to communicate to model which is in use.
Builds on the earlier NGWPC serialization work, present in the NGWPC/development branch, while adapting it for aforementiion ngen protocol. In particular, adheres to the same set of properties being serialized and deserialized as in the NGWPC version, excepted for deseiralization of
current_time_step(left out of restore because its wrong for hotstart).Note that it is likely other state properties should also be omitted from hotstart restore deserialization. However, that will be scoped to a different issue/PR; this is just for the initial functional port of serialization functionality compatible with the ngen protocol.
Additions
src/bmi_serialization.cpp,include/bmi_serialization.h, with Boost-based binary serialize/deserialize of model state, with support for stale payloads checks.include/ngen_utilities.h, with details on the protocol's four reserved variable names and their required units, types and item sizes.include/vecbuf.hpp, with vendored streambuf adapters to optimize archive reads/writes.test/main_unit_test_serialization.c, with protocol conformance and round trip teststest/main_integration_test_serialization.c, with 950 time step restart equivalence integration test.Removals
Changes
Get_value/Set_valueand other appropriate BMI functions to support variables for protocol and through those trigger serialization and deserializationTesting
cmake -B build && cmake --build build.runconfig names its inputs relative to cwd:cd build./serialization_unit_tests: 10 checks, all passing./serialization_integration_tests: 3 scenarios over 950 steps and 14 variables, all passing./simple_unit_tests: existing suite, unaffectedScreenshots
Notes
Todos
current_time_stepis read by the physics (channel routing), not only used as an index; it thus both clearly meets the criteria to be excluded AND clearly meets the criteria to be included in the state transferred in a hotstart restore (probably this means state needs to be extended, as this property is overloaded)Checklist
Testing checklist
Target Environment support