-
Notifications
You must be signed in to change notification settings - Fork 996
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
add rust bindings #736
add rust bindings #736
Conversation
Happy to get this towards merging once the testing setup is sorted out :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #736 +/- ##
==========================================
- Coverage 69.67% 65.69% -3.98%
==========================================
Files 55 55
Lines 4059 4061 +2
==========================================
- Hits 2828 2668 -160
- Misses 1231 1393 +162 ☔ View full report in Codecov by Sentry. |
Hi @slundberg & @Harsha-Nori - This PR is ready for review, and unlike earlier where I noted the need for it was speculative, I can confirm we'd like to integrate a Rust CFG engine shortly. For this PR, I'd like to call special attention to the possibility that pypi_upload.yml might fail if the pypa images do not contain a rust compiler. I was not able to, nor would want to, test it at this time. We should keep it in mind though for the next upload in case there is a failure relating to it. |
I think this LGTM, though good call out on the potential pypa issues. Do you think we should do a release before merging @paulbkoch, and/or do an attempted release and see what the failure modes are here? |
Given I've never released any Rust code, my main concern with adding this kind of dependency are the unknown unknowns. Things like it breaking manulinux, or mac arm builds, or adding a requirement to install some kind of other shared library on some linux flavor that I'm currently not aware of, or more likely something completely different. I wouldn't necessarily put out a release to test the pypa question, but I do think it would be good to put out a release sometime between when this PR is merged and when we move the engine into Rust. Once the engine moves into rust, if we put out a release and it does something like crashes every 100 LLM calls then we're in an emergency kind of situation. My preference would be to have a release with just a bare Rust dependency and see if it causes issues. If it does, it's easy to remove and debug at leisure. Even better would be to merge this PR, wait a week or two or three, and then put out a release. We have people installing from source, so that might expose some issues by itself without needing a release. Actually, this gives me an idea. I'd like to add one little thing: a call to the Rust code just to get verification that works and doesn't cause issues on any installed configurations. |
This PR is a bit speculative at the moment, but will be required if the engine is moved to Rust.