-
Notifications
You must be signed in to change notification settings - Fork 168
Fetch API #1916
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?
Fetch API #1916
Conversation
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.
Important
Looks good to me! 👍
Reviewed 1f1eeb8 in 1 minute and 49 seconds. Click for details.
- Reviewed
217
lines of code in1
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-runtime/src/eval_expr.rs:323
- Draft comment:
Switching from using a custom reqwest client to reqwest::get simplifies the GET request, but verify that no custom client options (e.g. headers or timeouts) are needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. engine/baml-runtime/src/eval_expr.rs:326
- Draft comment:
The call to render_output_format now passes EvaluationContext::default() instead of the response body. Confirm that this change in parameters and behavior is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. engine/baml-runtime/src/eval_expr.rs:307
- Draft comment:
Consider using a consistent placeholder name in the error message for the arrow extraction block. The placeholder changed from '{fetch_value}' to '{fetch}' which may cause confusion. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_s2OH6yxM4fGLk8Oq
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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 2cd8d5f in 3 minutes and 12 seconds. Click for details.
- Reviewed
111
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
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. integ-tests/baml_src/test-files/builtin/fetch.baml:15
- Draft comment:
Typo: The string 'GPT4o' might be a typo. Did you intend to use 'GPT-4' or 'GPT4'? - 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% Since this is a new file and GPT4o appears multiple times consistently, it's likely an intentional choice for a model name or client identifier. Without more context about valid client names in this codebase, we can't be certain this is actually a mistake. The comment is speculative and asks for confirmation rather than pointing out a definite issue. I could be wrong if GPT4o is actually an invalid client name in this system. The consistent usage doesn't necessarily mean it's correct. Even if it were incorrect, the comment violates our rules by being speculative ("might be") and asking for confirmation rather than stating a definite issue. Delete the comment because it's speculative and asks for confirmation rather than pointing out a definite issue. The consistent usage of GPT4o suggests it's intentional.
2. integ-tests/baml_src/test-files/builtin/fetch.baml:49
- Draft comment:
Typo: The string 'GPT4o' might be a typo. Consider confirming if it should be 'GPT-4' or 'GPT4'. - 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 consistent usage of "GPT4o" across multiple functions suggests this is an intentional client name rather than a typo. Without access to the broader codebase or documentation, we can't be certain what valid client names are. The comment is speculative and asks for confirmation rather than pointing out a definite issue. I might be wrong about the intentionality - just because something appears multiple times doesn't mean it's correct. There could be a copy-paste error. While that's true, the rules state we should not make speculative comments or ask for confirmation. We need strong evidence that the comment is correct to keep it. The comment should be deleted as it's speculative and asks for confirmation without strong evidence that "GPT4o" is actually incorrect.
3. integ-tests/baml_src/test-files/builtin/fetch.baml:58
- Draft comment:
Typo: The string 'GPT4o' appears here. Please verify if this is correct or should be 'GPT-4' or 'GPT4'. - 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 comment is not actionable and is speculative, as it asks the author to verify something that may already be correct. 'GPT4o' is a valid model, so there is no strong evidence of a typo. The rules say not to ask the author to verify or double-check, and not to make speculative comments. Perhaps there is a chance that 'GPT4o' is a typo, but without strong evidence, we should not keep the comment. The rules are clear about not making speculative or verification requests. Given the rules and the lack of strong evidence, the comment should be deleted. If there was a clear typo or invalid model, it would be actionable, but that's not the case here. The comment should be deleted because it is speculative, not actionable, and there is no strong evidence of an error. It violates the rules about verification and speculation.
4. integ-tests/baml_src/test-files/builtin/fetch.baml:69
- Draft comment:
Typo: The string 'GPT4o' might be incorrect. Consider updating it to 'GPT-4' or 'GPT4' for consistency. - 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 assumes "GPT4o" is a typo, but its consistent usage throughout the file and the presence of another similarly formatted client name suggests this is an intentional naming convention. Without knowing the valid client names in this system, we can't assume this is incorrect. The comment is speculative and not based on strong evidence of an actual issue. I could be wrong if there's a standardized naming convention for GPT models in this codebase that I'm not aware of. Even if there is a standard naming convention, the consistent usage across the file suggests this is intentional, and changing it would require knowledge of the valid client names in the system. Delete the comment as it's speculative and lacks strong evidence that "GPT4o" is incorrect, especially given its consistent usage throughout the file.
Workflow ID: wflow_AZl9LnX5q0Tf3EOw
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
// } | ||
|
||
// function ChatResponse(query: string) -> string { | ||
// let needs_search = NeedsWikipediaSearch(query); |
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.
Computed needs_search
is never used. Consider using it to conditionally call SearchWikipedia
.
<!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Add type checking for class constructors, ensuring field types match and all required fields are present, with tests and parser updates. > > - **Type Checking**: > - Adds type checking for class constructors in `expr_typecheck.rs`, ensuring field types match expected types and all required fields are present. > - Handles missing fields and unknown classes with error diagnostics. > - **Intermediate Representation**: > - Introduces `builtin_ir()` in `builtin.rs` to provide built-in classes and functions. > - Modifies `IntermediateRepr` in `repr.rs` to include public fields for type checking. > - **Tests**: > - Adds `constructors_invalid.baml` to test invalid class constructor scenarios. > - Updates `if.baml` to test if expressions with newlines. > - **Parser**: > - Updates `datamodel.pest` to allow newlines in if expressions. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=BoundaryML%2Fbaml&utm_source=github&utm_medium=referral)<sup> for da25dce. You can [customize](https://app.ellipsis.dev/BoundaryML/settings/summaries) this summary. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
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.
Important
Looks good to me! 👍
Reviewed dc68799 in 2 minutes and 11 seconds. Click for details.
- Reviewed
146
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
8
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-core/src/ir/builtin.rs:92
- Draft comment:
Fixed typo in docstring: 'generics do not appear' is clearer. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative as it only mentions a fixed typo in a docstring. It doesn't provide any actionable feedback or suggestions for improvement.
2. engine/baml-lib/baml/tests/validation_files/expr/builtin.baml:5
- Draft comment:
Removed trailing whitespace from the 'userId' field. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative as it only states what was done without providing any suggestion, question, or potential issue related to the change.
3. engine/baml-runtime/src/eval_expr.rs:11
- Draft comment:
Added import for std::str::FromStr, which is necessary for header conversions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining why an import was added. It doesn't provide a suggestion or ask for confirmation on a specific code change.
4. engine/baml-runtime/src/eval_expr.rs:276
- Draft comment:
Passing env.env_vars instead of an empty HashMap allows proper propagation of environment variables to call_function. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining the reason behind a code change. It doesn't suggest any action or ask for confirmation, which violates the rules.
5. engine/baml-runtime/src/eval_expr.rs:320
- Draft comment:
Replaced unused variable 'cls' with '_' in pattern matching for BamlValue::Class. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it simply describes a change that was made without providing any suggestion, question, or request for confirmation. It doesn't align with the rules for useful comments.
6. engine/baml-runtime/src/eval_expr.rs:347
- Draft comment:
Improved extraction and error handling for 'headers' and 'query_params' fields from the request object. Ensure that these fields match the expected map structure. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure that certain fields match an expected structure. It doesn't provide a specific suggestion or point out a specific issue, which violates the rule against asking the author to ensure or verify things.
7. engine/baml-runtime/src/eval_expr.rs:379
- Draft comment:
Simplified proxy handling by directly inserting the 'baml-original-url' header into header_map and updating base_url accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, describing what the code change does without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
8. engine/baml-runtime/src/eval_expr.rs:394
- Draft comment:
The HTTP request now correctly attaches query parameters and custom headers. Consider removing or enabling the debugging eprintln if not needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_OrjaaYdvV1LBi7K2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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 a2b4dd2 in 1 minute and 19 seconds. Click for details.
- Reviewed
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
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.
Workflow ID: wflow_UkZ2fKODJ7e0vSYk
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
engine/baml-runtime/src/eval_expr.rs
Outdated
// .run_test("MakePerson", "TestMakePerson", &ctx, Some(on_event), None) | ||
// .run_test("CompareHaikus", "Test", &ctx, Some(on_event)) | ||
// .run_test("LlmParseInt", "TestParse", &ctx, Some(on_event)) | ||
.await; | ||
dbg!(res); | ||
assert!(false); | ||
} | ||
|
||
// #[tokio::test] |
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 commenting out the #[tokio::test]
attribute. If the test is intentionally disabled, consider using #[ignore]
with an explanation instead of a manual comment.
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 62193e4 in 2 minutes and 5 seconds. Click for details.
- Reviewed
114
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
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-core/src/ir/builtin.rs:55
- Draft comment:
Added a 'method' field to the Request class but no documentation on allowed enum values or its intended use. Consider clarifying in comments or docs how this field should be used. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This appears to be a standard HTTP method field in a Request class, which is a very common pattern. The lack of documentation is consistent with the rest of the codebase style where other fields also lack docs. Asking for docs just for this field seems inconsistent. Additionally, HTTP methods are well understood in the industry. The comment could be valid if this is a public API that needs comprehensive documentation. Also, having only "Get" defined in the enum is unusual - maybe documentation would help explain why. While documentation could be helpful, singling out this field when others lack docs seems arbitrary. If documentation is needed, it should be requested for the whole class/module, not just this field. The comment should be deleted as it singles out one field for documentation when this appears to be a consistent style choice across the codebase.
2. engine/baml-lib/baml/tests/validation_files/expr/builtin.baml:8
- Draft comment:
The function header changed from 'fn' to 'function'. Confirm that this grammar change is intentional and consistently applied. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_K3Fos8OsD2eCviSV
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
engine/baml-runtime/src/eval_expr.rs
Outdated
|
||
fn GetTodo() -> Todo { | ||
std::fetch_value<Todo>(std::Request { | ||
method: Get, |
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 test for fetch_value
now passes a 'method: Get'
field in the Request
instance, but the built-in fetch_value
implementation still unconditionally uses GET
(via client.get
). Consider using the 'method'
field to select the appropriate HTTP verb.
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 58c5ee0 in 1 minute and 13 seconds. Click for details.
- Reviewed
57
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
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-runtime/src/eval_expr.rs:1157
- Draft comment:
Ensure that removing the 'method' field from std::Request is deliberate (default GET assumed). Update documentation if needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%
<= threshold50%
The comment is asking the author to ensure that the removal of themethod
field is deliberate, which is against the rules. However, it also provides a specific suggestion about the default behavior (GET assumed) and mentions updating documentation, which could be useful. The comment is borderline, but it leans towards being informative rather than actionable.
2. typescript/fiddle-proxy/server.js:56
- Draft comment:
The new followRedirects option is added. Verify that any redirection does not inadvertently leak API keys to unauthorized domains. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify something, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
Workflow ID: wflow_pGiahgUn8oQfXPL3
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
name: String::from(classes::REQUEST), | ||
docstring: None, | ||
static_fields: vec![ | ||
// Node { |
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 leaving large blocks of commented-out code. If the 'method' field is no longer needed, consider removing it completely.
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.
Important
Looks good to me! 👍
Reviewed a5567cf in 43 seconds. Click for details.
- Reviewed
32
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
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-runtime/src/eval_expr.rs:317
- Draft comment:
Consider moving type validation into a dedicated check. The TODO indicates planned type checks; clarifying expected types here can improve error messages. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. engine/baml-runtime/src/eval_expr.rs:413
- Draft comment:
HTTP error handling: Ensure that errors (e.g., redirection codes) are treated as intended. Also consider cleaning up the channel on errors to avoid leftover state. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_OdJcLQdMPeaiz8nD
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 00894b0 in 1 minute and 35 seconds. Click for details.
- Reviewed
66
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
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-core/src/validate/validation_pipeline/validations/expr_fns.rs:116
- Draft comment:
Consider usingis_empty()
instead of comparinglen() == 0
forapp.type_args
, and cache the result ofapp.name.name()
to avoid redundant calls. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The is_empty() suggestion is a valid Rust idiom that would make the code slightly more idiomatic. However, the caching suggestion seems premature - name() is likely just a getter and the extra variable would add complexity. The error message needs to show the function name twice for clarity. The comment combines two separate suggestions, which makes it less focused. Also, without profiling data, we don't know if caching name() would have any meaningful performance impact. The is_empty() suggestion alone would be worth keeping as it's a clear improvement following Rust conventions, but mixing it with the questionable caching suggestion weakens the comment overall. While the is_empty() suggestion has merit, the combined nature of the comment and the dubious caching suggestion make it better to skip this comment.
2. engine/baml-lib/baml/tests/validation_files/expr/builtin.baml:16
- Draft comment:
The test for missing generic type arguments is clear. Consider also adding a positive test ensuring that a valid type argument does not raise an error. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_ujFTSLukbUj5HheT
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Adds built-in
fetch_value
function andRequest
class, updates IR and type system for generics, and enhances proxy server for API key injection.fetch_value
function andRequest
class inbuiltin.rs
.HTTP_METHOD
enum with a single valueGet
.IntermediateRepr
inrepr.rs
to include built-in classes and functions.Expr
inexpr.rs
to support built-in functions and generic types.expr_typecheck.rs
to handle new built-ins and generics.server.js
to follow redirects and inject API keys for specific domains.expr_fns.rs
to check for generic function usage.datamodel.pest
andparse_expr.rs
for generic function applications.This description was created by
for 00894b0. You can customize this summary. It will automatically update as commits are pushed.