Skip to content

feat(py/plugins/amazon-bedrock): add Converse non-streaming generate path - #5876

Draft
hilariie wants to merge 7 commits into
mainfrom
py-bedrock-converse-sync
Draft

feat(py/plugins/amazon-bedrock): add Converse non-streaming generate path#5876
hilariie wants to merge 7 commits into
mainfrom
py-bedrock-converse-sync

Conversation

@hilariie

Copy link
Copy Markdown
Contributor

Slice 3 of #5820. Non-streaming Converse generate path for the Amazon Bedrock Python plugin, ported from genkit-ai/aws-bedrock-go-plugin (generate.go, types.go). Stacked on #5699.

What lands

  • transport.py: every boto3 call goes through here. One shared client built lazily under a lock, each call pushed onto a worker thread with asyncio.to_thread so the loop stays free for the seconds-to-minutes a generate takes. Keeping boto3 behind one module means we can swap in AWS's async SDK later without touching converters or models.
  • converters.py: pure request/response conversion. Roles, system extraction, media, tools and tool choice, cache points, stop reasons, token usage, tool-input coercion, reasoning parts.
  • models.py: BedrockModel.generate plus AWS error to GenkitError status mapping.
  • plugin.py: resolve() and list_actions() wired to real model actions.
  • samples/amazon-bedrock-sample: Dev UI sample with text, structured output, tool calling, and reasoning flows, over Nova Lite, Llama 3.3, and DeepSeek R1.

Region resolution now fails loudly instead of defaulting to us-east-1, same as the Go plugin. A silent default sends traffic, and data, to a region nobody picked.

Reasoning

Still working on Genkit ReasoningParts.

Divergences from Go

  1. Timeouts are botocore read_timeout/connect_timeout/max_pool_connections (3600/60/50) rather than Go's whole-call deadline, which kills long generations mid-flight.
  2. resolve() is lazy and serves any model ID, inference profile or ARN. init() returns [], the Bedrock catalogue can't be enumerated.
  3. AWS error codes map to GenkitError statuses. Go has no mapping.
  4. request.tool_choice is a fallback; the Bedrock config wins because it can name a specific tool the core enum can't express.
  5. Explicit temperature=0.0 is sent, not dropped as unset.
  6. AWS_REGION is read directly. botocore only started honoring it in 1.41, above our floor, so leaving it to the SDK chain would silently ignore the var our own error message tells you to set.
  7. Go injects a default maxTokens for Claude models. We don't: the requirement and the per-family values couldn't be verified here, and guessing one silently caps output. Callers set maxTokens themselves.
  8. Streaming requests work but arrive as one chunk until ConverseStream lands next slice.
  9. boto3 floor is >=1.37.24, first release with cache points and reasoningContent.

Structured output needs output_instructions=True passed alongside output_format and output_schema. supports.constrained is NONE because Converse has no constrained-decoding mode, and core's json format injects no instructions on its own, so without the flag the schema never reaches the model. The sample and its README both show the working call.

Lockfile

uv.lock refreshed with uv 0.12.0, what CI's unpinned setup-uv installs today. That drops the exclude-newer/exclude-newer-span options block this branch inherited (no released uv accepts exclude-newer-span, main's lock doesn't carry the block, and uv lock --check fails while it's there) and records the plugin's real version. Rest of the churn is 0.12.0 marker normalization, no dependency versions changed.

Tests

159 unit tests, no AWS, plus 3 opt-in live tests (BEDROCK_LIVE_TESTS=1).

Ran Nova Lite for text and the undocumented-tool round trip, DeepSeek R1 for reasoning, Llama 3.3 for structured output.

tests/request_validation_test.py runs botocore's own ParamValidator over built requests against the bedrock-runtime service model. Several Converse fields are NonEmptyString (min: 1) and botocore enforces that client-side, so an empty tool description, system text or toolUseId fails before it reaches AWS, as ParamValidationError (a BotoCoreError, not a ClientError). Go never hits this, its SDK does no client-side length check. The corpus covers every request-shaping branch the builder has: undocumented tool, blank system prompt, empty assistant text, tool round trip, media and cache points, dropped reasoning, and inference config on two model families.

Dev UI

Screenshot 2026-07-31 at 14 08 19 Screenshot 2026-07-31 at 14 09 07 Screenshot 2026-07-31 at 14 10 36

Declared models resolve, the plugin's customOptions schema (Max tokens, Tool choice) renders in the model runner, and the sample registers four flows and one tool.

Notes for the stack

  • First commit is the slice, second fixes two defects inherited from the slices below that are red on their CI too: package version pinned to 0.1.0 where check_consistency.py wants the core version, and model_info_test.py reaching through the optional Supports field (17 ty + 17 pyright errors). Happy to move that commit down the stack instead.
  • feat(py/plugins/amazon-bedrock): add model capability registry #5699's committed lock has the same exclude-newer block and stale 0.1.0 version, so its uv lock --check fails under current uv. A plain uv lock there fixes it the same way.

cabljac and others added 7 commits July 30, 2026 10:20
Package skeleton for the Bedrock port from the Go plugin
(genkit-ai/aws-bedrock-go-plugin): plugin class with constructor and
per-call config types, workspace wiring, and scaffold tests. Action
resolution, Converse generate/stream, embedders, image generation and
reranker land in follow-up slices.
boto3 + thin async bridge instead of aioboto3: aiobotocore pins botocore
to a narrow patch range, which would dictate boto3 versions for every
user of the plugin. A thread bridge keeps the event loop unblocked at
LLM concurrency levels, matching Strands, pydantic-ai and langchain-aws.
Transport goes behind an internal seam so the smithy-based official
async SDK can slot in once it matures.
Port of the Go plugin's models.go: 47-entry capability registry
(verified key-for-key and flag-for-flag against the source), cross-region
inference-profile prefix stripping for capability lookup only, and
get_model_info() with modern Converse defaults at the unstable stage for
unknown chat/text models so newer or profile-only models stay callable.
…k ARNs

Reduce foundation-model and inference-profile ARNs to their resource ID
before capability lookup, across all AWS partitions (aws, aws-us-gov,
aws-cn). Lookup-only, requests still carry the original identifier.
Addresses PR review feedback.
…path

Transport seam bridging sync boto3 onto worker threads, pure Converse
converters ported from the Go plugin's generate.go, lazy model
resolution, and AWS error mapping. Region resolution now fails loudly
instead of defaulting to us-east-1, matching Go. boto3 floor moves to
1.37.24 for cache points and reasoningContent.
…tionals

Both land from earlier slices: the scaffold pins 0.1.0 where
check_consistency.py wants the core version, and model_info_test.py trips
ty and pyright on the optional Supports field.
lock refreshed with uv 0.12.0; drops exclude-newer options released uv cannot reproduce
@hilariie
hilariie requested a review from cabljac July 31, 2026 15:41
@github-actions github-actions Bot added docs Improvements or additions to documentation python Python config labels Jul 31, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the genkit-amazon-bedrock plugin, enabling support for Amazon Bedrock models via the Converse API. The implementation includes the plugin structure, configuration, type converters, model capability registry, and comprehensive tests. Feedback highlights a potential namespace hijacking issue in the resolve method and a runtime safety concern in _reasoning_block_to_part when handling unexpected API response types.

Comment on lines +146 to +148
if action_type != ActionKind.MODEL:
return None
model_id = name.removeprefix(f'{BEDROCK_PLUGIN_NAME}/')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The resolve method does not verify if the requested action name starts with the bedrock/ namespace prefix before attempting to resolve it. If another plugin's model is requested (e.g., openai/gpt-4), this method will strip the prefix (which does nothing since it doesn't start with bedrock/), identify it as a 'chat' model, and return a BedrockModel action for it. This will cause the Bedrock plugin to greedily hijack and attempt to resolve actions belonging to other namespaces.\n\nTo fix this, explicitly check if the action name starts with the bedrock/ namespace prefix.

        if action_type != ActionKind.MODEL:\n            return None\n        if not name.startswith(f'{BEDROCK_PLUGIN_NAME}/'):\n            return None\n        model_id = name.removeprefix(f'{BEDROCK_PLUGIN_NAME}/')

Comment on lines +641 to +645
if isinstance(reasoning_text, str):
text, signature = reasoning_text, None
else:
text = reasoning_text.get('text') or ''
signature = reasoning_text.get('signature')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In _reasoning_block_to_part, if reasoning_text is not a string, the code assumes it is a dictionary and calls .get('text') and .get('signature') on it. If the API returns an unexpected type (such as a list or an integer) for reasoningText, this will raise an AttributeError. It is safer to explicitly check if reasoning_text is a dictionary before calling .get() to prevent potential runtime crashes.

        if isinstance(reasoning_text, str):\n            text, signature = reasoning_text, None\n        elif isinstance(reasoning_text, dict):\n            text = reasoning_text.get('text') or ''\n            signature = reasoning_text.get('signature')\n        else:\n            return None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

config docs Improvements or additions to documentation python Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants