First, thank you for Pathway and the recent native AWS Bedrock integration (#170) — the BedrockChat Converse wrapper is genuinely useful. I ran into a small inconsistency around the top_k inference parameter and wanted to report it before proposing any change, per CONTRIBUTING.md.
Summary
BedrockChat._accepts_call_arg advertises top_k as a supported inference argument, but BedrockChat.__wrapped__ never forwards it to the Converse call, so it is silently dropped.
Where
python/pathway/xpacks/llm/llms.py: _accepts_call_arg lists top_k in supported_args.
- In
__wrapped__, the Converse inferenceConfig is built only from max_tokens / temperature / top_p / stop_sequences. top_k is never read, so it does not reach client.converse(...).
- The docstring
Args: and the BEDROCK_VALID_ARGS list in tests/test_llms.py both omit top_k, so the advertised support is currently a dead entry.
Why this is the expected fix
Per the AWS Converse API reference, top_k is not part of the base InferenceConfiguration (only maxTokens, stopSequences, temperature, topP). Model-specific parameters such as top_k (e.g. Anthropic Claude) must be passed via additionalModelRequestFields. The AWS docs even show this exact case (Example 3): "additionalModelRequestFields": { "top_k": 200 } against anthropic.claude-3-sonnet.
Behavior
| Call |
Before |
After (proposed) |
chat(..., top_k=200) |
top_k dropped; never sent to Converse |
additionalModelRequestFields={"top_k": 200} sent |
_accepts_call_arg("top_k") |
True (but no effect) |
True (now honored) |
other args (max_tokens/temperature/top_p/stop_sequences) |
unchanged |
unchanged |
Proposed change (minimal)
Forward top_k via additionalModelRequestFields when provided — completing the support _accepts_call_arg already declares — plus a docstring line and a credential-free regression test that captures the kwargs passed to client.converse. This is framed as filling a gap in the existing convention, not adding new behavior.
I already have this implemented and verified locally (formatting via the repo-pinned black==24 and isort pass). Following CONTRIBUTING.md, I'm filing this issue first and will open a PR once a Pathway core-team engineer acks the approach.
This contribution was prepared with the help of an AI agent (Claude Code); a human reviewed the change, reasoning, and test results before submission.
First, thank you for Pathway and the recent native AWS Bedrock integration (#170) — the
BedrockChatConverse wrapper is genuinely useful. I ran into a small inconsistency around thetop_kinference parameter and wanted to report it before proposing any change, perCONTRIBUTING.md.Summary
BedrockChat._accepts_call_argadvertisestop_kas a supported inference argument, butBedrockChat.__wrapped__never forwards it to the Converse call, so it is silently dropped.Where
python/pathway/xpacks/llm/llms.py:_accepts_call_argliststop_kinsupported_args.__wrapped__, the ConverseinferenceConfigis built only frommax_tokens/temperature/top_p/stop_sequences.top_kis never read, so it does not reachclient.converse(...).Args:and theBEDROCK_VALID_ARGSlist intests/test_llms.pyboth omittop_k, so the advertised support is currently a dead entry.Why this is the expected fix
Per the AWS Converse API reference,
top_kis not part of the baseInferenceConfiguration(onlymaxTokens,stopSequences,temperature,topP). Model-specific parameters such astop_k(e.g. Anthropic Claude) must be passed viaadditionalModelRequestFields. The AWS docs even show this exact case (Example 3):"additionalModelRequestFields": { "top_k": 200 }againstanthropic.claude-3-sonnet.Behavior
chat(..., top_k=200)top_kdropped; never sent to ConverseadditionalModelRequestFields={"top_k": 200}sent_accepts_call_arg("top_k")True(but no effect)True(now honored)max_tokens/temperature/top_p/stop_sequences)Proposed change (minimal)
Forward
top_kviaadditionalModelRequestFieldswhen provided — completing the support_accepts_call_argalready declares — plus a docstring line and a credential-free regression test that captures the kwargs passed toclient.converse. This is framed as filling a gap in the existing convention, not adding new behavior.I already have this implemented and verified locally (formatting via the repo-pinned
black==24andisortpass). FollowingCONTRIBUTING.md, I'm filing this issue first and will open a PR once a Pathway core-team engineer acks the approach.This contribution was prepared with the help of an AI agent (Claude Code); a human reviewed the change, reasoning, and test results before submission.