PLUME-60: Writable model parameters negotiation - #30
Conversation
There was a problem hiding this comment.
Pull request overview
Implements “writable parameter” negotiation needed for two-way coupling, allowing models to declare which parameters may be written back by plugins, and enforcing cross-plugin write/read conflict rules via a configurable write-back policy—while keeping the core model/plugin interface centered on ModelData.
Changes:
- Add a
writableattribute to parameter definitions and surface it through Protocol offer/require paths (including new writable-offer C/Fortran API variants). - Introduce
WriteBackPolicyand extendNegotiatorto gate writable requests and track cross-plugin conflicts across negotiation order, with a post-negotiation summary. - Expand unit/API tests to cover writable offers/requests, policy gating, and multi-plugin conflict scenarios.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/core/test_protocol.cc | Adds Protocol-level tests for writable offer/require semantics |
| tests/core/test_plugin_params.cc | Adds Manager/Negotiator integration tests for write-back policy + conflicts |
| tests/core/test_negotiator.cc | Updates Negotiator tests for plugin-name-based negotiation + writable gating |
| tests/core/test_catalogue.cc | Adds tests for ParameterDefinition(writable) and derived+writable rejection |
| tests/api/test_params_api.F90 | Extends Fortran API test to include writable offers + writable plugin requests |
| src/plume/Protocol.h | Adds requireWritable() and extends offer() with writable flag |
| src/plume/Negotiator.h | Adds write-back policy + cross-plugin conflict state + log summary API |
| src/plume/Negotiator.cc | Implements writable gating, conflict detection, claim recording, and summary logging |
| src/plume/ManagerConfig.h | Adds write-back-policy key validation + accessors |
| src/plume/Manager.h | Documents new negotiation behavior and adds getActivePluginNames() |
| src/plume/Manager.cc | Wires Manager negotiation to WriteBackPolicy + Negotiator summary logging |
| src/plume/data/ParameterCatalogue.h | Adds writable field to ParameterDefinition API/schema |
| src/plume/data/ParameterCatalogue.cc | Parses writable, prevents derived+writable definitions, and stores writable in config |
| src/plume/coupling/WriteBackPolicy.h | New enum + helpers defining policy semantics |
| src/plume/coupling/WriteBackPolicy.cc | String parsing + stream output for policies |
| src/plume/coupling/README.md | New policy reference documentation for write-back behavior |
| src/plume/CMakeLists.txt | Adds new write-back policy sources to build |
| src/plume/api/plume.h | Adds writable-offer C API entrypoints |
| src/plume/api/plume.cc | Implements writable-offer C API wrappers |
| src/plume/api/plume_protocol.F90 | Exposes writable-offer APIs to Fortran |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/two-way-coupling #30 +/- ##
============================================================
+ Coverage 71.24% 72.99% +1.74%
============================================================
Files 92 97 +5
Lines 3912 4358 +446
Branches 348 386 +38
============================================================
+ Hits 2787 3181 +394
- Misses 1125 1177 +52 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
27e8e92 to
dbd791b
Compare
dbd791b to
7efa12e
Compare
7efa12e to
f7a58ee
Compare
antons-it
left a comment
antons-it
left a comment
There was a problem hiding this comment.
Nice work! only a minor suggestion
| * | ||
| * See README for the full policy reference. | ||
| */ | ||
| enum class WriteBackPolicy : uint8_t { |
There was a problem hiding this comment.
just a note: I think this list of policies should cover most (all) cases that we can think of right now. I would not worry about the flexibility of this.
There was a problem hiding this comment.
I am not sure I understand your note, do you mean that I currently have anticipated too many scenarios ? As you said, that's the configurations I could think of for now in our current context, and it will be straightforward in the future to add another policy via this enum if we identify a new case indeed.
| } | ||
|
|
||
|
|
||
| void Negotiator::logSummary() const { |
There was a problem hiding this comment.
I think it would be useful to also add the revers of this map. param -> plugins(s). From the "Claimants_" maps already recorded. If too verbose we can add it under a "verbose" configuration flag.
There was a problem hiding this comment.
Yes in fact I see you have a "verbose" flag in the configuration which is not really used anywhere, either plugins or Plume, but during this dev I was thinking it'd be nice to give power to this flag by implementing a verbose mode or even some levels. For now I can add the reverse map log here as well, and open a ticket specifically to leverage this verbose flag and filter through the current Plume log. What do you think?
There was a problem hiding this comment.
Created PLUME-88 to follow up on this, and will add extra logging here in the meantime.
Description
I decided to break down the PRs for the two-way coupling feature as to allow more efficient and targeted review.
This PR implements the negotiation changes required in Plume.
Other PRs will come: ifs-source plume module | Plume run phase (state pattern) | WF plugin
The following criteria were applied:
Contributor Declaration
By opening this pull request, I affirm the following: