fix(generative): send AWS stop_sequences through to the gRPC request - #2131
fix(generative): send AWS stop_sequences through to the gRPC request#2131Anai-Guo wants to merge 2 commits into
Conversation
`GenerativeConfig.aws_bedrock`/`aws_sagemaker` already accept a `stop_sequences` argument and store it on the runtime config, but `_GenerativeAWS._to_grpc` never forwarded it, so the value was silently dropped and never reached the server. The server-side `GenerativeAWS` proto has carried `stop_sequences` across every vendored version (v4216/v5261/v6300), so the field can be wired unconditionally the same way `max_tokens` already is. Also expose `stop_sequences` on the (deprecated) `aws()` factory for parity, and narrow the stale TODO to the two fields the proto still lacks (`top_k`, `top_p`).
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
|
To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge. |
| target_model: Optional[str] = None, | ||
| target_variant: Optional[str] = None, | ||
| temperature: Optional[float] = None, | ||
| stop_sequences: Optional[List[str]] = None, |
There was a problem hiding this comment.
Can you remove this field here? We do not want to update deprecated factories
shashvat-singham
left a comment
There was a problem hiding this comment.
Checked this against the vendored proto and the sibling providers rather than just the diff — it holds up on both counts.
The field really is there, so removing it from the TODO is correct:
>>> [f.name for f in generative_pb2.GenerativeAWS.DESCRIPTOR.fields]
['model', 'temperature', 'service', 'region', 'endpoint', 'target_model',
'target_variant', 'images', 'image_properties', 'max_tokens', 'stop_sequences']and the trimmed comment is still accurate for what's left — top_k and top_p are genuinely absent from GenerativeAWS, so "add top_k & top_p here when added to server-side proto" is the right residual.
The part that convinced me this is a real bug rather than an intentional omission: AWS was the only provider passing a hardcoded stop_sequences=None. Seven other generative factories already take stop_sequences: Optional[List[str]] = None and forward it, and _GenerativeAWS already declared stop_sequences: Optional[List[str]] on the dataclass — so the plumbing existed on both ends and only the factory argument and the _to_grpc line were missing. That's a straightforward oversight, and this closes it consistently with how the others are written.
test_generative_parameters_images_parsing exercises the _to_grpc path with stop_sequences=["\n"] and asserts the expected base_pb2.TextArray(values=["\n"]), which is the right level to test it at.
Looks good to me.
Per review: revert the stop_sequences addition to the deprecated aws() factory and move test coverage to aws_sagemaker. The core fix (sending stop_sequences through _to_grpc for GenerativeAWS) is unchanged, so aws_bedrock/aws_sagemaker still carry the field to the proto.
|
Done — reverted the |
What
GenerativeConfig.aws_bedrock(...)andaws_sagemaker(...)(the current, non-deprecated AWS generative factories) already accept astop_sequencesargument and store it on the runtime config — but_GenerativeAWS._to_grpcnever forwarded it to the gRPC request, so the value was silently dropped and never reached the server.Why it's safe to wire unconditionally
The server-side
GenerativeAWSmessage carriesstop_sequencesin every vendored proto version shipped in this repo (v4216,v5261,v6300), so it can be sent the same waymax_tokensalready is — no version guard needed. The# TODO - add top_k, top_p & stop_sequences ...comment was stale forstop_sequences; I've narrowed it to the two fields the proto still lacks (top_k,top_p).Changes
_GenerativeAWS._to_grpc: forwardstop_sequences=_to_text_array(self.stop_sequences).top_k&top_p.stop_sequenceson the deprecatedaws()factory for parity (it hard-codedstop_sequences=None)._to_grpctest to assertstop_sequencesround-trips.test/collection/test_classes_generative.pypasses (20 passed);ruff format --checkandruff checkare clean.🤖 Generated with Claude Code