fix: strip constructor-set sampling fields at construction time#83
Merged
Conversation
cosminacho
requested review from
DragosBobolea,
cristipufu,
dragosvelcea,
ionmincu,
ionut-mihalache-uipath and
radu-mocanu
as code owners
cosminacho
changed the title
ionut-mihalache-uipath
previously approved these changes
May 20, 2026
`UiPathChatAnthropicBedrock(model="anthropic.claude-opus-4-7", temperature=0.7)` previously leaked the disabled field into the request body because langchain- anthropic and langchain-aws build payloads from `self.<field>`, bypassing the existing kwargs-level `strip_disabled_kwargs`. The gateway then rejected the call with 400 (modelDetails.shouldSkipTemperature: true for reasoning models). Adds `disabled_fields_stripped` context manager in core sampling utils — a sibling of `strip_disabled_kwargs` that temporarily nulls matching instance attributes for the duration of the underlying call and restores them on exit. Wired into the four `_generate`/`_agenerate`/`_stream`/`_astream` wrappers in `UiPathBaseChatModel`, so caller-visible state (`chat.temperature`) is unchanged but the vendor SDK reads `None` while building the request. Plugs the init-time leak called out as a known follow-up in 1.10.0. Tests: - 7 new unit tests pin down the during-call/after-call semantics, exception restoration, value-list spec handling, and warning logging. - 2 new VCR-cassetted integration tests against `anthropic.claude-opus-4-7` exercise both vendor SDK families (anthropic-bedrock + bedrock-converse). The recorded 200 is itself proof of the fix — `before_record_response` drops 4xx, so a pre-fix run would have refused to persist the cassette. Core 1.11.0 -> 1.11.2 (new helper exposed) Langchain 1.11.1 -> 1.11.2 (wiring + dep floor bump) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t manager Prior approach used a context manager that temporarily nulled matching fields for the duration of each call and restored them on exit, so `chat.temperature` read the caller's original value between invocations. That's sneaky: the gateway will never accept the value, so reporting 0.7 on the instance is a lie that hides the truth from the user. Replace `disabled_fields_stripped` (context manager) with `strip_disabled_fields` (eager, one-shot). Wire it into `UiPathBaseLLMClient.setup_model_info` so the strip happens once, immediately after `disabled_params` resolves. Each strip logs a warning that includes the original value, so the caller sees exactly what was dropped at construction time rather than being silently surprised later. The four `_generate`/`_agenerate`/`_stream`/`_astream` wrappers are now plain again — no context-manager wrapping, no save/restore, no exception edge cases, no generator subtlety. Net effect for callers: - `UiPathChatAnthropicBedrock(model="anthropic.claude-opus-4-7", temperature=0.7)` now logs a warning and leaves `chat.temperature is None`. - Per-call `chat.invoke(..., temperature=0.7)` still gets stripped by the existing `strip_disabled_kwargs` filter, unchanged. Tests updated to assert the field is None after construction (not "restored after call"). Restore-on-exception test removed (no longer applicable). VCR cassettes still replay — the gateway request body is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous object.__setattr__ was defensive bypass-validation, but every field in DISABLED_SAMPLING_PARAMS is declared as `<type> | None = None` on the underlying langchain subclasses, so None is always valid. `strip_disabled_fields` runs inside `@model_validator(mode="after")` and operates on the constructed pydantic model, so plain setattr already goes through `BaseModel.__setattr__` and respects field types. If a future caller tries to disable a non-nullable custom field, raising is the right behavior — better than silently bypassing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cosminacho
force-pushed
the
fix/strip-disabled-fields-at-invoke
branch
from
7524161 to
8d267e9
Compare
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
2 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.
Summary
UiPathChatAnthropicBedrock(model="anthropic.claude-opus-4-7", temperature=0.7)previously leaked the disabled field into the outgoing request body — langchain-anthropic and langchain-aws build payloads fromself.<field>rather than the per-call kwargs, so the existingstrip_disabled_kwargsfilter never saw it. The gateway then rejected the call with400 BadRequestErrorbecausemodelDetails.shouldSkipTemperature: truefor reasoning-style models.strip_disabled_fieldsinuipath.llm_client.utils.sampling— a sibling ofstrip_disabled_kwargsthat eagerly nulls matching instance attributes. Called once fromUiPathBaseLLMClient.setup_model_inforight afterdisabled_paramsresolves, so the strip happens at construction time. A warning is logged per stripped field with the original value, so callers see exactly what was dropped (no silent surprises).1.10.0.Why construction-time strip (not invoke-time)
The first iteration of this PR used a context manager that temporarily nulled matching fields for the duration of each call and restored them on exit. That preserved
chat.temperature == 0.7on the instance, but it was sneaky: the gateway will never accept that value, so reporting0.7was a lie that hid the truth from the user. The eager construction-time strip is honest (chat.temperature is Nonematches what the gateway sees), simpler (no save/restore, no exception edge cases, no generator wrapping for streams), and gives the user immediate feedback via the warning rather than letting them be surprised at invoke time.Packages affected
Both — touches core (
utils/sampling.py) and langchain (base_client.py):1.11.0->1.11.2(exposes the new helper)1.11.1->1.11.2(wiring + dep-floor bump to>=1.11.2)Tests
tests/langchain/test_disabled_sampling_params.pypin down: field nulled when flag set; not touched when flag absent; value-list spec respected; default-None fields not warned about; warning includes the original value; silent whenlogger=None.tests/langchain/test_disabled_sampling_params_integration.pyexercise the fix end-to-end againstanthropic.claude-opus-4-7for both vendor SDK families (UiPathChatAnthropicBedrock-> langchain-anthropic,UiPathChatBedrockConverse-> langchain-aws). The recorded 200 response is itself proof of the fix —before_record_responseintests/conftest.pydrops 4xx/5xx, so a pre-fix recording would have refused to persist a cassette.Test plan
ruff check && ruff format . --check— cleanpyright— 0 errorspytest tests— 1560 passed, 0 failedUiPathChatAnthropicBedrock,UiPathChatBedrockConverse, and the INVOKE-flavor dispatch — all three succeed with constructor-settemperature=0.7.🤖 Generated with Claude Code