feat: Add more foundational models to bedrock testcases#187
Conversation
There was a problem hiding this comment.
Hello @dhruvj07, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request adds more test cases to the bedrock_client_universal.py example, specifically targeting the amazon.titan-text-lite-v1, amazon.titan-text-premier-v1, and cohere.command-light-text-v14 models. The changes include adding tests for both invoke and invoke-with-response-stream modes for the Titan models, as well as converse and converse-stream modes. Additionally, the PR cleans up some docstrings and removes unnecessary comments in the init_bedrock function.
Highlights
- Expanded Test Coverage: The pull request introduces new test cases for
amazon.titan-text-lite-v1,amazon.titan-text-premier-v1, andcohere.command-light-text-v14models, covering various modes likeinvoke,invoke-with-response-stream,converse, andconverse-stream. - Model Identifier Handling: The code now includes a check for invalid model identifiers, specifically for
amazon.titan-text-premier-v1, and skips the test if the model identifier is invalid, printing a message indicating the skip. - Code Cleanup: Docstrings and unnecessary comments have been removed from the
init_bedrockfunction to improve readability.
Changelog
- examples/bedrock/bedrock_client_universal.py
- Removed unnecessary comments and docstrings from
init_bedrockfunction (lines 18-31) - Removed docstrings from bedrock examples (lines 40-43, 61-64, 86-89, 113-116)
- Added tests for
amazon.titan-text-lite-v1usinginvoke_model_with_response_stream(lines 323-334) - Added tests for
amazon.titan-text-premier-v1usinginvoke_modelandinvoke_model_with_response_stream(lines 337-363) - Added tests for
amazon.titan-text-premier-v1usingconverseandconverse_stream(lines 366-386) - Added tests for
cohere.command-light-text-v14usinginvoke,converse, andconverse_stream(lines 389-414) - Added conditional skipping of tests based on model identifier validity (lines 360-361, 383-384)
- Removed unnecessary comments and docstrings from
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A test for each model,
Ensuring all is in order,
Bugs flee in fright.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request adds more test cases to the bedrock_client_universal.py file, focusing on models that are sometimes skipped due to availability or other issues. The changes include testing various Amazon Titan models and Cohere models across different modes (invoke, converse, stream). The removal of docstrings from the example functions is a bit concerning, as it reduces the clarity of the code. However, the addition of more test cases is a positive step towards ensuring the reliability of the Bedrock client.
Summary of Findings
- Missing Docstrings: The docstrings for the example functions (bedrock_invoke_example, bedrock_converse_example, bedrock_invoke_stream_example, bedrock_converse_stream_example) have been removed. This reduces the readability and maintainability of the code. Docstrings are essential for understanding the purpose and usage of functions.
- Error Handling: The error handling for the amazon.titan-text-premier-v1 model checks if the 'provided model identifier is invalid' is in the error message. This is a good approach to skip tests for unavailable models, but it relies on string matching, which might not be robust.
Merge Readiness
The addition of more test cases is a positive step, but the removal of docstrings is a concern. I recommend adding the docstrings back to the example functions to improve code clarity. The error handling for skipped tests could be improved to be more robust. I am unable to approve this pull request, and users should have others review and approve this code before merging. Due to the medium severity issue of missing docstrings, I recommend that the pull request not be merged until this is addressed.
| @@ -37,65 +33,45 @@ def init_bedrock(): | |||
|
|
|||
|
|
|||
| def bedrock_invoke_example(bedrock_runtime_client): | |||
There was a problem hiding this comment.
The docstring for this function has been removed. It's important to keep docstrings to explain the purpose and usage of the function. Please add it back.
def bedrock_invoke_example(bedrock_runtime_client):
"""
Demonstrates a basic 'invoke' style call (non-streaming).
Returns a JSON-formatted string of the response.
"""| return json.dumps(response_body, indent=2) | ||
|
|
||
|
|
||
| def bedrock_converse_example(bedrock_runtime_client): |
There was a problem hiding this comment.
The docstring for this function has been removed. It's important to keep docstrings to explain the purpose and usage of the function. Please add it back.
def bedrock_converse_example(bedrock_runtime_client):
"""
Demonstrates a 'converse' style call by including 'system' text plus a user message.
Still uses 'invoke_model', but the request body includes additional fields.
"""| return json.dumps(response_body, indent=2) | ||
|
|
||
|
|
||
| def bedrock_invoke_stream_example(bedrock_runtime_client): |
There was a problem hiding this comment.
The docstring for this function has been removed. It's important to keep docstrings to explain the purpose and usage of the function. Please add it back.
def bedrock_invoke_stream_example(bedrock_runtime_client):
"""
Demonstrates a streaming 'invoke' call by processing the response tokens as they arrive.
Iterates over the streaming response lines and prints them in real-time.
"""| return "".join(tokens) | ||
|
|
||
|
|
||
| def bedrock_converse_stream_example(bedrock_runtime_client): |
There was a problem hiding this comment.
The docstring for this function has been removed. It's important to keep docstrings to explain the purpose and usage of the function. Please add it back.
def bedrock_converse_stream_example(bedrock_runtime_client):
"""
Demonstrates a streaming 'converse' call by processing the response tokens as they arrive.
Iterates over the streaming response lines for a conversation style input.
"""| if "provided model identifier is invalid" in str(e): | ||
| print("✅ Skipped amazon.titan-text-premier-v1 test (model identifier invalid)") |
There was a problem hiding this comment.
The check for skipping the amazon.titan-text-premier-v1 test relies on string matching the error message. This might not be robust, as the error message could change. Consider using a more specific exception type or a more reliable way to determine if the model is unavailable.
except Exception as e:
if isinstance(e, some_specific_exception):
print("✅ Skipped amazon.titan-text-premier-v1 test (model identifier invalid)")
elif "provided model identifier is invalid" in str(e):
print("✅ Skipped amazon.titan-text-premier-v1 test (model identifier invalid)")c6d0998 to
8b56f96
Compare
No description provided.