-
Notifications
You must be signed in to change notification settings - Fork 269
[WIP] Native Rust codegen: BAML IR to Rust types with Client Templates #2348
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
base: canary
Are you sure you want to change the base?
Conversation
- Add BAML-RUST.md: Comprehensive analysis of Rust client implementation approach - Add BAML-GO.md: Deep dive into Go client FFI architecture and performance - Document why OpenAPI-generated Rust clients are limited (~60% feature loss) - Outline implementation plan for native Rust client generator - Compare direct FFI clients vs HTTP-based clients 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🚀 Code Review Initiated The review process for this pull request has started. Our system is analyzing the changes for:
You will receive structured and actionable feedback shortly! ⏳ |
@CeciliaZ030 is attempting to deploy a commit to the Boundary Team on Vercel. A member of the Team first needs to authorize 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.
Caution
Changes requested ❌
Reviewed everything up to 2309b48 in 6 minutes and 9 seconds. Click for details.
- Reviewed
7760
lines of code in30
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/baml-types/src/generator.rs:35
- Draft comment:
The new Rust variant is added with default client mode set to Sync. Please double-check whether Sync is the desired default for Rust (vs. async) and document the rationale. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment asks the PR author to double-check their choice and document the rationale, which violates the rule against asking for confirmation or explanation. It does not provide a specific suggestion or point out a clear issue with the code.
2. engine/generators/languages/rust/src/type.rs:305
- Draft comment:
There is a duplicate implementation ofsafe_rust_identifier
. Consolidate these into a single utility function to avoid maintenance issues. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. engine/language_client_go/pkg/cffi/cffi.pb.go:328
- Draft comment:
The generated protobuf code now omits the combined check forprotoimpl.UnsafeEnabled && x != nil
and always checks forx != nil
. Ensure this change is intentional and document the rationale if it’s due to an update in protoc-gen-go. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment is asking the author to ensure that a change is intentional and to document the rationale. This violates the rule against asking the author to confirm their intention or to update documentation. However, it does point out a specific change in behavior, which could be useful if it was framed differently.
Workflow ID: wflow_nAH9TterNQA9PnWn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
}} | ||
|
||
"#, | ||
func.name.to_lowercase(), |
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.
Using to_lowercase()
on the function name may be insufficient if the original name isn’t already in snake_case. Consider applying a proper snake_case conversion (e.g. via the provided to_snake_case
in utils) to ensure valid Rust identifiers.
path.push_str("crate::"); | ||
} else if i > 0 { | ||
path.push_str(part); | ||
path.push_str("::"); |
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.
The module path built in relative_from
appends '::' after each part. Consider trimming the trailing separator for a cleaner module path when concatenating with type names.
cFunctionName := C.CString(functionName) | ||
defer C.free(unsafe.Pointer(cFunctionName)) | ||
|
||
result := C.WrapCallFunctionStreamFromC(runtime, cFunctionName, cEncodedArgs, C.uintptr_t(len(encodedArgs)), C.uint32_t(id)) |
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.
Typo: The variable 'cEncodedArgs' is used here, but the function parameter is named 'encodedArgs'. This appears to be a lexicographical error and should be corrected.
result := C.WrapCallFunctionStreamFromC(runtime, cFunctionName, cEncodedArgs, C.uintptr_t(len(encodedArgs)), C.uint32_t(id)) | |
result := C.WrapCallFunctionStreamFromC(runtime, cFunctionName, encodedArgs, C.uintptr_t(len(encodedArgs)), C.uint32_t(id)) |
@@ -113,6 +113,9 @@ pub fn check_version( | |||
GeneratorOutputType::Go => { | |||
format!("go install github.com/boundaryml/baml/go@{gen_version}") | |||
} | |||
GeneratorOutputType::Rust => { | |||
format!("cargo install baml-py --version {gen_version}") |
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.
Typo note: The Rust branch command uses "cargo install baml-py --version {gen_version}". Is "baml-py" the intended package name for Rust, or should it be something like "baml-rs" to match the Rust target? Please verify.
format!("cargo install baml-py --version {gen_version}") | |
format!("cargo install baml-rs --version {gen_version}") |
- Refactor Rust client generation templates for improved code structure - Remove redundant runtime.rs.j2 template - Update generator configuration to support Rust client output - Add Rust generator to BAML test configuration 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
➜ generators: ✗ cargo test --package generators-rust
Summary
Adds first-pass native Rust code generation:
Repo: aomi-labs/baml
What's changed
engine/generators/languages/rust/src/**
to emit native Rust types from IR.Testing
Next PR
rust_language_client
.Checklist
rust_language_client
(next PR)Important
Adds Rust code generation for BAML IR using Askama templates, with support for classes, enums, unions, and function stubs.
engine/generators/languages/rust/src/**
.generate_sdk_files
inlib.rs
to generate Rust SDK files.Cargo.toml
andlib.rs
templates for Rust client.generate_sdk
ingenerators_lib/src/lib.rs
for Rust codegen (marked astodo
).cffi.pb.go
for CFFI protocol buffer changes.This description was created by
for 2309b48. You can customize this summary. It will automatically update as commits are pushed.