add compat feature for unsafe get_value_ptr

[JoshCu]

another public and possibly wrong pr so others can learn from my mistakes

I updated the rust lstm to use the latest versions published in crates.io and was getting this error

Running Models
Updating layer: surface layer
Running timestep 0
terminate called after throwing an instance of 'std::runtime_error'
  what():  BMI C model failed to get pointer for BMI variable land_surface_water__runoff_depth.

land_surface_water__runoff_depth is one of the output variables set as the main_output_variable in ngen.
I got it working again by modifying get_value_ptr but I'm not entirely sure it was the correct thing to do?
should I be overwriting that function in my lstm lib? do I need to change ngen to use get_value()?
I'd prefer to confine changes to the rust lstm code but I wasn't sure if that was possible

[aaraney]

Thanks for opening a PR, @JoshCu! I this is actually a bug in ngen. If I'm understanding this correctly, ngen is requiring that bmi-c models expose output variables over get_var_ptr for performance reasons. IMO the model should be able to dictate if you are allowed to get the variable by value or by reference. So, ngen should try get_var_ptr then if that fails try via get_value.

This problem is something i've been thinking about and just haven't had time / haven't settled on a solution yet on the rust side of things. My main concern is rust implementations are explicit about opting into exposing some or all variables over get_value_ptr via ffi. Im thinking something like adding an unsafe get_value_ptr member function to the bmi_rs::Bmi trait that by default returns BMI failure. This would allow rust implementations to decide if a driver, like ngen, can interact with the model in a "safe" or "unsafe" way. There are lots of other reasons besides safety why you might prefer using get_value over get_value_ptr but i'll save that for #3.

How this all works efficiently through the bmi_rs::Wrapper module is kind of an open question and what I'm wrestling with.

Im open to ideas and semantics that you have in mind!

[hellkite500]

Perhaps a rust specific BMI adapter is warranted for ngen instead of trying to shoe horn it through the BMI C semantics.

[aaraney]

@hellkite500 I guess it boils down to requiring bmi-c modules to expose all output variables over get_value_ptr. I get the technical reasons for efficiency sake, but it's unclear to me from a bmi stance if that should be required. Thoughts?

[hellkite500]

BMI doesn't prescribe any requirement beyond returning/indicating success/failure.

ngen, on the other hand, requires C models to expose pointers for efficiency.

If you can't/shouldn't expose pointers, then using the C model semantics in ngen doesn't seem appropriate. Hence a specific adapter to map the pointer interface to the get value can, as is done with the fortran adapter, even though it uses the C compatibility layer.

[aaraney]

What i'm hearing is there is nothing stopping ngen from supporting both get_value and get_value_ptr for bmi-c models, but at this point in time ngen only supports going through get_value_ptr. I don't see value in adding a separate adapter for rust to ngen... and I don't think there is a real use case to add the code to ngen's bmi-c adapter to support get_value 😃.

Are there any cases where ngen mutates data behind pointers it gets back from get_value_ptr? Or is that guaranteed to be strictly read only?

[hellkite500]

I would have to double check to be sure, but I wouldn't depend on the answer either way, as it is subject to change. Values pointed to by BMI acquired pointers should generally be considered mutable. The pointer itself would be safe.

If all that is needed is an override of get_value_pointer for the adapter it would be pretty trivial to derive the adapter from the existing C adapter and add a constructor hook.

Providing an unsafe pointer from the ffi is reasonable too as long as the consequence is acceptable to general rust BMI implementations, which may be hard to speculate on right now.

[aaraney]

If all that is needed is an override of get_value_pointer for the adapter it would be pretty trivial to derive the adapter from the existing C adapter and add a constructor hook.

Here are the options on the rust side i've come up with so far. It's really just a matter of getting the return type right; either *mut T or *const T. I'm also open to returning two pointers, one to the start and the end+1 (see as_mut_ptr_range) so the rust api would be easier to bind to C++ in the future.

    unsafe fn get_value_ptr(&mut self, name: &str) -> BmiResult<*mut T>;

or

    unsafe fn get_value_ptr(&mut self, name: &str) -> BmiResult<*const T>;

I am more and more convinced that *mut T is the right choice especially given the C get_value_ptr signature. This is the most powerful option, but consequently puts restrictions on the model developer.

int get_value_ptr(void* self, const char* name, void** dest_ptr);

Aside, the unsafe qualifier is not technically needed, but given how this is used over an ffi boundary I am inclined to mark it as such.