-
Notifications
You must be signed in to change notification settings - Fork 202
Async VM #2155
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: antonio/vm
Are you sure you want to change the base?
Async VM #2155
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
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 4809dfd in 2 minutes and 15 seconds. Click for details.
- Reviewed
383
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
1
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-vm/src/vm.rs:818
- Draft comment:
LLM function arguments are currently hardcoded to an empty map. Make sure to implement proper conversion of VM values to runtime values if needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is pointing out something that the code author already knows and has marked with a TODO comment. The comment essentially just restates what's already in the code as a TODO. It's not providing any new information or specific guidance on how to implement the conversion. The comment could be seen as reinforcing the importance of implementing proper value conversion. Maybe there are security or correctness implications that make this worth emphasizing. While proper value conversion is important, the author has already acknowledged this with their TODO comment. The review comment doesn't add any new information or specific guidance that would help implement the conversion. The comment should be deleted since it merely restates what's already marked as a TODO in the code without providing additional actionable guidance.
Workflow ID: wflow_qZSO2GUFVrkgRVJY
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
// Either async LLM call or regular function call. | ||
if self.llm_functions.contains(app.name.name()) { | ||
self.emit(Instruction::DispatchFuture(app.args.len())); | ||
self.emit(Instruction::Await(app.args.len())); |
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.
Instruction::Await is defined without any arguments. Remove the parameter (app.args.len()) when emitting Await.
self.emit(Instruction::Await(app.args.len())); | |
self.emit(Instruction::Await); |
self.tx.send((future, function_name, value)).unwrap(); | ||
} | ||
|
||
pub fn recv(&self) -> (usize, baml_types::BamlValue) { |
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.
Avoid a busy spin loop in AsyncChannel::recv; consider yielding the thread or using a blocking recv to prevent high CPU usage.
Important
Adds async LLM function support to BAML VM with new instructions and async handling.
baml-compiler/src/lib.rs
andbaml-vm/src/vm.rs
.DispatchFuture
andAwait
instructions inbaml-vm/src/bytecode.rs
.Vm::exec()
inbaml-vm/src/vm.rs
.llm_functions
toCompiler
struct inbaml-compiler/src/lib.rs
.compile()
to handle LLM functions.Future
enum toObject
inbaml-vm/src/vm.rs
.AsyncChannel
for async communication inbaml-vm/src/vm.rs
.Cargo.toml
to includetokio
andbaml-types
dependencies.This description was created by
for 4809dfd. You can customize this summary. It will automatically update as commits are pushed.