C++ BMI Module Integration #343

mattw-nws · 2021-11-22T20:58:53Z

Support for C++ BMI modules loaded from shared libraries.

Additions

C++ module loading code (Adapter, Formulation, Formulation_Constructor...)
Test harness C++ BMI model (which mirrors C test model implementation)
Test code (largely duplicated from C BMI test code)
Documentation updates

Removals

Changes

Testing

Added test runner tasks to the workflows for C++ models on Ubuntu and macOS--these tests pass.

Screenshots

Notes

Todos

Checklist

Testing checklist (automated report can be put here)

Target Environment support

Linux

mattw-nws · 2021-11-29T15:21:57Z

doc/BMI_MODELS.md

 * `bmi_c`
+* `bmi_fortran`
+* `bmi_python`


Notably, the docs don't cover this yet... though it's supported... should it be removed?

Those (bmi_fortran, bmi_python, bmi_multi) should have been added previously, so adding them now is appropriate. A section on details of Python module usage is probably also still needed, but can be added separately.

…he link string causing conflicts!)

mattw-nws · 2021-11-29T15:42:52Z

include/realizations/catchment/Bmi_Cpp_Adapter.hpp

+                    bmi_model = std::shared_ptr<Cpp_Bmi>(
+                        dynamic_creator(),
+                        [](Cpp_Bmi* p) {
+                            // DO NOTHING.


Looking for feedback on the below. It may make the most sense to simply go back to classic pointers... this is what I evolved to. ???

This is confusing me a bit, just walking through the life cycle of the bmi_model in this adapter.

Adapter is created
a) BMI dll handle is created
b) BMI Model pointer created, associated with ONE adapter (and ONE dll handle)

Adapter proxy's calls to BMI pointer

Adapter is no longer needed, gets deleteed.
a) The way this code is right now, the only custom cleanup happens in AbstractCLibBmiAdapter::~AbstractCLibBmiAdapter(), since this class' destructor is empty (it gets executed, then the parent class destructor will get executed)
b) Next finalizeForLibAbstraction() is called. Since there is no override for this virutal function in this subclass, the parent implmentation is called.
c) FInally, this triggers the dlclose
c++ void finalizeForLibAbstraction() { // close the dynamically loaded library if (dyn_lib_handle != nullptr) { dlclose(dyn_lib_handle); } }
As I look through this implementation, I don't see how Finalize() is ever actually called. In the BMI C lib apdater, Finalize() on the BMI model is triggered by an external call to Finalize() (which we don't use) or implicitlyby the Adapter destructure (which overrides and points to the same function that the adapter Finalize calls...).

So alltogether, I don't see how any finalization and cleanup is going to happen with this implementation, and I'm concerned that if you were seeing issues with ref-counting this shared pointer and that causing the order of deletion to be affected (the Adapter and its base were destroyed before the shared pointer) then I also wonder if this pointer really needs to be shared (in the OWNERSHIP perspective) or if it should truely be a unique_ptr and we simply share REFERENCES to the pointer as needed.

This may require some additional disscussion.

Okay, I may be replying too quick here, but... Yes, that's correct... my assumption was that we treat every adapter as if it were a plain BMI C++ model... that means that there must be a step # 2.(n-1) where Finalize() is called on the adapter (which should be proxied to the underlying model object in the adapter's Finalize() method, but was missed above, per my comment there). Are we not calling Finalize() on the adapters before deleting them in step 3? If not... I might start to argue that we should be. Otherwise we're implementing the BMI interface according to the .hpp file but are not using the interface according to its semantics (I'm assuming those semantics require or imply that Finalize() should be called before delete).

A few things:

finalizeForLibAbstraction() is deliberately not virtual, so the logic needed for Finalize() could be called from the destructor more safely (and then reused more easily by Finalize() itself)

See earlier comment again about how this type's destructor probably needs to be calling Finalize() or something equivalent

I agree with the earlier comment, and the one above, that the module's own Finalize() doesn't seem to be getting called as part of this object's Finalize(), but should be

Yes, we are implementing the BMI interface, but we are not limiting ourselves to exclusively using the semantics of the interface

It makes sense to have an adapter implement the interface, as this aligns it intuitively for its role as a proxy

It should not be necessary to restrict a BMI implementation to only work (internally) via the BMI interface functions

It doesn't make sense to restrict the framework to only working with an adapter via BMI, because we have the code (e.g., why require explicitly calling Finalize() when it makes sense to have this happen during the destructor?)

However, adapters must be written to work in a consistent manner whether we follow the more strict BMI workflow (e.g., explicitly call Finalize() somewhere) or not (e.g., expect the destructor to also handling Finalize())

It doesn't make sense to restrict the framework to only working with an adapter via BMI, because we have the code (e.g., why require explicitly calling Finalize() when it makes sense to have this happen during the destructor?)

Well, I'd argue It does make sense to restrict ourselves, at least (or especially) if we ever expected to treat adapters and non-adapter C++ BMI models interchangeably...for which we might not have the code. It's not an explicit part of the interface, but is probably implicit in the name--that if you don't call it explicitly then memory may not be freed properly... at the least, we couldn't assume that the non-adapter model would call Finalize on itself in its destructor. It becomes a LSP thing at that point.

A related question...are we considering it safe to call Finalize() more than once? I would generally think we'd assume that was not safe, but I think what's being discussed makes that possible or even likely. Again, the BMI docs are not helpful here.

@hellkite500 On another part of your first comment above... The bmi_model property is defined in the parent class Bmi_Adapter<T> as shared_ptr<T>; I'm not sure if that was for a reason of needing to hand out the underlying model for some reason--to me it probably makes sense for it to be unique_ptr instead. But, that would require refactoring all the adapters to some degree.

@mattw-nws, FWIW, we are actually getting the underlying C struct and Python model in certain tests. It may still be more appropriate to change to unique_ptr and do something differently in the tests, but that'd be another aspect to any changes.

Refactoring the bmi_model property seems out of scope. Other topics surrounding Finalize() are resolved in d69094a. Resolve this conversation, @hellkite500 and @robertbartel ?

Did you mean this commit? d73eee3

@hellkite500 yes

extern/test_bmi_cpp/include/bmi.hpp

extern/test_bmi_cpp/src/test_bmi_cpp.cpp

test/realizations/catchments/Bmi_Cpp_Adapter_Test.cpp

hellkite500 · 2022-02-07T16:57:42Z

test/realizations/catchments/Bmi_Cpp_Adapter_Test.cpp

+    adapter->Finalize();
+}
+
+/** Test that the update function works for a single update. */


Does it really work if it doesn't get the right values back? Why have Update_0_a and Update_0_b tests?

I dunno! Ask him. (@robertbartel) Maybe now you can guess what //Everything below this line is?

Actually...I'm inferring that this is based on an approach to keeping unit tests to testing a single thing... the first test tries to catch exceptions or other unexpected failures...if that passes, then we can worry about whether it actually succeeded (assuming they run in order).

I'm more wondering why we are naming our human-readable test names with zero-based indexing...Stockholm Syndrome?

doc/BMI_MODELS.md

extern/test_bmi_cpp/include/test_bmi_cpp.hpp

extern/test_bmi_cpp/src/test_bmi_cpp.cpp

…ng create/destroy function names.

donaldwj

Looks to me like everything is in order. We may want to formally discuss the register/create issue and how what start point different languages should expect from a DLL.

donaldwj · 2022-03-08T18:53:21Z

src/realizations/catchment/Bmi_Cpp_Adapter.cpp

+                             utils::StreamHandler output, bool do_initialization)
+                             : AbstractCLibBmiAdapter<Cpp_Bmi>(type_name, library_file_path, std::move(bmi_init_config), std::move(forcing_file_path), allow_exceed_end,
+                             has_fixed_time_step, creator_func, output)
+                             //TODO: We are passing creator_func as registration_func because AbstractCLibBmiAdapter expects it to exist, but are not using it the same way...may be okay but we may want to generalize that assumption out!


In general Finalize() should only clean up memory(). Setup for a new run should probably be Finalize() followed by Init()

hellkite500

Would like to see the inline comments about the life cycle of a Cpp_BMI pointer/object updated to keep track of exactly what is going on with that complex component, but I won't hold this up if we want to address that in a follow up PR.

hellkite500 · 2022-03-08T21:02:48Z

include/realizations/catchment/Bmi_Cpp_Adapter.hpp

+                    dynamic_creator = (ModelCreator) symbol;
+                    bmi_model = std::shared_ptr<Cpp_Bmi>(
+                        dynamic_creator(),
+                        [](Cpp_Bmi* p) {


The class/destruction heiarchy looks like bmi::BMI -> BMI_Adapter -> AbstrarctCLibBmiAdapter -> Bmi_Cpp_adapter

Possibilities to consider:

shared_ptr ref count goes to 0

only happens if BMI_Adapter drops its reference??? But no guarantee of this...

BMI_adapter is deleted

Delete Bmi_cpp_adapter

delete AbstarctCLibBmiAdapter

calls dl_close

delete BMI_Adapter

delete shared_ptr, ref_count == 0, delete on pointer

error, dl_close happened already, cannot call destroy function...

We should protect the resource behind the shared pointer...I think it works now, but for the wrong reasons...

Scenario 1 won't likely happen right now....but the cas when that becomes problematic is in the future when something decides to use get_bmi_model() and gets a shared_ptr (ref count up...) and they HOLD that pointer until after the adapter is deleted (for whatever reason...) then you have a very problematic state...

A future PR to refactor these BMI pointer resources to unique_ptr should help with ensuring that ownership and cleanup of these abstract resources is the sole responsibility of the adapter. What we have here should work for the moment...

The comment in these delete should probably also better reflect this overral life cycle (will help explain why the seg fault occurs if destrory() is called there....)

mattw-nws added 5 commits November 22, 2021 20:54

Initial stab at C++ modules...working! Needs tests

848caab

glorius tedium

605fb78

A few improvements suggested by testing

851116c

Adds dlerror handling to dynamic_library_load

05c7a2b

TTTC?

1d5f076

mattw-nws force-pushed the integrate-freezethaw branch from e3e68cd to 1d5f076 Compare November 26, 2021 19:09

mattw-nws added 3 commits November 26, 2021 20:32

Apparently needed for macOS

05d532d

Removed half-baked handler code that only worked in gcc.

88359a6

Adding docs

ffc39ae

mattw-nws requested a review from robertbartel November 29, 2021 15:15

mattw-nws marked this pull request as ready for review November 29, 2021 15:17

mattw-nws commented Nov 29, 2021

View reviewed changes

Moved C++ docs down to avoid changing C links (++ is deleted from t…

d25d548

…he link string causing conflicts!)

mattw-nws commented Nov 29, 2021

View reviewed changes

mattw-nws mentioned this pull request Dec 13, 2021

Support look-back for nest BMI module input values and bootstrapping of defaults. #346

Merged

hellkite500 reviewed Feb 7, 2022

View reviewed changes

robertbartel reviewed Feb 8, 2022

View reviewed changes

doc/BMI_MODELS.md Show resolved Hide resolved

robertbartel reviewed Feb 8, 2022

View reviewed changes

extern/test_bmi_cpp/include/test_bmi_cpp.hpp Show resolved Hide resolved

robertbartel reviewed Feb 8, 2022

View reviewed changes

extern/test_bmi_cpp/src/test_bmi_cpp.cpp Outdated Show resolved Hide resolved

robertbartel reviewed Feb 8, 2022

View reviewed changes

extern/test_bmi_cpp/src/test_bmi_cpp.cpp Outdated Show resolved Hide resolved

Matt Williamson added 9 commits February 8, 2022 19:59

The "easy" changes for NOAA-OWP#343 review comments

d69094a

Fixed missing call to Finalize(), improved error handling for missi…

d73eee3

…ng create/destroy function names.

Added submodule for bmi-cxx

16da992

Use extern bmi-cxx's bmi.hxx

2f8cee7

bugfixes for getting function names from json

d1c6184

Bad include path...but worked on my system..?

ce24fd0

mystery revealed

981a075

Better documentation of intent

11eb6f0

C++ style casting

4913934

mattw-nws mentioned this pull request Mar 4, 2022

Add exception types for C++ BMI Modules #386

Open

Matt Williamson added 2 commits March 7, 2022 15:48

Exception improvements

58669c0

More appropriate parameter name

4ae1801

mattw-nws requested review from hellkite500 and robertbartel March 7, 2022 22:14

Somebody's got a case of the Mondays.

dcbc8ce

donaldwj approved these changes Mar 8, 2022

View reviewed changes

hellkite500 approved these changes Mar 8, 2022

View reviewed changes

mattw-nws merged commit 59d1f6d into NOAA-OWP:master Mar 9, 2022

mattw-nws pushed a commit that referenced this pull request Mar 9, 2022

The "easy" changes for #343 review comments

32e8c90

mattw-nws deleted the integrate-freezethaw branch March 9, 2022 13:57

This was referenced Mar 9, 2022

Refactor Bmi_Adapter and AbstractCLibBmiAdapter to make shared_ptr a unique_ptr #390

Closed

Setting up environment for Evapotranspiration submodule in ngen #383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++ BMI Module Integration #343

C++ BMI Module Integration #343

mattw-nws commented Nov 22, 2021 •

edited

Loading

mattw-nws Nov 29, 2021

robertbartel Feb 8, 2022

mattw-nws Nov 29, 2021

hellkite500 Feb 7, 2022

mattw-nws Feb 7, 2022 •

edited

Loading

robertbartel Feb 8, 2022 •

edited

Loading

mattw-nws Feb 8, 2022

mattw-nws Feb 18, 2022

robertbartel Feb 18, 2022

mattw-nws Mar 3, 2022

hellkite500 Mar 4, 2022

mattw-nws Mar 4, 2022

hellkite500 Feb 7, 2022

mattw-nws Feb 7, 2022

mattw-nws Feb 7, 2022

donaldwj left a comment

donaldwj Mar 8, 2022

hellkite500 left a comment

hellkite500 Mar 8, 2022

C++ BMI Module Integration #343

C++ BMI Module Integration #343

Conversation

mattw-nws commented Nov 22, 2021 • edited Loading

Additions

Removals

Changes

Testing

Screenshots

Notes

Todos

Checklist

Testing checklist (automated report can be put here)

Target Environment support

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattw-nws Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

robertbartel Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldwj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellkite500 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattw-nws commented Nov 22, 2021 •

edited

Loading

mattw-nws Feb 7, 2022 •

edited

Loading

robertbartel Feb 8, 2022 •

edited

Loading