Rust bindings (mir-sys crate)

[Choochmeque]

Description

Full implementation of the -sys crate for mir

Contributor Declaration

By opening this pull request, I affirm the following:

• All authors agree to the Contributor License Agreement.
• The code follows the project's coding standards.
• I have performed self-review and added comments where needed.
• I have added or updated tests to verify that my changes are effective and functional.
• I have run all existing tests and confirmed they pass.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.50%. Comparing base (27daabd) to head (9f952f9).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop      #49   +/-   ##
========================================
  Coverage    57.50%   57.50%           
========================================
  Files          620      620           
  Lines        26548    26548           
  Branches      2326     2326           
========================================
  Hits         15267    15267           
  Misses       11281    11281           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pmaciel]

Hi @Choochmeque, can you book a meeting to discuss this? Thanks!

[pmaciel]

Remarks:

• can you add headers to every added source file (.h, .cc, .rs, .toml), similar to the C++ sources (like MIRJob.h and others); we have adopted SPDX headers but haven't transitioned yet
• style:
  • can you remove lines like //-------------------------...
  • two empty lines between: license, headers/pre-processor directives, namespace opening/closing, code propper (class declarations, methods implementation, etc.)
  • remove functionality related to mir_tool_call, representation_from (we're transitioning out of this)
  • rename the create... methods with make... (this is for consistency with the python bindings)

I don't see that this bindings makes optional use of metkit (but, it's also fine like this.)

It also is disabling tests, which might be part of an implicit contract that these are to be built separately from a main build that actually tests, and stops if tests fail? It is missing tests and/or examples, which are extremely valuable as a high-level documentation or starting points for a newcomer -- specifically the processing of GRIB messages to/from memory are extremely valuable. A unit test and a (small) Jupyter notebook at least?

Note that when I mean "I don't like" or "I prefer" it really is just personal preference and no criticism, just a point of discussion that might sway to a compromise we'd both be happy with!

[pmaciel]

These comments aren't necessary (I'm being pedantic)

[Choochmeque]

Fixed in a901981

[pmaciel]

I prefer long comments using the block version (/* /, or in this case /* */). Since you're refering to reusing MIRJob, also document that the input (MIRInput argument) is consumed with next()

[Choochmeque]

Fixed in edc850b

[pmaciel]

This comment says 3 times the same thing

[Choochmeque]

Fixed in a901981

[pmaciel]

Can this be made dynamic? We are already ahead of this, and with a planned 2.0 release this year, or early next year

[Choochmeque]

Fixed in cdfa37b

[pmaciel]

In our stacks we build metkit after atlas/before mir, maybe this order should generally be reflected here

[Choochmeque]

Fixed in b8dde12

[pmaciel]

Remove Low-level

[Choochmeque]

Fixed in 69ab838

[pmaciel]

I apologize for such a long letter - I didn't have time to write a short one.
— Mark Twain

Most of the comments below are about making the API consistent while it's cheap, so later additions follow a pattern. A few points are raised across the other -sys as a ecosystem (eckit-sys already sets conventions this crate follows).

Discussion 1. "Bridge" and namespace

• I wasn't aware of "Bridge" terminology (lack of experience), but the c++ namespace is a different question. For me namespace mir::bridge reads well and is compatible with the bindings header(s). Alternatively namespace mir::rust but it conflicts with ::rust:: types. namespace mir::ffi has the same nesting benefit without the collision, and ffi is also the Rust-side module name (pub mod ffi). Would you agree with the namespace change (I would vote on namespace mir::bridge)?
• As an ecosystem decision though, it should apply to eckit-sys (eckit_bridge, EckitBridge.h) and the other crates you're creating. This crate already refers to eckit_bridge::DataHandleWrapper, so here you might reason to keep with mir_bridge as it is.

Discussion 2. Job/Parametrisation/MIROutput set* naming

Rust has no overloading, so a one-to-one mapping forces typed names somewhere. But the available set of rust=facing methods ins't consistent between the classes that are supporting it, with Job/Parametrisation having a different set -- this "set of setters" doesn't need to completely implement the c++ side, but ideally be consistent together.

Going further, a shared trait for "settings" (set/clear/to_json), implemented for both Job and Parametrisation, would be clearer. Job.cc and Parametrisation.cc currently duplicate the conversions. A small template (or free functions taking the target by reference) would keep them in one place.

• Job has set_str_list, set_from_string and clear_key;
• Parametrisation has neither the string-list setter nor clear;
• JSON is json_str on Job, to_json on Parametrisation, and metadata_json on MIROutput;
• for both Job and Parametrisation: the scalar and slice setters (str, f64, i64, bool, &[f64], &[i64], &[&str]), clear, and to_json. set_from_string stays Job-only, since it's the mir tool's argument syntax;
• rename clear_key to clear, matching C++;
• use to_json throughout, since Rust's to_* naming signals a conversion;
• use &[&str] rather than &Vec<String> for the string-list setter (c++ supports rust::Slice<const rust::Str>), for consistency with the other slice setters and to avoid forcing an allocation.

Discussion 3: constructors/generators vs factories, make, OutputBox

• Factories in C++ are a keyed, polymorphic registry. I'd rename the // Factories sections to "Constructors" or I've also seen (and agree more) with "Generators"). If a keyed entry point is wanted later, a new MIRInputFactory::build / MIROutputFactory::build method would be the real factory.
• "OutputBox" here conflicts with mir's output and bounding-box vocabulary. I'd call it OutputCallback (or MessageSink), with OutputCallback::new(f) replacing make_output_box.

Discussion 4. Versions: a single source of truth

• The points below point to documenting as a top-level entry in the main Rust notes/readme/example (or what's appropriate)
• mir-sys crate version 1.30.1 duplicates the repository base VERSION. Cargo can't read the file, so the drift test is the right minimum. To catch drift before cargo test runs, the same check could live in build.rs (a hard error for in-tree builds, which it already detects), or releases could set the version from VERSION. One of those, rather than relying on remembering to bump it.
• ECBUILD_TAG = "3.13.1" is pinned here and again in eckit-sys, while mir requires ecbuild 3.4 (but I agree, that's too low). Suggestion: eckit-sys builds or clones ecbuild once and publishes its location through its links metadata (cargo:ecbuild=…), and dependent crates use DEP_ECKIT_SYS_ECBUILD. That gives one pin for the whole family, one clone per build, and no version spread.
• minimum:** cmake_find_package("mir", "1.28.2") is another hard-coded version. Worth a named constant with a comment on why 1.28.2 (the oldest release with the APIs the glue uses -- not sure?), so it's updated deliberately.
• branch = "develop", branch = "rust-bindings", branch = "pre-publish", and eccodes-sys from a "playground" repository. Together with Cargo.lock being ignored, builds aren't reproducible, and CI can change behaviour without any commit here. Pinning revs (as is already done for eccodes-sys and bindman) and committing Cargo.lock would fix both. Current Cargo guidance is to commit the lockfile for workspaces with tests/CI. Publishing to crates.io will also need registry dependencies eventually, which is worth tracking.

Discussion 5. LibMir::version/git_sha1

I don't think this will be of benefit for Rust, and removing the Rust-side hardcoding of 40 relieves a (tiny) maintenance burden, but is a good principle. Or, the eckit solution looks elegant too, eckit::system::Library::gitsha1(unsigned int count = 40) has the default, but mir::LibMir overrides it without one, so a call through LibMir& must pass the length. Calling it through the base reference picks up eckit's default.

The Python bindings hard-code gitsha1(40) for the same reason. The comment that LibMir is a type "because every bridge entry point has to be a member" isn't quite right: c++ supports namespaced free functions. Grouping the functions under a type is still a good choice for the Rust path (LibMir::version()); only the comment should change.

Discussion 6. Exposing MultiDimensionalInput

This is worth adding, since it's the general form behind from_multi_dimensional_grib_file (vector pairs for uv2uv / vod2uv from arbitrary sources, e.g. two memory messages or two raw arrays). Two constraints shape the design:

• MultiDimensionalInput::append(MIRInput*) takes ownership (it deletes the inputs in its destructor, and in next() once exhausted);
• our wrappers own storage that the inner input points into.

So the combined input has to take ownership of the child wrappers, not just their inner pointers:

static std::unique_ptr<MIRInput> from_components(rust::Vec<...> inputs);

Something like that? C++ can't pass Vec<UniquePtr<T>> directly. An append on a from_components()-built input avoids that and matches the C++ API. It's also worth documenting that each component must be 1-dimensional and step in lockstep (that is, next), since mir asserts on both.

Discussion 7. Loose points

• from_data_handle: GribDataHandleInput keeps an eckit::DataHandle&, but the Rust signature (Pin<&mut DataHandleWrapper>) doesn't tie the input's lifetime to the handle (should it? If so the signature would have to be different and take ownership). As it stands, the "must outlive" comment isn't enforced, so either:
  • take ownership (UniquePtr<DataHandleWrapper>, stored in the wrapper like message_), which is the simplest (and superior?) option; or
  • make it unsafe fn with the precondition documented (which would bring a bad practice in).
• The example needs eckit_sys::init() before anything else, and nothing enforces that. Consider a run-once init inside the constructors (or a documented mir_sys::init()), since this is the first thing a new user will have issues with.
• Exceptions header: I don't understand the fact this is special -- is this to capture/manage the exceptions from a Rust runtime? (yes I only need a short answer :-) )

Discussion 8. Tests

Only the version test runs in CI, there could be simple tests; There are (C++) derived classes from MIRInput/MIROutput (not using necessarily GRIB) that could be used to make simple, conceptual tests, also showing Rust API usage.

• from_gridspec → to_empty / to_resizable (check values().len() and the returned metadata);
• from_raw with a Parametrisation → to_resizable;
• setting each value type on Job / Parametrisation → to_json round-trip;
• an invalid key or value → Err with the expected exception kind (this exercises the exception mapping);
• to_callback receiving messages, and (after D9) a failing callback surfacing as Err.

It would also help to extend track_cpp_api beyond MIRJob (at least SimpleParametrisation and LibMir), so upstream API drift is flagged for everything that's wrapped. I think here the testing would rather help a lot to build code and real-sized tests for applications.

[Choochmeque]

• The example needs eckit_sys::init() before anything else, and nothing enforces that. Consider a run-once init inside the constructors (or a documented mir_sys::init()), since this is the first thing a new user will have issues with.
• Exceptions header: I don't understand the fact this is special -- is this to capture/manage the exceptions from a Rust runtime? (yes I only need a short answer :-) )

Thanks for the detailed review. Commits are pushed to this branch; per discussion:

D1. Namespace. Keeping mir_bridge. The namespace only exists inside the C++ glue of the -sys crates; users go through the safe wrappers and never see it. Renaming only mir would leave it calling eckit_bridge::DataHandleWrapper from mir::bridge, and doing it family-wide is churn across every -sys crate for no user-visible gain.

D2. Setters.

• clear_key → clear (a0d6929), json_str → to_json (9240835).
• set_str_list takes &[&str] (a800494).
• The C++ conversions now live in one CRTP template, cpp/Settings.h, inherited by both Job and Parametrisation (972b696). Parametrisation gained set_str_list and clear, so both have the same setter set (ec00c1e).
• MIROutput::metadata_json became metadata(), returning &Parametrisation, so the call is output.metadata()?.to_json() (59577a2). output.to_json() would read as serialising the output itself.
• No shared Rust trait: once the method sets match, a trait belongs in the safe wrapper crate rather than in -sys.

D3. "Factories" sections are now "Constructors" (165b16b); OutputBox is now OutputCallback, built with OutputCallback::new(f) (0c7a106).

D4. Versions.

• The system minimum is a named constant, MIR_MIN_VERSION = "1.28.2" (01f23b9). I checked it: every header the glue uses exists in 1.28.2 with the same signatures.
• Drift between the crate version and VERSION stays a warning in build.rs plus the CI test. A hard error in build.rs would break every downstream git build whenever develop bumps VERSION before Cargo.toml.
• Committing Cargo.lock, pinning revs, and a single ecbuild pin through eckit-sys are deferred to a follow-up. Note that eckit-sys cannot be pinned by rev here while atlas-sys and metkit-sys depend on it by branch = "develop"; cargo would resolve two eckit-sys crates with the same links. Keep in mind current configuration with github links are temporal. I am preparing crates for publishing to crates.io and it will cleanup all this mess and build become reproducible.

D5. git_sha1 calls gitsha1() through the eckit::system::Library& base, so the length comes from eckit's default, and the comment about entry points having to be members is corrected (088ccb2).

D6. Added MIRInput::from_components() and append(component) (71a5306). append hands the component's inner input to MultiDimensionalInput and keeps the emptied wrapper, with the storage it reads from, alive in the combined input. append fails with UserError on an input not built by from_components, and with AssertionFailed on a null component. The docs state that components must be 1-dimensional and step together through next. Tests are in 114b7f2; there is no uv2uv end-to-end test yet, as that needs vector GRIB data.

D7.

• from_data_handle now takes ownership: UniquePtr<DataHandleWrapper>, stored in the wrapper ahead of the input, plus a null check (2e30259). As you said, the old signature let safe Rust drop the handle under a live input.
• Added mir_sys::init(), documented in the crate docs and README, and used by the example (7c43039). It runs once through std::sync::Once (bae5d23): the parallel tests showed that concurrent first calls to eckit_sys::init() race in RustMain::initialise, which I'll fix in eckit-sys separately.
• Exceptions header: nothing to do with Rust-side exceptions. build.rs parses mir/util/Exceptions.h to generate the Rust Error enum (one variant per mir::exception class). The docs-headers/ symlink is there so docs.rs, which skips the native build, can still generate it.

D8. Tests.

• Inputs and outputs (66ea1f3): raw → resizable reproduces "Interfacing 2" from tests/unit/raw_memory.cc exactly, values and metadata; gridspec → empty runs, and gridspec → resizable gives 120 × 61 values with {"grid":[3,3]}.
• Exception mapping (23394fd): SeriousBug, AssertionFailed and mir::CannotConvert each arrive as the right typed error.
• to_callback (9f952f9): the callback receives exactly one GRIB message, byte-identical to what to_grib_memory holds. Writing that test showed the vendored build lacked AEC, because mir-sys turned off eccodes-sys's default features without re-enabling aec, so GRIB2 with CCSDS packing failed; eccodes-sys/aec is now enabled (f44b505).
• track_cpp_api stays on MIRJob only. On SimpleParametrisation and LibMir it would mostly produce ignore lists (we wrap a small slice of each), it only catches additions (signature changes already break the glue at compile time), and it panics in build.rs, so a newer system mir with one extra method would break an otherwise working build.