Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Removes the litellm dependency and updates the Bedrock chat model implementation to call AWS Bedrock directly via boto3 (Converse / ConverseStream), with corresponding test updates.
Changes:
- Dropped
litellmfrom optional/test dependency groups and the dependency manager. - Reworked
bedrockChatModel to useboto3Bedrock Runtimeconverse_streamwith fallback toconverse. - Updated Bedrock unit tests and example metadata to reflect the removal.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
marimo/_ai/llm/_impl.py |
Replace LiteLLM-based Bedrock calls with direct boto3 Converse/ConverseStream logic. |
tests/_ai/llm/test_impl.py |
Update Bedrock fixtures/tests to mock boto3 client and validate new request/response handling. |
marimo/_dependencies/dependencies.py |
Remove DependencyManager.litellm. |
pyproject.toml |
Remove litellm from test-optional and typecheck extras. |
examples/ai/chat/bedrock_example.py |
Remove litellm from example dependency header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
There was a problem hiding this comment.
TestBedrock currently validates the streaming path only. Since the implementation now falls back from converse_stream to converse on streaming errors, add a test that forces converse_stream to raise a streaming-related exception and asserts the non-streaming fallback behavior (including response parsing). This will prevent regressions in the error-handling path introduced here.
| def test_call_streaming_error_fallback_to_converse( | |
| self, mock_bedrock_client, test_messages, test_config | |
| ): | |
| """Test that Bedrock falls back to non-streaming Converse on streaming errors. | |
| This forces converse_stream to raise a streaming-related exception and | |
| asserts that the implementation calls converse instead and correctly | |
| parses the non-streaming response. | |
| """ | |
| model_name = "anthropic.claude-3-sonnet-20240229" | |
| # Simulate a streaming-related error from converse_stream | |
| mock_bedrock_client.converse_stream.side_effect = RuntimeError( | |
| "streaming error" | |
| ) | |
| # Set up a minimal non-streaming response structure for converse | |
| mock_bedrock_client.converse.return_value = { | |
| "output": { | |
| "message": { | |
| "content": [ | |
| {"text": "Fallback response"}, | |
| ] | |
| } | |
| } | |
| } | |
| model = bedrock(model_name) | |
| result = list(model(test_messages, test_config)) | |
| # The fallback path should return the parsed text from the converse response | |
| assert result == ["Fallback response"] | |
| # Ensure we attempted streaming first, then fell back to non-streaming | |
| mock_bedrock_client.converse_stream.assert_called_once() | |
| mock_bedrock_client.converse.assert_called_once() |
| ) -> dict[str, Any]: | ||
| converse_messages: list[dict[str, Any]] = [] | ||
| for msg in messages: | ||
| content = str(msg.content) if msg.content else "" |
There was a problem hiding this comment.
In _build_converse_params(), message content is derived from str(msg.content) if msg.content else "", which (a) treats falsy-but-valid content (e.g. 0/False) as empty, and (b) stringifies potentially rich Python objects even when ChatMessage.parts contains the actual text to send. This can leak reprs into the prompt and diverges from the other providers’ parts-based conversions. Prefer building Bedrock message text from msg.parts when available, and only fall back to msg.content when it is a string / when parts are empty; also check msg.content is None rather than truthiness.
| content = str(msg.content) if msg.content else "" | |
| # Prefer building content from parts when available, to match other providers | |
| content = "" | |
| parts = getattr(msg, "parts", None) | |
| if parts: | |
| text_fragments: list[str] = [] | |
| for part in parts: | |
| if isinstance(part, str): | |
| text_fragments.append(part) | |
| elif hasattr(part, "text"): | |
| part_text = getattr(part, "text") | |
| if part_text is not None: | |
| text_fragments.append(str(part_text)) | |
| content = "".join(text_fragments) | |
| if content == "": | |
| # Fall back to msg.content when parts are empty or unavailable | |
| if isinstance(msg.content, str): | |
| content = msg.content | |
| elif msg.content is None: | |
| content = "" | |
| else: | |
| content = str(msg.content) |
| if content and "text" in content[0]: | ||
| return content[0]["text"] |
There was a problem hiding this comment.
Non-streaming fallback parsing for client.converse() only returns content[0]["text"] when present. Bedrock Converse responses can contain multiple content blocks; returning only the first block can truncate the assistant response. Consider concatenating all text blocks (and safely ignoring non-text blocks) to produce the complete string response.
| if content and "text" in content[0]: | |
| return content[0]["text"] | |
| if isinstance(content, list): | |
| text_parts = [] | |
| for block in content: | |
| if isinstance(block, dict) and "text" in block: | |
| text_value = block["text"] | |
| if isinstance(text_value, str): | |
| text_parts.append(text_value) | |
| if text_parts: | |
| return "".join(text_parts) |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.21.2-dev48 |
📝 Summary
Remove litllm