Add Rust Bindings#1187
Conversation
There was a problem hiding this comment.
I highly recommend following the library and test structure of the existing bindings unless there's a compelling reason to do otherwise. It makes the bindings easier to synchronize during API and test changes. Furthermore, all the tests should be ported since that has caught binding bugs before.
As for the name, I'd prefer just sleipnir because I've found the common name reduces confusion. The PyPI package is still sleipnirgroup-jormungandr because PyPI doesn't allow renames.
You can use https://github.com/calcmogul/Sleipnir/blob/rust-bindings/.github/workflows/rust.yml to add CI.
|
|
||
| fn solve(guess: &f64) -> Result<MultistartResult<f64>, SleipnirError> { | ||
| let arena = VariableArena::new(); | ||
| let mut problem = Problem::new(&arena); |
There was a problem hiding this comment.
To save some user boilerplate, you might be able to have Problem, Gradient, Hessian, and Jacobian contain their own arenas. The Java wrapper in wpimath does something like this with VariablePool so that try-with-resources frees all the Variables deterministically.
|
For the name it looks like Sleipnir is already taken on crates, you are able to do what jormungandr does where the package name and the library name differ but that's usually discouraged. The crate naming right now is a mismatch of what its supposed to be, when its confirmed what the final name should ill clean everything up |
We worked around that in other package registries by using
Well, I really don't like the idea of naming the imports something other than |
|
I'm using this at work now and having it on crates would make certain things much simpler. If a good naming solution can be decided on I can just publish my fork under an arbitrary name and just use it for myself. Perhaps sleipnir-nlp or something along those lines? I'm working on getting wpiformat and clang tidy working locally for those to pass and ill look at the windows failures too. Seems to be in the setup of the workflow I copied in from your forks branch probably not the actual code. |
For what it's worth, you can specify a Git repo as a dependency via: [dependencies]
sleipnir = { git = "https://github.com/SleipnirGroup/Sleipnir.git", tag = "v0.5.3" }The lockfile includes the specific hash so you can do supply chain audits. Here's a strawman example: [[package]]
name = "sleipnir"
version = "0.5.3"
source = "git+https://github.com/SleipnirGroup/Sleipnir?tag=v0.5.3#1234567890123456789012345678901234567890"
dependencies = [
"cmake",
"cxx",
"cxx-build",
"ndarray",
"rayon",
"thiserror 2.0.17",
]Tho that doesn't help if there's an organizational requirement that everything be on crates.io.
The lint-format workflow uploads a patch you can apply to fix wpiformat's errors. The clang-tidy errors are a three-line fix:
The following part should only be applied to non-Windows parts of the build matrix, but it's currently applied to all of them: env:
RUSTC_WRAPPER: sccache
SCCACHE_GHA_ENABLED: true |
calcmogul
left a comment
There was a problem hiding this comment.
FYI, this is going to take me a long time to review fully, since I have to comb through every line.
| let eigen_include = find_dep_include(&build_dir, "eigen").unwrap_or_else(|| { | ||
| panic!( | ||
| "could not locate fetched Eigen headers under {}", | ||
| build_dir.display() | ||
| ) | ||
| }); |
There was a problem hiding this comment.
Is this a failure mode that can actually occur in practice? TrajoptLib hasn't needed to bother with them. Even if it did occur, CMake gives a pretty understandable error. Does the cxx crate not expose CMake's stderr contents?
| .define("SLEIPNIR_BUILD_BENCHMARKS", "OFF") | ||
| .define("SLEIPNIR_BUILD_EXAMPLES", "OFF") | ||
| .define("SLEIPNIR_BUILD_PYTHON", "OFF") | ||
| .define("CMAKE_POSITION_INDEPENDENT_CODE", "ON") |
There was a problem hiding this comment.
Is this flag here to support a dynamically-linked Rust library?
| println!("cargo:rustc-link-search=native={}", install_lib.display()); | ||
| println!( | ||
| "cargo:rustc-link-search=native={}", | ||
| dst.join("lib64").display() |
There was a problem hiding this comment.
Why is lib64 being searched for libraries?
| @@ -0,0 +1,3 @@ | |||
| [workspace] | |||
| members = ["rust"] | |||
| resolver = "2" No newline at end of file | |||
| // a C++23 toolchain. `<print>` is the canonical smoke test: it | ||
| // ships in GCC 14+ libstdc++ and libc++ 19+. Probe it up front so | ||
| // the user gets a single clear error instead of a compiler | ||
| // diagnostic buried 500 lines into a cmake build log. |
There was a problem hiding this comment.
I understand the rationale for including this, but it adds a lot of complexity to the build script. Does cxx have a better way to do this?
| /// have the same column count and share the same arena. The arena is | ||
| /// extracted from `top` — which must therefore be a `VariableMatrix` | ||
| /// (compile error if it's a bare `ndarray::Array2`). | ||
| pub fn vstack<'a, A, B>(top: A, bottom: B) -> VariableMatrix<'a> |
There was a problem hiding this comment.
vstack and hstack don't exist in the other bindings. Is a function like block() not feasible in Rust?
| ffi::variable_matrix_set_value(self.as_pin_mut(), &flat); | ||
| } | ||
|
|
||
| pub fn t(&self) -> VariableMatrix<'arena> { |
There was a problem hiding this comment.
The C++ API violated snake case in favor of T() because it represents the transpose operator ᵀ.
| } | ||
|
|
||
| /// Constant-valued matrix from an `ndarray::Array2<f64>`. | ||
| pub fn from_array_in(arena: &'arena VariableArena, values: &Array2<f64>) -> Self { |
There was a problem hiding this comment.
Is ndarray the linalg library we want to build Sleipnir around? For context, the other bindings designed themselves to mimic the linalg library of choice to make usage less jarring.
|
|
||
| /// Clears the wall-clock timeout. | ||
| #[inline] | ||
| pub fn no_timeout(mut self) -> Self { |
There was a problem hiding this comment.
Why would the user set a timeout just to unset it again? The default state is no timeout.
| } | ||
| } | ||
|
|
||
| best.ok_or_else(|| last_err.unwrap_or(SleipnirError::LocallyInfeasible)) |
There was a problem hiding this comment.
Why does this use LocallyInfeasible?
|
That's fine, It has been working amazingly for what I needed it for so atleast the core functionality is there. Really really nice library to use, big fan. |
Adds rust bindings under the
hafgufacrate name (open to changing it I looked up "norse myth crab" and that came up).This implementation tries to stay as true to the CPP DSL as possible but has been forced to vary in 2 main ways.
Variable Arena
Due to the CPP Variable objects having to be behind a UniquePtr due to the ffi boundry the rust wrapper couldnt implement
Copy. The downside of this is you would have to add&literally everywhere.For example
problem.minimize(x * x + y * y);turned intoproblem.minimize(&(&x * &x + &y * &y));.To get around this i made the Variable struct hold a immutable pointer to the same location the UniquePtr pointed and stored the UniquePtr in a
VariableArena. I manage to maintain memory safety and lifetime invariance by giving Variable a lifetime parameter which will always be that of the lifetime of the arena which stores its corresponding CPP Variable. After this I was able to make the Variable implement Copy drastically cleaning up the api. I did not do this for VariableMatrix due to worries of memory usage because every intermediate value gets stored in the arena until it is dropped.Comparison and Equality Operands
Due to rust not being able to return arbitrary types from
==,>=,<=,<and>thecmp!macros andsubject_to!macros were implemented. They work likesubject_to!(problem, x + y == 1.0);andlet c = cmp!(x >= 1.0);where c is an inequality constraint. cmp!/subject_to! can be nested and works with any expression no matter how complex as long as there is only a singular constraint.Features
multistart: uses rayon to handle multi-start natively in rustdiagnostic: can be enabled to add sleipnir diagnostic printingI have not tested performance but i have added rust versions of all the examples so those can be used to do so.
For full transparency much of this code was written by AI due to most of it simply being boilerplate. I have tried to add sufficient tests and have looked over the code for blatant mistakes the best i can.