[FIX] Strip deprecated sampling params for Claude Opus 4.7#1934
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds regex-based Anthropic model-id normalization and detection plus a helper that returns a shallow copy of validated params with ChangesSampling parameter cleanup & model detection
Sequence DiagramsequenceDiagram
participant Client as Client
participant Validator as Adapter Validator
participant Stripper as _strip_deprecated_sampling_params
participant Provider as Provider API
Client->>Validator: call validate(params)
Validator->>Stripper: _strip_deprecated_sampling_params(validated)
Stripper-->>Validator: cleaned_params
Validator->>Provider: forward cleaned_params
Provider-->>Client: response
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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
| Filename | Overview |
|---|---|
| unstract/sdk1/src/unstract/sdk1/adapters/base1.py | Adds _has_deprecated_sampling_params / _strip_deprecated_sampling_params helpers with a trailing-anchored regex and wires them into all four Anthropic-proxying adapter validate() methods. model_id field on AWSBedrockLLMParameters ensures the AIP ARN fallback path works correctly through Pydantic. |
| unstract/sdk1/tests/test_sampling_strip.py | New test module covering 24 positive / 14 negative detection cases, strip contract, AIP ARN fallback via model_id, debug-log guard behaviour, and end-to-end adapter wiring for all four providers including a dedicated Vertex AI regression test. |
Reviews (6): Last reviewed commit: "[FIX] Narrow strip-skipped breadcrumb to..." | Re-trigger Greptile
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
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?
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).
Address PR #1934 review: - HIGH: anchor regex no longer admits `v`-prefixed alpha continuations. `(?=$|[-:@/v])` accepted `claude-opus-4-7verbose`/`vnext`/`variant`. Switched to `(?=$|[-:@/]|v\d)` — keeps `v1` / `v9` version tags valid, rejects bare-`v` continuations. - HIGH: `_strip_deprecated_sampling_params` now logs a debug breadcrumb when sampling params are present but no model id field matches. This is the silent-skip path for opaque Bedrock AIP ARNs / Azure Foundry deployments that omit the model id — operators hitting the upstream 400 now have a way to confirm detection was attempted-and-skipped vs never attempted. - MED: helper no longer mutates its input. Returns a shallow copy, matching the file's `result_metadata = adapter_metadata.copy()` style. - MED: docstring documents the contract change — returned dict may omit the `temperature` key. Consumers must use `.get("temperature")`. Grepped platform-service / SDK / workers / backend for direct `["temperature"]` reads; only one consumer accesses it and uses `.get()` already, so no caller-side change needed today. - LOW: comment drift — `substring-matched` → `regex-searched`; misleading route-prefix bullet rewritten as "leading text passes through because the regex is anchored only at the trailing edge"; docs URL switched from `platform.claude.com` (console host) to `docs.claude.com` (canonical docs host); docstring extends top_p/top_k strip rationale; redundant Foundry inline comment removed (helper docstring covers it). - MED: new `tests/test_sampling_strip.py` (57 cases) pins - 23 positive id formats (native, Bedrock foundation/cross-region/ARNs, Vertex `@<date>`, Azure deployments, dot/underscore separators, `v\d` version tags) - 18 negatives including prefix collisions (`4-70`, `4-75`, `4-79`, `4-71-...`) and bare-`v` continuations (`4-7verbose`/`vnext`/`variant`) - copy-not-mutate, all-three-params strip, AIP fallback via both `model` and `model_id`, double-opaque limitation + debug log, quiet-no-op for non-deprecated models - parametrized end-to-end strip across all four adapters (Anthropic, Bedrock, Azure AI Foundry, Vertex AI) — locks the Vertex AI gap that surfaced in this PR's review. All 278 sdk1 tests pass (221 pre-existing + 57 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@unstract/sdk1/tests/test_sampling_strip.py`:
- Around line 158-160: The docstrings in test_sampling_strip.py (the three
multiline literals immediately preceding the inp definitions around the sections
noted) must follow D205/D209: ensure each docstring has a single-line summary,
then a blank line, then the body (if any), and place the closing triple quotes
on their own line; update the three offending docstrings (the one above the inp
= {...} at the first location and the two others referenced) to add the required
blank line after the summary and move the closing triple quotes to a separate
line.
🪄 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: 1eb9fa0e-795f-4c87-9d64-f57e08c69305
📒 Files selected for processing (2)
unstract/sdk1/src/unstract/sdk1/adapters/base1.pyunstract/sdk1/tests/test_sampling_strip.py
🚧 Files skipped from review as they are similar to previous changes (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>
Address PR #1934 review: - HIGH: anchor regex no longer admits `v`-prefixed alpha continuations. `(?=$|[-:@/v])` accepted `claude-opus-4-7verbose`/`vnext`/`variant`. Switched to `(?=$|[-:@/]|v\d)` — keeps `v1` / `v9` version tags valid, rejects bare-`v` continuations. - HIGH: `_strip_deprecated_sampling_params` now logs a debug breadcrumb when sampling params are present but no model id field matches. This is the silent-skip path for opaque Bedrock AIP ARNs / Azure Foundry deployments that omit the model id — operators hitting the upstream 400 now have a way to confirm detection was attempted-and-skipped vs never attempted. - MED: helper no longer mutates its input. Returns a shallow copy, matching the file's `result_metadata = adapter_metadata.copy()` style. - MED: docstring documents the contract change — returned dict may omit the `temperature` key. Consumers must use `.get("temperature")`. Grepped platform-service / SDK / workers / backend for direct `["temperature"]` reads; only one consumer accesses it and uses `.get()` already, so no caller-side change needed today. - LOW: comment drift — `substring-matched` → `regex-searched`; misleading route-prefix bullet rewritten as "leading text passes through because the regex is anchored only at the trailing edge"; docs URL switched from `platform.claude.com` (console host) to `docs.claude.com` (canonical docs host); docstring extends top_p/top_k strip rationale; redundant Foundry inline comment removed (helper docstring covers it). - MED: new `tests/test_sampling_strip.py` (57 cases) pins - 23 positive id formats (native, Bedrock foundation/cross-region/ARNs, Vertex `@<date>`, Azure deployments, dot/underscore separators, `v\d` version tags) - 18 negatives including prefix collisions (`4-70`, `4-75`, `4-79`, `4-71-...`) and bare-`v` continuations (`4-7verbose`/`vnext`/`variant`) - copy-not-mutate, all-three-params strip, AIP fallback via both `model` and `model_id`, double-opaque limitation + debug log, quiet-no-op for non-deprecated models - parametrized end-to-end strip across all four adapters (Anthropic, Bedrock, Azure AI Foundry, Vertex AI) — locks the Vertex AI gap that surfaced in this PR's review. All 278 sdk1 tests pass (221 pre-existing + 57 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
036ba8f to
328c58f
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
🧹 Nitpick comments (1)
unstract/sdk1/tests/test_sampling_strip.py (1)
243-245: ⚡ Quick winMake
top_p/top_kstrip assertions non-vacuous in adapter wiring tests.At Line 243 and Line 249, only
temperatureis passed in, so checks fortop_pandtop_kpass even if strip wiring regresses for those keys.Patch suggestion
@@ - result = cls.validate({"model": model, "temperature": 0.5, **extra}) + result = cls.validate( + {"model": model, "temperature": 0.5, "top_p": 0.9, "top_k": 40, **extra} + ) @@ -def test_vertex_validate_strips_temperature_for_opus_4_7() -> None: - result = VertexAILLMParameters.validate(_vertex_metadata("claude-opus-4-7@20260101")) +def test_vertex_validate_strips_temperature_for_opus_4_7() -> None: + result = VertexAILLMParameters.validate( + { + **_vertex_metadata("claude-opus-4-7@20260101"), + "top_p": 0.9, + "top_k": 40, + } + )Also applies to: 249-251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@unstract/sdk1/tests/test_sampling_strip.py` around lines 243 - 245, The test is only passing "temperature" into cls.validate so assertions that deprecated sampling keys like "top_p" and "top_k" are stripped are vacuous; update the adapter wiring tests that call cls.validate (the calls that produce the local variable result and then iterate over _DEPRECATED_SAMPLING_PARAMS) to include explicit values for top_p and top_k (e.g., top_p: 0.9, top_k: 50) in the input dict along with temperature so the loop asserting those keys are removed from result is non-vacuous and will catch regressions in the strip wiring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@unstract/sdk1/tests/test_sampling_strip.py`:
- Around line 243-245: The test is only passing "temperature" into cls.validate
so assertions that deprecated sampling keys like "top_p" and "top_k" are
stripped are vacuous; update the adapter wiring tests that call cls.validate
(the calls that produce the local variable result and then iterate over
_DEPRECATED_SAMPLING_PARAMS) to include explicit values for top_p and top_k
(e.g., top_p: 0.9, top_k: 50) in the input dict along with temperature so the
loop asserting those keys are removed from result is non-vacuous and will catch
regressions in the strip wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a62c4124-a32d-456e-b202-9c4cc16289fb
📒 Files selected for processing (2)
unstract/sdk1/src/unstract/sdk1/adapters/base1.pyunstract/sdk1/tests/test_sampling_strip.py
🚧 Files skipped from review as they are similar to previous changes (1)
- unstract/sdk1/src/unstract/sdk1/adapters/base1.py
The pre-commit.ci auto-fix moved the closing `"""` to its own line but left the multi-line summary intact, so D205 still flagged three docstrings on test_sampling_strip.py:158/177/234. Rewrite the three as single-line summary + blank + body so D205 actually passes. Also let ruff normalize the import block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
unstract/sdk1/tests/test_sampling_strip.py (1)
248-250: ⚡ Quick winMake adapter regression assertions non-vacuous for
top_p/top_k.These tests assert all deprecated params are stripped, but only
temperatureis provided.top_pandtop_kchecks can pass even if adapter stripping regresses.Proposed patch
def test_validate_strips_temperature_for_opus_4_7( name: str, cls: type, extra: dict[str, Any] ) -> None: @@ - result = cls.validate({"model": model, "temperature": 0.5, **extra}) + result = cls.validate( + {"model": model, "temperature": 0.5, "top_p": 0.9, "top_k": 40, **extra} + ) for param in _DEPRECATED_SAMPLING_PARAMS: assert param not in result, f"{name}: {param} should be stripped" @@ def test_vertex_validate_strips_temperature_for_opus_4_7() -> None: - result = VertexAILLMParameters.validate(_vertex_metadata("claude-opus-4-7@20260101")) + payload = _vertex_metadata("claude-opus-4-7@20260101") + payload.update({"top_p": 0.9, "top_k": 40}) + result = VertexAILLMParameters.validate(payload) for param in _DEPRECATED_SAMPLING_PARAMS: assert param not in result, f"vertex: {param} should be stripped"Also applies to: 254-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@unstract/sdk1/tests/test_sampling_strip.py` around lines 248 - 250, The assertions checking that deprecated sampling params are stripped are vacuous because only "temperature" is passed; update the test calls to pass values for top_p and top_k (e.g., include "top_p": 0.9 and "top_k": 40 in the extra dict) when calling cls.validate so that _DEPRECATED_SAMPLING_PARAMS checks for "top_p" and "top_k" are exercised; apply the same change to the second occurrence (the block at lines 254-256) to make both adapter-regression assertions non-vacuous.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@unstract/sdk1/tests/test_sampling_strip.py`:
- Around line 248-250: The assertions checking that deprecated sampling params
are stripped are vacuous because only "temperature" is passed; update the test
calls to pass values for top_p and top_k (e.g., include "top_p": 0.9 and
"top_k": 40 in the extra dict) when calling cls.validate so that
_DEPRECATED_SAMPLING_PARAMS checks for "top_p" and "top_k" are exercised; apply
the same change to the second occurrence (the block at lines 254-256) to make
both adapter-regression assertions non-vacuous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6a1e897-4e60-4e89-93d7-5ad081f9f7cc
📒 Files selected for processing (1)
unstract/sdk1/tests/test_sampling_strip.py
for more information, see https://pre-commit.ci
SonarCloud flagged four `== 0.5` checks against temperature reads in test_sampling_strip.py as "Do not perform equality checks with floating point values". Switch to `pytest.approx(0.5)` to match the project convention (test_gemini_adapter.py:42 uses the same form). Reword one trailing comment as a leading comment to keep the assertion under the 90-char line limit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jaseemjaskp
left a comment
There was a problem hiding this comment.
LGTM on the sampling-strip fix. The trailing-edge anchor, the model_id fallback, the four-adapter wiring, and the new test suite all address the prior round of review feedback. The one remaining outstanding concern on this PR — greptile-apps P1 (debug breadcrumb fires for every non-Opus-4.7 call because BaseChatCompletionParameters.temperature has Field(default=0.1), so model_dump() always emits it) — is a real, valid point and worth addressing before merge, but it is already posted and does not need duplication from me. Nothing additional to raise.
Greptile flagged that `BaseChatCompletionParameters.temperature` declares `default=0.1`, so Pydantic's `model_dump()` always emits `temperature` — the previous `elif any(p in result for p in _DEPRECATED_SAMPLING_PARAMS)` fired for every routine call through these adapters (claude-3-5-sonnet, claude-opus-4-6, gpt-4o, …), producing log noise that mimicked a failed strip attempt. Replace the guard with `_looks_like_opaque_aip_arn(...)` keyed off the `application-inference-profile` substring — the one concrete case where the model id is genuinely unrecoverable from the string and the upstream will 400 if the underlying foundation model is Opus 4.7+. Add a regression test (`test_strip_does_not_log_when_sampling_params_present_ but_model_not_opaque`) that locks the no-noise invariant for the routine case. The existing test that exercises the breadcrumb path (`test_strip_skipped_when_both_fields_opaque_and_logs_debug`) still passes because both fields hold AIP ARNs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|



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.