-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
experimental: Add minimal rust backend. #405
base: main
Are you sure you want to change the base?
Conversation
First pass at adding a simple rust backend that use nalgebra for its matrix library.
..._codegen_test_data/symengine/symforce_rust_codegen_test/src/backend_test_function_float32.rs
Show resolved
Hide resolved
test/symforce_rust_codegen_test.py
Outdated
TEST_DATA_DIR = ( | ||
path_util.symforce_data_root() | ||
/ "test" | ||
/ "symforce_function_codegen_test_data" | ||
/ symforce.get_symbolic_api() | ||
/ "symforce_rust_codegen_test" | ||
/ "src" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to actually compile the rust code to make sure we are generating valid rust code? One step further would be to write unit tests for the generated rust code. @aaron-skydio could you help me figure out the best path forward on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, yeah that seems like a great idea. Some thoughts:
- I'm guessing what we want to use to build the rust code is cargo, or rustc, called directly and not managed by cmake. In that case, I'm guessing the easiest way to do this is to write a python unit test which e.g. calls cargo to build the generated code and build a unit test that exercises the generated code, and then invokes the unit test. Unless it's easier to make this a cmake-native test than I'm expecting; it seems like it's probably unnecessary to add rust to our cmake build just for this one test
- I think we'll want to run that python test only if a rust toolchain is available, this is similar to what we do for numba and pytorch
- For actually running that test in CI, I think we should install a rust toolchain in one or more of the jobs here, and then your python unit test should run in those jobs
Does that all make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! I just added it to this test. For now I think if we just build these generated files thats probably sufficient. I think I added rust to the ci.yml correctly but I'm not sure how to kick off ci?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running CI on a PR here requires a maintainer approving each run - I can do that (I'll hit the button again now), but of course that doesn't work great for you to iterate on things. If you have GitHub Actions enabled on your fork, you can run the CI job manually on your branch from the Actions tab on your fork, like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh thanks. I don't use github all that much. I'm still getting quite a few errors on the cuda codegen because I changed the order in which some expressions get generated. I thought I updated all the necessary files but it still is giving me issues. Couldn't figure out tonight, but hopefully will track it down this weekend.
https://github.com/matte1/symforce/actions/runs/11809269710/job/32899285933
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Your totally right, sorry I thought I got them all. Still working through some more!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we have passing tests!
https://github.com/matte1/symforce/actions/runs/11839918889/job/32992525216
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! I'll try and get this merged soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the help with this @aaron-skydio !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if there is anything else I can do to help get this over the line!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! I took a stab at this a few weeks ago but I quickly gave up after getting stuck with the templates.
I know this is experimental and a work in progress but I couldn't help giving it a quick test. I only did a small test with some matrix multiplications but I ran into a few small issues so I figured I might as well leave some suggestions.
symforce/codegen/backends/rust/templates/function/FUNCTION.rs.jinja
Outdated
Show resolved
Hide resolved
symforce/codegen/backends/rust/templates/function/FUNCTION.rs.jinja
Outdated
Show resolved
Hide resolved
* Fixed issue with matrix creation * Used fully qualified nalgebra path * Add better support for generating / compiling tests * Added unit test for matrix math.
Thanks for testing / reviewing @avsaase. Took another quick pass at improving it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Great work, I'm excited for this. Overall this looks great, mostly had comments on some style things, and tried to answer your questions about testing and such
test/symforce_function_codegen_test_data/symengine/symforce_rust_codegen_test/Cargo.lock
Outdated
Show resolved
Hide resolved
..._codegen_test_data/symengine/symforce_rust_codegen_test/src/backend_test_function_float32.rs
Show resolved
Hide resolved
test/symforce_rust_codegen_test.py
Outdated
TEST_DATA_DIR = ( | ||
path_util.symforce_data_root() | ||
/ "test" | ||
/ "symforce_function_codegen_test_data" | ||
/ symforce.get_symbolic_api() | ||
/ "symforce_rust_codegen_test" | ||
/ "src" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, yeah that seems like a great idea. Some thoughts:
- I'm guessing what we want to use to build the rust code is cargo, or rustc, called directly and not managed by cmake. In that case, I'm guessing the easiest way to do this is to write a python unit test which e.g. calls cargo to build the generated code and build a unit test that exercises the generated code, and then invokes the unit test. Unless it's easier to make this a cmake-native test than I'm expecting; it seems like it's probably unnecessary to add rust to our cmake build just for this one test
- I think we'll want to run that python test only if a rust toolchain is available, this is similar to what we do for numba and pytorch
- For actually running that test in CI, I think we should install a rust toolchain in one or more of the jobs here, and then your python unit test should run in those jobs
Does that all make sense?
There might be some issues with our CI right now; I'm letting it run, but generally I need to fix up some things so there might be some failures that aren't related to this PR |
@aaron-skydio Thanks for the review! I did my best to address them and left them open for you to resolve. I think if we are happy with where this landed could you help me kick off CI so that I can get it all passing? |
..._codegen_test_data/symengine/symforce_rust_codegen_test/src/backend_test_function_float64.rs
Show resolved
Hide resolved
e67ade6
to
8726f8b
Compare
First pass at adding a simple rust backend that use nalgebra for its matrix library.