-
Notifications
You must be signed in to change notification settings - Fork 97
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
Addressed issue where dynamic types were not streaming. #1606
base: canary
Are you sure you want to change the base?
Conversation
- Updated `ir_helpers/mod.rs` to introduce `IRHelperExtended` and `IRSemanticStreamingHelper` traits, adding methods for class field management and streaming behavior. - Modified `mod.rs` to export the new traits and updated relevant imports across the codebase. - Refactored `validate_streaming_state` and `process_node` functions in `semantic_streaming.rs` to utilize the new traits for improved type handling. - Adjusted various modules to implement the extended functionality, ensuring better integration with runtime context and type management. This update improves the flexibility and usability of the IR helper functions, facilitating better handling of class fields and streaming requirements.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
👍 Looks good to me! Reviewed everything up to 654f1e5 in 3 minutes and 14 seconds
More details
- Looked at
2416
lines of code in18
files - Skipped
0
files when reviewing. - Skipped posting
22
drafted comments based on config settings.
1. engine/language_client_codegen/src/ruby/generate_types.rs:43
- Draft comment:
Typo in doc comment: "TypeDefinedjInBaml" likely should be "TypeDefinedInBaml". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/language_client_codegen/src/typescript/generate_types.rs:260
- Draft comment:
The union handling in to_partial_type_ref uses join and conditional wrapping. Consider clarifying the intended TS type semantics with explicit parentheses and consistent nullability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-lib/jsonish/src/deserializer/semantic_streaming.rs:89
- Draft comment:
Nested iterative transformations perform multiple clones and unwraps. Consider refactoring to reduce cloning and add better error context. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/baml-runtime/src/internal/prompt_renderer/scoped_ir.rs:221
- Draft comment:
The pattern matching on FieldType::WithMetadata is extensive. Ensure that recursive alias merging (lines 221–236) is thoroughly tested against deeply nested metadata. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that a specific part of the code is thoroughly tested. This falls under the rule of not asking the author to ensure the change is tested. The comment does not provide a specific suggestion or point out a specific issue with the code.
5. engine/language_client_codegen/src/typescript/mod.rs:387
- Draft comment:
The to_partial_type_ref implementation returns a tuple (String, bool); consider renaming the bool or adding documentation to clarify its meaning. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. engine/type_builder/mod.rs:71
- Draft comment:
Frequent use of unwrap() on lock() calls; consider using expect() with clear messages for better diagnostics in production. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. engine/language_client_codegen/src/ruby/mod.rs:57
- Draft comment:
Templates for generating client code are spread out; consider centralizing common logic (e.g. docstring rendering) to ease future maintenance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. engine/language_client_codegen/src/ruby/generate_types.rs:43
- Draft comment:
Typo in comment: "TypeDefinedjInBaml" should be "TypeDefinedInBaml". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. engine/language_client_codegen/src/typescript/generate_types.rs:168
- Draft comment:
Consider clarifying the return value and boolean flag from to_partial_type_ref. The tuple (String, bool) is not self-explanatory; adding comments or renaming the bool to 'isOptional' will improve readability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. engine/baml-runtime/src/internal/llm_client/mod.rs:425
- Draft comment:
Tracing logic is duplicated across branches when finishing spans. Consider extracting common tracing functionality to reduce code repetition and improve maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. engine/baml-runtime/src/type_builder/mod.rs:60
- Draft comment:
Be mindful of potential performance overhead due to frequent cloning when converting metadata in From<&Arc<Mutex>> for PropertyAttributes. If metadata is large, consider if sharing references could be optimized. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. engine/baml-runtime/src/internal/prompt_renderer/scoped_ir.rs:19
- Draft comment:
The enum FindResult is used to capture override lookup results. Although its variants are self‐descriptive, consider adding documentation comments for each variant to clarify their intended use and improve code maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The enum is fairly simple and its purpose is clear from its usage in the code. The variant names are descriptive and self-documenting. The pattern matching code that uses it makes the intent obvious. Documentation would be nice-to-have but not essential for maintainability.
Documentation is generally good practice. The enum is used in multiple places so having clear docs could help future maintainers.
While docs are good, this is a simple internal enum with clear usage patterns. The comment doesn't point out any actual confusion or maintenance issues.
This comment suggests a nice-to-have improvement but doesn't highlight a significant maintainability issue. The code is already clear without additional documentation.
13. engine/baml-lib/baml-core/src/ir/ir_helpers/mod.rs:509
- Draft comment:
Typo: In the comment for distribute_constraints, 'A constrained base type stors its constraints...' should be changed to 'stores' to fix the typographical error. - Reason this comment was not posted:
Comment was on unchanged code.
14. engine/baml-lib/baml-core/src/ir/ir_helpers/to_baml_arg.rs:354
- Draft comment:
Typo: In the unreachable! message, "FieldType::Constrainted" should probably be "FieldType::Constrained". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. engine/baml-lib/jsonish/src/deserializer/semantic_streaming.rs:57
- Draft comment:
Typo: In the comment above validate_streaming_state, 'vial@stream.with_state
' should be corrected to 'via@stream.with_state
'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
16. engine/baml-lib/jsonish/src/helpers/mod.rs:291
- Draft comment:
Typo found in the comment: 'Node that we set the StreamState toNone
unlessallow_partials
.' should be corrected to 'Note that we set the StreamState toNone
unlessallow_partials
.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
17. engine/baml-runtime/src/internal/llm_client/mod.rs:65
- Draft comment:
Typo: The comment says "Node that we set the StreamState toNone
unlessallow_partials
." It should be "Note that we set the StreamState..." to avoid confusion. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
18. engine/baml-runtime/src/internal/prompt_renderer/render_output_format.rs:211
- Draft comment:
Typo in comment: The comment above 'relevant_data_models' refers to a duplicate ofrelevant_dat_models
. Consider correcting it torelevant_data_models
to maintain consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. engine/baml-runtime/src/internal/prompt_renderer/scoped_ir.rs:20
- Draft comment:
Typographical error: The enum variant 'Overriden' appears to be misspelled. The correct spelling is 'Overridden'. Please update all occurrences accordingly. - 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%
While technically correct about English spelling, this is a variable name choice that's used consistently throughout the codebase. Changing it would require updates in multiple places and wouldn't affect functionality. The current spelling, while not standard English, is clear in meaning and consistently applied.
The comment is technically correct about English spelling. Variable naming standards are important for code readability.
However, this is a minor stylistic issue that doesn't affect functionality, and the current spelling is clear and consistently used. The cost of changing it outweighs the benefit.
Delete the comment. While technically correct, this spelling variation is consistently used and clear in meaning. The change would be purely cosmetic with no functional benefit.
20. engine/language_client_codegen/src/python/generate_types.rs:230
- Draft comment:
Typographical error: The comment on line 230 says 'Short-circuite with a None default value...' – 'Short-circuite' should be corrected to 'Short-circuit'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
21. engine/language_client_codegen/src/ruby/generate_types.rs:43
- Draft comment:
Typo detected in the comment: 'Partial'. Consider correcting it to 'Partial' if that's what was intended. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
22. engine/language_client_codegen/src/typescript/mod.rs:232
- Draft comment:
Minor typographical issue: the variable name 'typscript_client' is misspelled. Please correct it to 'typescript_client' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_uXQfv0WAKDt6s9ay
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
} | ||
}) | ||
.collect(), | ||
FindResult::OnlyDynamic(_) => Default::default(), |
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.
Dynamic-only classes don't need all their fields represented? (Don't need null fillers?)
let done = ir.distribute_metadata(&return_type).1 .1.done; | ||
let state = ir.distribute_metadata(&return_type).1 .1.state; |
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.
let done = ir.distribute_metadata(&return_type).1 .1.done; | |
let state = ir.distribute_metadata(&return_type).1 .1.state; | |
let done = ir.distribute_metadata(&return_type).1.1.done; | |
let state = ir.distribute_metadata(&return_type).1.1.state; |
I had no idea you can put a space between a record and a field accessor! Your code parses correctly, but maybe it's less surprising without the space.
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.
Left a couple comments - generally looks great, happy to have it merged!
I wonder if it'd be possible in the future to have:
fn add_runtime_context(ir: IntermediateRepr, ctx; RuntimeContext) -> IntermediateRepr {
...
}
perhaps by modifying IntermediateRepr
so that it can store whatever parts of RuntimeContext
are currently lacking from it, in order to monomorphise again some of the core functions.
ir_helpers/mod.rs
to introduceIRHelperExtended
andIRSemanticStreamingHelper
traits, adding methods for class field management and streaming behavior.mod.rs
to export the new traits and updated relevant imports across the codebase.validate_streaming_state
andprocess_node
functions insemantic_streaming.rs
to utilize the new traits for improved type handling.This update improves the flexibility and usability of the IR helper functions, facilitating better handling of class fields and streaming requirements.
Important
Enhance BAML IR with
IRHelperExtended
andIRSemanticStreamingHelper
traits for improved dynamic type streaming and class field management.IRHelperExtended
andIRSemanticStreamingHelper
inir_helpers/mod.rs
for enhanced class field management and streaming behavior.IntermediateRepr
andScopedIr
.validate_streaming_state
andprocess_node
insemantic_streaming.rs
to use new traits for improved type handling.parsed_value_to_response
inllm_client/mod.rs
andhelpers/mod.rs
to utilize new traits.mod.rs
and update imports across the codebase.runtime_interface.rs
andstream.rs
to support new traits.python
,ruby
, andtypescript
to align with new IR traits.This description was created by
for 654f1e5. It will automatically update as commits are pushed.