Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

matte1
Copy link

@matte1 matte1 commented Nov 2, 2024

First pass at adding a simple rust backend that use nalgebra for its matrix library.

First pass at adding a simple rust backend that use nalgebra for its
matrix library.
symforce/codegen/format_util.py Show resolved Hide resolved
symforce/test_util/backend_coverage_expressions.py Outdated Show resolved Hide resolved
symforce/test_util/backend_coverage_expressions.py Outdated Show resolved Hide resolved
Comment on lines 23 to 30
TEST_DATA_DIR = (
path_util.symforce_data_root()
/ "test"
/ "symforce_function_codegen_test_data"
/ symforce.get_symbolic_api()
/ "symforce_rust_codegen_test"
/ "src"
)
Copy link
Author

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.

Copy link
Member

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?

Copy link
Author

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?

Copy link
Member

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:

image

Copy link
Author

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

Copy link
Author

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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

Copy link
Author

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 !

Copy link
Author

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!

@matte1 matte1 mentioned this pull request Nov 2, 2024
Copy link

@avsaase avsaase left a 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.

* Fixed issue with matrix creation
* Used fully qualified nalgebra path
* Add better support for generating / compiling tests
* Added unit test for matrix math.
@matte1
Copy link
Author

matte1 commented Nov 5, 2024

Thanks for testing / reviewing @avsaase. Took another quick pass at improving it.

Copy link
Member

@aaron-skydio aaron-skydio left a 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

symforce/codegen/__init__.py Outdated Show resolved Hide resolved
symforce/codegen/backends/rust/README.rst Outdated Show resolved Hide resolved
symforce/codegen/backends/rust/rust_code_printer.py Outdated Show resolved Hide resolved
symforce/codegen/backends/rust/rust_code_printer.py Outdated Show resolved Hide resolved
symforce/test_util/backend_coverage_expressions.py Outdated Show resolved Hide resolved
Comment on lines 23 to 30
TEST_DATA_DIR = (
path_util.symforce_data_root()
/ "test"
/ "symforce_function_codegen_test_data"
/ symforce.get_symbolic_api()
/ "symforce_rust_codegen_test"
/ "src"
)
Copy link
Member

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?

test/symforce_rust_codegen_test.py Outdated Show resolved Hide resolved
@aaron-skydio
Copy link
Member

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

@matte1
Copy link
Author

matte1 commented Nov 11, 2024

@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?

@matte1 matte1 force-pushed the rust_backend branch 2 times, most recently from e67ade6 to 8726f8b Compare November 14, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants