Skip to content

bedrock collector#1703

Merged
aaronvg merged 4 commits into
canaryfrom
bedrock-collector
Apr 1, 2025
Merged

bedrock collector#1703
aaronvg merged 4 commits into
canaryfrom
bedrock-collector

Conversation

@aaronvg
Copy link
Copy Markdown
Contributor

@aaronvg aaronvg commented Apr 1, 2025

  • Add bedrock http request and response
  • update tests

Important

Introduces Bedrock HTTP request/response handling with CollectorInterceptor for tracing and updates AWS client tests for enhanced error handling and dynamic configuration.

  • Bedrock HTTP Request and Response:
    • Added CollectorInterceptor in aws_client.rs to log HTTP requests and responses to BAML_TRACER.
    • Implemented smithy_json_headers() to convert headers to JSON format.
    • Updated client_anyhow() to use CollectorInterceptor for tracing.
  • Testing:
    • Added TestOpenAIDummyClient in dummy-clients.baml and corresponding test cases in test_client_response.py.
    • Enhanced AWS tests in aws.test.ts to cover various scenarios including invalid credentials and dynamic client configuration.
  • Dependencies:
    • Added futures and reqwest to Cargo.lock and Cargo.toml for async operations and HTTP requests.

This description was created by Ellipsis for e9b304f. It will automatically update as commits are pushed.

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 1, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
baml ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 1, 2025 7:42pm

Copy link
Copy Markdown
Contributor

@ellipsis-dev ellipsis-dev Bot left a 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 e9b304f in 2 minutes and 53 seconds

More details
  • Looked at 2022 lines of code in 36 files
  • Skipped 0 files when reviewing.
  • Skipped posting 21 drafted comments based on config settings.
1. integ-tests/typescript/tests/providers/aws.test.ts:380
  • Draft comment:
    Avoid modifying global env variables (e.g. AWS_REGION) without resetting them after the test. Consider using setup/teardown to isolate changes.
  • 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. integ-tests/typescript/tests/providers/aws.test.ts:11
  • Draft comment:
    Consider directly testing promise rejection rather than wrapping an async function in expect. This can make the test more idiomatic.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
3. integ-tests/typescript/tests/providers/aws.test.ts:380
  • Draft comment:
    Consider resetting modified environment variables after tests. The test sets AWS_REGION and AWS_DEFAULT_REGION but doesn’t restore them, which may affect subsequent tests.
  • 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. integ-tests/typescript/tests/providers/aws.test.ts:11
  • Draft comment:
    Standardize error assertions across tests. Some tests expect a simple code ('GenericFailure') while others match a detailed error object (e.g. 'BamlClientHttpError' with status 403). Consistency can reduce ambiguity.
  • 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.
5. engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs:304
  • Draft comment:
    Typographical error: In the comment that checks the AWS secret access key, the word 'remainer' is used instead of 'remainder'. Please correct the typo.
  • 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/language_client_typescript/package.json:70
  • Draft comment:
    The '--module nodenext' flag is repeated in the 'build:ts_build' script (and similarly in 'build:ts_build_local'). Consider removing the duplicate to avoid any potential confusion.
  • Reason this comment was not posted:
    Comment was on unchanged code.
7. integ-tests/baml_src/test-files/providers/dummy-clients.baml:16
  • Draft comment:
    There is a missing trailing newline at the end of the file. Please add a newline at the end of the file.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is purely informative, suggesting a stylistic change that doesn't impact functionality. It doesn't provide a specific code suggestion or highlight a potential issue.
8. integ-tests/openapi/baml_client/openapi.yaml:2409
  • Draft comment:
    Typo in $ref value: several occurrences use '#components/schemas/Check' instead of the correct '#/components/schemas/Check'. Please insert the missing slash after '#' to conform to the OpenAPI specification.
  • 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. integ-tests/python/baml_client/async_request.py:597
  • Draft comment:
    In the parameter list of the function 'DescribeMedia1599', the parameters appear as "img: baml_py.Image,client_sector: str,client_name: str" without a space after the comma. Consider changing it to "img: baml_py.Image, client_sector: str, client_name: str" for better 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. integ-tests/python/baml_client/async_request.py:525
  • Draft comment:
    In the parameter list of the function 'DescribeImage2', the parameters are written as "classWithImage: types.ClassWithImage,img2: baml_py.Image" without a space after the comma. Please insert a space so it becomes "classWithImage: types.ClassWithImage, img2: baml_py.Image".
  • 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. integ-tests/python/baml_client/async_request.py:1470
  • Draft comment:
    In the function 'InOutEnumMapKey', the parameters are declared as "i1: Dict[types.MapKey, str],i2: Dict[types.MapKey, str]". A space after the comma would improve readability (i.e., "i1: Dict[types.MapKey, str], i2: Dict[types.MapKey, str]").
  • 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. integ-tests/python/baml_client/async_request.py:1500
  • Draft comment:
    In the function 'InOutLiteralStringUnionMapKey', the parameter list joins the two parameters without a space (e.g., "...],i2: Dict[...]" instead of ", i2: Dict[...]"). Please add a space after the comma to separate the parameters clearly.
  • 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.
13. integ-tests/python/baml_client/async_request.py:3314
  • Draft comment:
    The function 'TestMulticlassNamedArgs' declares its parameters as "myArg: types.NamedArgsSingleClass,myArg2: types.NamedArgsSingleClass" without a space after the comma. Adding a space (i.e., "myArg: types.NamedArgsSingleClass, myArg2: types.NamedArgsSingleClass") would improve clarity.
  • 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.
14. integ-tests/python/http-server.py:37
  • Draft comment:
    There's an extra leading space in the response content string (' Yes'). Consider removing the space to maintain 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 leading space appears to be intentional part of the response format, not a mistake. The space is consistently represented in multiple places: the content string, the token field, and the bytes array. This suggests it's part of the expected format. The comment assumes this is an error without strong evidence.
    Maybe the space really is unintentional and could cause issues in the application consuming this response. Maybe there's a style guide that prohibits leading spaces.
    Without additional context about requirements or seeing the consuming code, we can't assume the space is wrong. The careful inclusion of the space in tokens and bytes suggests it's deliberate.
    The comment should be deleted as it assumes an error without strong evidence, and the space appears to be an intentional part of the response format.
15. integ-tests/python/tests/test_collector.py:440
  • Draft comment:
    In the comment on line 440, consider changing "baml doesnt yet throw if theres a connection error during the stream.." to "baml doesn't yet throw if there's a connection error during the stream..." for better clarity and correct punctuation.
  • 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. integ-tests/react/baml_client/inlinedbaml.ts:10
  • Draft comment:
    Typo in the comment: consider changing 'Haiku require 2048 tokens to cache' to 'Haiku requires 2048 tokens to cache' for correct grammar.
  • 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. integ-tests/react/baml_client/sync_client.ts:3538
  • Draft comment:
    It appears that the function name 'TestOpenAIGPT4oMini' might contain a typographical error. Consider verifying whether the intended name is 'TestOpenAIGPT4Mini' (without the extra 'o').
  • Reason this comment was not posted:
    Comment was on unchanged code.
18. integ-tests/react/baml_client/sync_request.ts:1614
  • Draft comment:
    In the method 'PrimitiveAlias', the union type is defined as 'number | string | boolean | number'. The type 'number' appears twice, which is redundant. Consider removing the duplicate to simplify the type to 'number | string | boolean'.
  • 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. integ-tests/typescript/baml_client/inlinedbaml.ts:38
  • Draft comment:
    There's an inconsistency in spacing: in the TestGeminiSystemAsChat function's prompt, the user role is invoked as {{_.role("user")}} without a space after the opening {{. For consistency with other parts of the file (e.g., {{ _.role('system') }}), please add a space so it reads {{ _.role("user") }}.
  • 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.
20. integ-tests/typescript/baml_client/sync_client.ts:67
  • Draft comment:
    Typo in the deprecated comment for the 'stream' getter (lines 63-66): the phrase 'streaming must by async' should be corrected to 'streaming must be async'.
  • 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. integ-tests/typescript/baml_client/sync_client.ts:68
  • Draft comment:
    In the error message for the 'stream' getter (line 68), the string literal appears to be missing a closing backtick after the module name (i.e., it should end with a backtick after 'baml_client/async_client'). Please add the missing backtick to correctly format the message.
  • 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_PqHIvd4GaD5pW7uF


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@aaronvg aaronvg enabled auto-merge April 1, 2025 19:37
@aaronvg aaronvg added this pull request to the merge queue Apr 1, 2025
Merged via the queue into canary with commit e7c45c2 Apr 1, 2025
@aaronvg aaronvg deleted the bedrock-collector branch April 1, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant