[FIX] Strip deprecated sampling params for Claude Opus 4.7#1934
Open
pk-zipstack wants to merge 2 commits intomainfrom
Open
[FIX] Strip deprecated sampling params for Claude Opus 4.7#1934pk-zipstack wants to merge 2 commits intomainfrom
pk-zipstack wants to merge 2 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughAdds detection for Anthropic models that reject sampling parameters and a helper that removes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
|
| Filename | Overview |
|---|---|
| unstract/sdk1/src/unstract/sdk1/adapters/base1.py | Adds two module-level helpers (_has_deprecated_sampling_params, _strip_deprecated_sampling_params) and wires them into VertexAI, Bedrock, Anthropic, and Azure AI Foundry validate() methods; regex anchoring addresses the previous substring-collision concern; thinking-mode / temperature interaction needs verification for Claude Opus 4.7. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[adapter_metadata input] --> B[validate_model - prefix model id]
B --> C{enable_thinking?}
C -- Yes --> D[set temperature=1 in result_metadata]
C -- No --> E[skip]
D --> F[model_dump via Pydantic]
E --> F
F --> G[_strip_deprecated_sampling_params]
G --> H{model matches claude-opus-4-7?}
H -- Yes --> I[pop temperature / top_p / top_k]
H -- No --> J[return unchanged]
I --> K[return validated dict - temperature=1 also stripped]
J --> K
Comments Outside Diff (1)
-
unstract/sdk1/src/unstract/sdk1/adapters/base1.py, line 586-615 (link)Thinking mode temperature stripped for Claude Opus 4.7
When
enable_thinking=Trueand the model is Claude Opus 4.7,AWSBedrockLLMParameters.validate()explicitly setstemperature=1at line 588 (required for thinking mode on earlier Claude models), but_strip_deprecated_sampling_paramsthen pops that sametemperaturekey before the dict is returned.If Bedrock's Converse API for Claude Opus 4.7 still needs
temperature=1to activate extended thinking, those calls will silently lose that field. The same race exists inAnthropicLLMParameters.validate()at line 680. Confirm with the Anthropic / Bedrock docs that Claude Opus 4.7 thinking mode operates correctly without anytemperaturefield before shipping; if it does require it, the strip should guard thinking-mode calls.Prompt To Fix With AI
This is a comment left during a code review. Path: unstract/sdk1/src/unstract/sdk1/adapters/base1.py Line: 586-615 Comment: **Thinking mode temperature stripped for Claude Opus 4.7** When `enable_thinking=True` and the model is Claude Opus 4.7, `AWSBedrockLLMParameters.validate()` explicitly sets `temperature=1` at line 588 (required for thinking mode on earlier Claude models), but `_strip_deprecated_sampling_params` then pops that same `temperature` key before the dict is returned. If Bedrock's Converse API for Claude Opus 4.7 still needs `temperature=1` to activate extended thinking, those calls will silently lose that field. The same race exists in `AnthropicLLMParameters.validate()` at line 680. Confirm with the Anthropic / Bedrock docs that Claude Opus 4.7 thinking mode operates correctly without any `temperature` field before shipping; if it does require it, the strip should guard thinking-mode calls. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: unstract/sdk1/src/unstract/sdk1/adapters/base1.py
Line: 586-615
Comment:
**Thinking mode temperature stripped for Claude Opus 4.7**
When `enable_thinking=True` and the model is Claude Opus 4.7, `AWSBedrockLLMParameters.validate()` explicitly sets `temperature=1` at line 588 (required for thinking mode on earlier Claude models), but `_strip_deprecated_sampling_params` then pops that same `temperature` key before the dict is returned.
If Bedrock's Converse API for Claude Opus 4.7 still needs `temperature=1` to activate extended thinking, those calls will silently lose that field. The same race exists in `AnthropicLLMParameters.validate()` at line 680. Confirm with the Anthropic / Bedrock docs that Claude Opus 4.7 thinking mode operates correctly without any `temperature` field before shipping; if it does require it, the strip should guard thinking-mode calls.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "[FIX] Strip sampling params on Vertex AI..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@unstract/sdk1/src/unstract/sdk1/adapters/base1.py`:
- Around line 50-53: VertexAILLMParameters.validate currently returns
validated_data without stripping Anthropic deprecated sampling params; modify
validate to call self._strip_deprecated_sampling_params(validated_data) (like
the other providers do) before returning so temperature/top_p/top_k are removed
for Anthropic/Claude Opus 4.7+ model IDs (e.g., vertex_ai/claude-opus-4-7@...) —
update the return path in VertexAILLMParameters.validate to return the stripped
validated_data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c85dc73c-bee1-4dd3-b96a-4f28eeb190f7
📒 Files selected for processing (1)
unstract/sdk1/src/unstract/sdk1/adapters/base1.py
Address review feedback on the Opus 4.7 sampling-param strip: - VertexAILLMParameters.validate() now returns the stripped dict, so Claude Opus 4.7 routed through Vertex AI (vertex_ai/claude-opus-4-7@...) no longer sends temperature/top_p/top_k. - Model pattern is now an anchored regex (`claude-opus-4-7(?=$|[-:@/v])`) so prefix collisions like claude-opus-4-70, claude-opus-4-75 do not silently strip sampling params from unrelated future models. The allowed delimiters cover every real id format: date suffix, Vertex @<date>, ARN paths, v1:0 version tag, and end-of-string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
chandrasekharan-zipstack
approved these changes
Apr 29, 2026
Contributor
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
@pk-zipstack will we still show the temperature field in the UI for such models?
Deepak-Kesavan
approved these changes
Apr 29, 2026
jaseemjaskp
reviewed
May 2, 2026
Contributor
jaseemjaskp
left a comment
jaseemjaskp
left a comment
There was a problem hiding this comment.
Automated review pass (PR Review Toolkit: code-reviewer, silent-failure-hunter, type-design-analyzer, comment-analyzer, code-simplifier, pr-test-analyzer). Findings posted inline; deduplicated against existing greptile/CodeRabbit/maintainer comments. Grouped by line and roughly prioritised (HIGH → LOW/optional).
| # the `v1:0` version tag. | ||
| # See https://platform.claude.com/docs/en/about-claude/models/whats-new-claude-4-7 | ||
| _SAMPLING_DEPRECATED_MODEL_PATTERNS: tuple[re.Pattern[str], ...] = ( | ||
| re.compile(r"claude-opus-4-7(?=$|[-:@/v])"), |
Contributor
There was a problem hiding this comment.
HIGH — anchor lookahead admits v-prefixed words, not just version tags
(?=$|[-:@/v]) puts a bare v in the character class, so the trailing-edge boundary fires for any token starting with the letter v — e.g. claude-opus-4-7verbose, claude-opus-4-7vnext, or any future suffix-with-v id would match (currently theoretical, but the comment on lines 34–38 explicitly claims the anchor blocks unrelated future ids).
Fix: require the version pattern explicitly, e.g.
re.compile(r"claude-opus-4-7(?=$|[-:@/]|v\d)")Keeps …-v1:0 / …v1 valid while rejecting alpha continuations starting with v. Extends the anchor concern raised earlier by greptile rather than duplicating it.
| """ | ||
| if any(_has_deprecated_sampling_params(validated.get(f)) for f in _MODEL_ID_FIELDS): | ||
| for param in _DEPRECATED_SAMPLING_PARAMS: | ||
| validated.pop(param, None) |
Contributor
There was a problem hiding this comment.
HIGH — silent no-op when sampling params are present but no model-id field matches
If a Bedrock Application Inference Profile ARN with an opaque tail (or an Azure Foundry deployment whose name omits the model id) reaches this helper while temperature/top_p/top_k are populated, the strip silently returns the dict unchanged. The failure surfaces only as a 400 from Anthropic after dispatch, with no breadcrumb tying it back to the strip path. Six months from now an operator hitting that 400 has no way to know detection was attempted-and-skipped vs. never attempted.
Fix: add a single logger.debug (or logger.warning if you want it to page operators) on the no-strip branch only when at least one of _DEPRECATED_SAMPLING_PARAMS is present in validated:
if matched:
for p in _DEPRECATED_SAMPLING_PARAMS:
validated.pop(p, None)
elif any(p in validated for p in _DEPRECATED_SAMPLING_PARAMS):
logger.debug(
"Sampling-strip skipped; model ids did not match deprecation patterns: %s",
{f: validated.get(f) for f in _MODEL_ID_FIELDS if validated.get(f)},
)Keeps the AIP/Foundry fallback non-fatal but observable.
| standard model id only appears in `model_id`. | ||
| """ | ||
| if any(_has_deprecated_sampling_params(validated.get(f)) for f in _MODEL_ID_FIELDS): | ||
| for param in _DEPRECATED_SAMPLING_PARAMS: |
Contributor
There was a problem hiding this comment.
MED — post-strip temperature key invariant breaks validated["temperature"] consumers
BaseChatCompletionParameters.temperature defaults to Field(default=0.1), so before this PR every validate() return was guaranteed to carry temperature. After the strip, callers using validated["temperature"] (vs. .get) on an Opus 4.7 path now raise KeyError. Anything that reads this key for cost-tracking, telemetry, or UI display in the platform service / SDK runtime would silently break only on 4.7.
Fix: grep platform-service / SDK consumers of validate() output for direct ["temperature"] access and switch to .get("temperature"); document in this docstring that temperature may be absent post-strip so future consumers know the key is now optional.
| return any(rx.search(normalized) for rx in _SAMPLING_DEPRECATED_MODEL_PATTERNS) | ||
|
|
||
|
|
||
| def _strip_deprecated_sampling_params(validated: dict[str, "Any"]) -> dict[str, "Any"]: |
Contributor
There was a problem hiding this comment.
MED — mutate-and-return is inconsistent with surrounding callers
The rest of this file consistently builds result_metadata = adapter_metadata.copy() (e.g. lines 264, 350, 584) when avoiding mutation. This helper mutates validated in place and returns the same dict; today’s four call sites are safe, but the asymmetry invites a future caller to do before = validated; after = _strip_deprecated_sampling_params(validated) and be surprised that before is after.
Fix (lowest-risk): rename to _strip_deprecated_sampling_params_inplace and return None, or shallow-copy and return the copy. The dict is small — a copy costs nothing and matches surrounding style.
Type-design note: invariant strength would also improve if the pattern ↔ forbidden-params link were structural rather than positional (parallel _SAMPLING_DEPRECATED_MODEL_PATTERNS and _DEPRECATED_SAMPLING_PARAMS tuples). Worth folding into ((pattern, frozenset(params)), …) only when a second deprecation lands with a different param set — not before.
| if any(_has_deprecated_sampling_params(validated.get(f)) for f in _MODEL_ID_FIELDS): | ||
| for param in _DEPRECATED_SAMPLING_PARAMS: | ||
| validated.pop(param, None) | ||
| return validated |
Contributor
There was a problem hiding this comment.
MED — zero committed tests for the new helpers or the four wired call sites
git diff origin/main…HEAD --name-only shows only base1.py — the PR description’s 24-positive / 8-negative / AIP-fallback / four-end-to-end matrix is ad-hoc local. Repo-wide grep for claude-opus-4-7, _strip_deprecated_sampling_params, _has_deprecated_sampling_params matches only this file. The Vertex AI wiring gap that surfaced in review (commit 5a4ea27f shipped without it; addressed in 7fb66f15) is exactly the failure mode a parametrized adapter test would have caught.
Suggested minimal lock-in (unstract/sdk1/tests/adapters/test_base1_sampling_strip.py):
test_has_deprecated_sampling_params_positive—pytest.mark.parametrizeover the 24 id formats from the PR description (native,anthropic.Bedrock,us./eu./apac./global.cross-region, foundation-model ARN, inference-profile ARN,vertex_ai/...@<date>, Azure deployment,converse/,invoke/,./_separators, mixed case).test_has_deprecated_sampling_params_negative—claude-opus-4-6,claude-opus-4-70,claude-opus-4-75,claude-sonnet-4-7,claude-haiku-4-5,gpt-4o,gemini-2.0-flash, opaque AIP ARN,None,"". The4-70/4-75cases lock the(?=$|[-:@/v])anchor.test_strip_checks_model_id_field_fallback— opus id inmodelonly, inmodel_idonly, both opaque (no strip).test_validate_strips_for_<adapter>— four parametrized cases (AnthropicLLMParameters,AWSBedrockLLMParameters,AzureAIFoundryLLMParameters,VertexAILLMParameters), each asserting the three keys absent for opus 4.7 / present for opus 4.6. A future refactor that drops a single call site regresses silently without these.
| # after lowercasing and normalizing `.` / `_` to `-`, so a single entry covers: | ||
| # - Native Anthropic `claude-opus-4-7`, `anthropic/claude-opus-4-7` | ||
| # - Bedrock foundation model `anthropic.claude-opus-4-7-<date>-v1:0` | ||
| # - Bedrock route prefixes `converse/...`, `invoke/...` |
Contributor
There was a problem hiding this comment.
LOW — misleading bullet: route prefixes are not part of the pattern
“Bedrock route prefixes converse/..., invoke/...” reads as if the regex enumerates prefixes; it does not. The regex matches the claude-opus-4-7 token regardless of leading text, so prefixes like converse/ / invoke/ / bedrock/converse/ are accepted only as a side-effect of the trailing-edge-only anchor. Either drop the bullet or rephrase as “prefixes such as converse/ and invoke/ pass through because the match is anchored only at the trailing edge.”
| # `claude-opus-4-70`, `claude-opus-4-75` do not match. The allowed delimiters | ||
| # cover the date suffix (`-20260101-v1:0`), Vertex `@<date>`, ARN paths, and | ||
| # the `v1:0` version tag. | ||
| # See https://platform.claude.com/docs/en/about-claude/models/whats-new-claude-4-7 |
Contributor
There was a problem hiding this comment.
LOW — verify canonical docs URL
https://platform.claude.com/docs/en/about-claude/models/whats-new-claude-4-7 — the canonical Anthropic docs host is docs.claude.com / docs.anthropic.com; platform.claude.com is the console host and may not serve this docs path stably. Worth checking the link resolves before merge so the rationale comment doesn’t rot.
| def _strip_deprecated_sampling_params(validated: dict[str, "Any"]) -> dict[str, "Any"]: | ||
| """Drop sampling params for models that reject them (e.g. Claude Opus 4.7). | ||
|
|
||
| Pydantic's `model_dump()` re-emits the default `temperature=0.1` even when |
Contributor
There was a problem hiding this comment.
LOW — docstring “why” understates the scope of the strip
“Pydantic’s model_dump() re-emits the default temperature=0.1” is the load-bearing reason for popping temperature, but top_p / top_k are not declared fields on BaseChatCompletionParameters and so are not subject to the same Pydantic-default re-emit. The strip’s reason for handling them is to defend against caller-supplied values flowing through **adapter_metadata. Worth one extra clause so future readers understand both halves of the invariant.
(Distinct from the existing pk-zipstack / chandrasekharan-zipstack thread on this line about whether litellm handles the cleanup upstream.)
| # cover the date suffix (`-20260101-v1:0`), Vertex `@<date>`, ARN paths, and | ||
| # the `v1:0` version tag. | ||
| # See https://platform.claude.com/docs/en/about-claude/models/whats-new-claude-4-7 | ||
| _SAMPLING_DEPRECATED_MODEL_PATTERNS: tuple[re.Pattern[str], ...] = ( |
Contributor
There was a problem hiding this comment.
OPTIONAL (simplify) — tuple-of-patterns + _MODEL_ID_FIELDS are over-abstracted for one entry today
With exactly one regex and exactly two id-bearing fields, the indirection costs more than it saves. If you want to reduce surface:
_SAMPLING_DEPRECATED_MODEL_RE = re.compile(r"claude-opus-4-7(?=$|[-:@/v])")
…
def _strip_deprecated_sampling_params(validated):
if _has_deprecated_sampling_params(validated.get("model")) \
or _has_deprecated_sampling_params(validated.get("model_id")):
for param in _DEPRECATED_SAMPLING_PARAMS:
validated.pop(param, None)
return validatedGoing back to a tuple/loop when (if) a second deprecation lands is a 2-line change. Reasonable people will disagree — the current shape is readable; this is purely a YAGNI call. Skip if you prefer keeping the table for forward-compat.
| validated = AzureAIFoundryLLMParameters(**adapter_metadata).model_dump() | ||
| # Azure AI Foundry proxies Anthropic models that reject sampling params | ||
| # (e.g. Claude Opus 4.7); detection relies on the deployment name | ||
| # embedding the model id. |
Contributor
There was a problem hiding this comment.
LOW — inline comment is redundant and understates the failure mode
The helper docstring already documents the Foundry deployment-name caveat, so the three lines here mostly restate it. If the comment stays, strengthen the second half: “detection relies on the deployment name embedding the Anthropic model id; opaque deployment names skip the strip and will surface as a 400 from the provider.” Otherwise delete — the helper call reads self-explanatorily.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
_has_deprecated_sampling_params(model)and_strip_deprecated_sampling_params(validated)helpers inunstract/sdk1/.../adapters/base1.py. The helpers substring-match the model id againstclaude-opus-4-7(case-insensitive, with./_normalized to-) and poptemperature/top_p/top_kfrom the validated metadata when matched.AnthropicLLMParameters.validate(),AWSBedrockLLMParameters.validate(), andAzureAIFoundryLLMParameters.validate().modelandmodel_id, so Bedrock callers routing through an Application Inference Profile (AIP) are covered when the standard model id appears in either field.Why
Anthropic deprecated
temperature,top_p, andtop_kstarting with Claude Opus 4.7. Sending any of them now returns a 400 from Anthropic and from every provider that proxies it (Bedrock, Azure AI Foundry, Vertex AI). The Azure AI Foundry path is what surfaced first in the connector test:Pydantic's
model_dump()onBaseChatCompletionParametersre-emits the defaulttemperature=0.1even when the caller never set one, so we cannot rely on "don't pass it" — we have to actively pop it after validation.Reference: https://platform.claude.com/docs/en/about-claude/models/whats-new-claude-4-7
How
base1.py:_SAMPLING_DEPRECATED_MODEL_PATTERNS = ("claude-opus-4-7",)and_DEPRECATED_SAMPLING_PARAMS = ("temperature", "top_p", "top_k"). The pattern table is the single point of extension — if Anthropic deprecates sampling on more models later, we add an entry here._has_deprecated_sampling_paramslowercases the input and normalizes.and_separators to-before substring-matching, which covers every encoding of the id we found:claude-opus-4-7,anthropic/claude-opus-4-7anthropic.claude-opus-4-7-<date>-v1:0, with optionalbedrock/converse/.../bedrock/invoke/...route prefixesus.,eu.,apac.,global.variantsvertex_ai/claude-opus-4-7@<date>_strip_deprecated_sampling_paramschecks bothmodelandmodel_id(Bedrock's separate AIP ARN field) and pops the three sampling params if either one matches.AnthropicLLMParameters.validate(),AWSBedrockLLMParameters.validate(),AzureAIFoundryLLMParameters.validate()— return_strip_deprecated_sampling_params(validated)instead ofvalidated.Coverage gap (documented in helper docstring)
Two cases the string-based detector cannot cover:
arn:aws:bedrock:...:application-inference-profile/abcd1234. The string carries no model id. Mitigation: callers should keep the standard model id in eithermodelormodel_id. If both fields hold opaque ARNs, the strip won't fire.claude-opus-4-7. Mitigation: rename the deployment to include the model id, or — if this becomes common — add an explicit UI flag.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No.
claude-opus-4-7after normalization. Every other Anthropic / Bedrock / Azure AI Foundry model retains itstemperature/top_p/top_kexactly as before — verified by directvalidate()calls againstclaude-3-5-sonnet,claude-opus-4-6,claude-sonnet-4-7,gpt-4o, etc.top_p/top_kare not declared fields on the parameter classes today, so Pydantic was already dropping them silently for non-deprecated models. The defensive pop on the validated dict is a no-op for them in the normal path.Database Migrations
Env Config
Relevant Docs
thinking.budget_tokensalso removed (out of scope here).Related Issues or PRs
Dependencies Versions
Notes on Testing
@-suffixed, Azure deployments, mixed case, dot/underscore separators). All match. Negatives —claude-opus-4-6,claude-sonnet-4-7,claude-haiku-4-5,gpt-4o,gemini-2.0-flash, opaque AIP ARN,None, empty string — all correctly do not match.modelwhen standard id is inmodeland AIP ARN is inmodel_id; confirmed strip viamodel_idwhen AIP ARN is inmodeland standard id is inmodel_id; confirmed limitation when both fields hold opaque ARNs.AnthropicLLMParameters.validate(),AWSBedrockLLMParameters.validate(),AzureAIFoundryLLMParameters.validate()— temperature stripped for Opus 4.7 inputs, retained for older Claude / non-Claude inputs.pytest tests/ --ignore=tests/file_storage --ignore=tests/patches: 221 pass. (The unrelated import error intests/patches/test_litellm_cohere_timeout.pyis pre-existing onmain.)ruff format --checkclean on the modified file.Screenshots
N/A — backend SDK change.
Checklist
I have read and understood the Contribution Guidelines.