Conversation
hellovai
commented
Feb 25, 2025
hellovai
commented
- compile for go!
- gitignore
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
YourTechBud
left a comment
YourTechBud
left a comment
There was a problem hiding this comment.
Had some thoughts after revisiting this code.
| for result := range callback { | ||
| // TODO: Handle the result | ||
| // error handling, type checking, etc. | ||
| return_channel <- DoSomethingStreamResult{StreamResult: StreamResult[string, string]{partial: &result.data}} |
There was a problem hiding this comment.
This will block this goroutine and hence the trigger callback goroutine untill the user reads the message from this channel. This can cause the rust thread to block as well. I suggest you make the channel on line 65 a buffered channel to avoid blocking on this line
There was a problem hiding this comment.
it doesn't look like its blocking! ran a test and it runs all 3 in parallel from what i see
There was a problem hiding this comment.
The user may not read the channel at the pace you are. What if the user decides to do something compute intensive in the range loop on the receiving end of this channel?
There was a problem hiding this comment.
Can confirm with an unbuffered channel this is a blocking send until the consumer reads.
| } | ||
| } | ||
|
|
||
| func create_unique_id() (C.uint32_t, chan ResultCallback) { |
There was a problem hiding this comment.
Why not return a clean go type here? Convert to corresponding C type where it's being consumed
| "unsafe" | ||
| ) | ||
|
|
||
| type StreamResult[Partial any, Final any] struct { |
There was a problem hiding this comment.
We should confirm if gophers apart from me are comfortable to get a stream of such an object
There was a problem hiding this comment.
What would the other option be?
There was a problem hiding this comment.
That's what I'd like to know as well. I recommend this way myself.
There was a problem hiding this comment.
I think regardless of channel vs callback this is definitely the 'right' data structure.
Curious though, and this may be my lack of knowledge w/ baml streaming. When would Partial and Final be a different type?
| return result.error | ||
| } | ||
|
|
||
| type DoSomethingStreamResult struct { |
There was a problem hiding this comment.
Wr can honestly dump this. Its not really adding any value.
| } | ||
|
|
||
| // UpperCase in Go only! | ||
| func DoSomethingStream(arg string) <-chan DoSomethingStreamResult { |
There was a problem hiding this comment.
I'd be cool if we returned StreamResult directly here. Finalize the generic types in the declaration here itself.
| C.register_callback((C.callback_fcn)(C.trigger_callback)) | ||
| runtime = C.create_baml_runtime() | ||
| if runtime == nil { | ||
| panic("Failed to create Baml runtime") |
There was a problem hiding this comment.
This would have to be documented. I really dont mind baml exposing a NewClient method which holds the instance of the runtime within it. All Baml functions could be methods of this object
There was a problem hiding this comment.
Yes please, panic on init would be less than desirable!
Plus doing it as @YourTechBud mentions would allow you to have multiple baml runtimes (if that ever is helpful) and let you use runtime.AddCleanup to destroy the underlying runtime
| func main() { | ||
| channel := DoSomethingStream("Hello, world!") | ||
| for result := range channel { | ||
| if result.IsPartial() { |
There was a problem hiding this comment.
We just need one of IsPartial and IsFinal. Don't need both of em. Also, i know this is a sample, but please do the error check first. Lol.
|
closing in favor of hellovai-go branch |
3 participants
