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

Replace basic Numba FSM functions with Rust implementations #7

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

brandonwillard
Copy link
Member

@brandonwillard brandonwillard commented Aug 20, 2024

This PR introduces a simple Rust extension that replaces the index construction code.

We can base our work towards a primarily Rust project (i.e. #4) on these changes, which will allow us to make use of outlines-core in outlines much sooner and permit parallel work on more code conversion. We should also be able to apply straightforward changes to this setup that make the Python bindings and PyO3 dependencies optional (e.g. for Rust-only use of the underlying crate).

  • Replace Python implementations with Rust extension
  • Make sure tests pass with Rust extension
  • Make sure benchmarks pass with Rust extension

@brandonwillard
Copy link
Member Author

FYI: The failure is only due to missing coverage, which could be related to a distinct configuration issue: dottxt-ai/outlines#1105.

@rlouf
Copy link
Member

rlouf commented Aug 20, 2024

Looks good to me, especially the benchmarks vs main:

Screenshot Capture - 2024-08-20 - 21-13-24

Copy link
Collaborator

@ErikKaum ErikKaum left a comment

Choose a reason for hiding this comment

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

Nice, thank you Brandon 👍

As far as I can tell it looks good. Small nits on naming that don't need to be addressed now, but rather when there's a clearer separation between rust crate and bindings.

  • rust_fsm --> maybe outlines-core-rs
  • _walk_fsm_internal as to not mix python & rust naming conventions

I didn't find any obvious bugs in the fsm implementation itself, but I'm not as experienced as you either. In other words, good if someone else can double check it.

@rlouf
Copy link
Member

rlouf commented Aug 20, 2024

I didn't find any obvious bugs in the fsm implementation itself, but I'm not as experienced as you either. In other words, good if someone else can double check it.

Note that the tests that were in Outlines all pass with this implementation.

rlouf
rlouf previously approved these changes Aug 20, 2024
@brandonwillard
Copy link
Member Author

  • rust_fsm --> maybe outlines-core-rs
  • _walk_fsm_internal as to not mix python & rust naming conventions

Done

Copy link
Collaborator

@drbh drbh left a comment

Choose a reason for hiding this comment

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

LGTM! and benchmarks look really good. Great job @brandonwillard

not sure if the "Tests / Combine & check coverage. (pull_request)" are important?

@brandonwillard
Copy link
Member Author

brandonwillard commented Aug 20, 2024

LGTM! and benchmarks look really good. Great job @brandonwillard

Most of this is adapted from our initial internal work by @kc611, which was exclusively focused on obtaining parity with Numba. There are many more big improvements to make, but it does have parity and AOT compilation, so it's a worthy replacement for the code in outlines.

not sure if the "Tests / Combine & check coverage. (pull_request)" are important?

It's not a problem; see #7 (comment).

@brandonwillard brandonwillard merged commit e48a5f4 into main Aug 20, 2024
4 of 5 checks passed
@brandonwillard brandonwillard deleted the add-rust-extension branch August 20, 2024 20:18
@rlouf rlouf linked an issue Aug 21, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hook the Rust-based library to the existing benchmarks
4 participants