Skip to content

fix(bedrock,sagemaker): keep temperature=0 and top_p=0 - #1299

Merged
tbille merged 1 commit into
mozilla-ai:mainfrom
tonycoder-hub:cursor/fix-zero-valued-sampling-params-7272
Aug 18, 2026
Merged

tbille merged 1 commit into
mozilla-ai:mainfrom
tonycoder-hub:cursor/fix-zero-valued-sampling-params-7272

Conversation

@tonycoder-hub

@tonycoder-hub tonycoder-hub commented Aug 17, 2026 •

Copy link
Copy Markdown
Contributor

Description

Bedrock and SageMaker _convert_params gated temperature and top_p on truthiness, so 0.0 was dropped and the request used the model default (e.g. Claude 1.0 on Bedrock) instead of greedy decoding. Other providers already use is not None.

max_tokens and stop stay on truthiness: 0 and empty sequences are rejected by both providers.

PR Type

  • Bug Fix

Relevant issues

No open issue. Distinct from #1292 (Anthropic tool_choice) and our #1296/#1297/#1298.

Checklist

  • I understand the code I am submitting.
  • I have added unit tests that prove my fix/feature works
  • I have run this code locally and verified it fixes the issue.
  • New and existing tests pass locally
  • Documentation was updated where necessary
  • I have read and followed the contribution guidelines
  • AI Usage:
    • This is fully AI-generated.

AI Usage Information

  • AI Model used: Claude Opus 5
  • AI Developer Tool used: Cursor cloud agent
  • I am an AI Agent filling out this form (check box if true)

uv run pytest tests/unit → 2161 passed, 69 skipped. Target files fail on current main (KeyError: inferenceConfig / missing keys) and pass after.

Summary by CodeRabbit

  • Bug Fixes

    • AWS Bedrock and SageMaker integrations now correctly honour explicitly configured zero values for temperature and top-p sampling parameters.
    • Unset sampling parameters continue to be omitted from requests.
  • Tests

    • Added coverage confirming zero-valued parameters are forwarded and unset values are excluded.

…dropping them

Co-authored-by: Tony Coder <407243179@qq.com>
@coderabbitai

coderabbitai Bot commented Aug 17, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e75537a5-53d1-4e81-b487-1e94737074a3

📥 Commits

Reviewing files that changed from the base of the PR and between c0f3ffb and ecf8400.

📒 Files selected for processing (4)
  • src/any_llm/providers/bedrock/utils.py
  • src/any_llm/providers/sagemaker/utils.py
  • tests/unit/providers/test_aws_provider.py
  • tests/unit/providers/test_sagemaker_provider.py

Included review availability: Your plan includes up to 8 reviews per rolling hour; 5 remain after this review.


Walkthrough

The change updates Bedrock and SageMaker parameter conversion to preserve explicit temperature=0.0 and top_p=0.0 values. Tests cover forwarding zero values and omitting unset sampling parameters.

Changes

Sampling parameter conversion

Layer / File(s) Summary
Preserve explicit sampling values
src/any_llm/providers/bedrock/utils.py, src/any_llm/providers/sagemaker/utils.py
The conversion logic now checks temperature and top_p against None, so zero values are forwarded.
Validate conversion behaviour
tests/unit/providers/test_aws_provider.py, tests/unit/providers/test_sagemaker_provider.py
Tests verify that zero-valued parameters are retained and unset parameters are omitted.

Suggested reviewers: peteski22

Merge Risk: ⚪ Minimal · up to ecf84

The change preserves explicit zero values for temperature and top_p so greedy decoding is requested as intended; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the fix for zero-valued temperature and top_p parameters in Bedrock and SageMaker.
Description check ✅ Passed The description follows the required template, explains the fix, identifies the bug-fix type, records testing, and completes the checklist and AI usage details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/any_llm/providers/bedrock/utils.py 82.70% <100.00%> (-7.79%) ⬇️
src/any_llm/providers/sagemaker/utils.py 20.87% <100.00%> (+12.08%) ⬆️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tbille
tbille merged commit 40f2b44 into mozilla-ai:main Aug 18, 2026
16 checks passed
JamMaster1999 added a commit to JamMaster1999/any-llm that referenced this pull request Aug 18, 2026
Brings in upstream's merges of our mozilla-ai#1291/mozilla-ai#1292/mozilla-ai#1310 plus mozilla-ai#1297, mozilla-ai#1299,
mozilla-ai#1301, mozilla-ai#1302, mozilla-ai#1303, mozilla-ai#1305. Carried-until-merged fork work stays:
mozilla-ai#1294 (gemini reasoning_effort=none), mozilla-ai#1308 (aresponses timeout),
mozilla-ai#1309 (gemini native tool dicts), and the mozilla-ai#1300 carry.
One conflict in tests/unit/test_responses.py: kept our mozilla-ai#1308 timeout
test next to upstream's flatten test. Unit suite: 2234 passed.

Claude-Session: https://claude.ai/code/session_018D3FGNvb1hRZQmsXFoA44J
@github-actions github-actions Bot added the 1.27.0 Included in release 1.27.0 label Sep 3, 2026

This branch was previously deployed

1 inactive deployment
integration-tests — ecf84001 Deployed Aug 18, 2026 by tonycoder-hub via run-docs-tests #2454
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.27.0 Included in release 1.27.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants