Skip to content

Feat: strip sampling params when modelDetails.shouldSkipTemperature is true#74

Merged
cosminacho merged 14 commits intomainfrom
fix/skip-disabled-sampling-params
Apr 23, 2026
Merged

Feat: strip sampling params when modelDetails.shouldSkipTemperature is true#74
cosminacho merged 14 commits intomainfrom
fix/skip-disabled-sampling-params

Conversation

@cosminacho
Copy link
Copy Markdown
Collaborator

@cosminacho cosminacho commented Apr 23, 2026

Summary

  • Problem: anthropic.claude-opus-4-7 (and other reasoning-style models) reject every sampling parameter. The LLM Gateway returns 400 for llm.invoke(..., temperature=0.2). The discovery endpoint already advertises this via modelDetails.shouldSkipTemperature, but we weren't reading it.
  • Fix (invoke-time only, this PR): UiPathBaseChatModel now strips temperature, top_p, top_k, frequency_penalty, presence_penalty, seed, logit_bias, logprobs, top_logprobs from invoke kwargs before forwarding to _uipath_generate/_uipath_agenerate/_uipath_stream/_uipath_astream when modelDetails.shouldSkipTemperature is true. n is intentionally not treated as a sampling knob.
  • Plumbing: new model_details: dict | None field on UiPathBaseLLMClient. get_chat_model forwards modelDetails to every chat-model constructor so the common path has zero extra network cost. Direct instantiation lazy-resolves via client_settings.get_model_info on the first _skip_sampling call (get_available_models is already class-cached inside the settings layer, so at most one discovery call per process).
  • Observability: each strip logs a WARNING via self.logger when one is configured; silent otherwise.
  • Package: langchain only (uipath-langchain-client 1.9.9 → 1.10.0). Core untouched.

Out of scope: clearing sampling fields that were set at instantiation time (UiPathChat(model="opus47", temperature=0.5)) — the cleanest mechanism for that is still under discussion. Tracked for a follow-up.

Test plan

  • tests/langchain/test_disabled_sampling_params.py — 13 new mocked unit tests (no HTTP): sync/async strip, non-sampling n preserved, pass-through when flag absent/false, warning gated by self.logger, lazy resolution populates and swallows discovery errors, factory forwarding.
  • ruff check && ruff format --check . — green.
  • pyright — 0 errors.
  • pytest tests — 1535 passed, 0 failed.
  • Manual smoke against a real tenant after approval: 
    from uipath_langchain_client import get_chat_model
llm = get_chat_model("anthropic.claude-opus-4-7", temperature=0.5)  # no 400
llm.invoke("hi", temperature=0.3)                                   # no 400

🤖 Generated with Claude Code

@cosminacho @claude
Feat: strip sampling params at invoke time when modelDetails.shouldSk…
3ac80db 
…ipTemperature is true

Reasoning-style models (anthropic.claude-opus-4-7) reject any sampling
parameter and the gateway returns 400. The discovery endpoint already
advertises this via modelDetails.shouldSkipTemperature. We now honor it:

- Add model_details: dict | None on UiPathBaseLLMClient.
- Lazy-resolve it via client_settings.get_model_info on first _skip_sampling
  call (backed by the class-cached discovery response).
- Strip temperature/top_p/top_k/frequency_penalty/presence_penalty/seed/
  logit_bias/logprobs/top_logprobs from invoke kwargs inside UiPathBaseChatModel's
  _generate/_agenerate/_stream/_astream wrappers. n is intentionally not
  treated as a sampling knob.
- Factory forwards modelDetails to every chat-model constructor so the
  common path has zero extra network cost.
- Each strip logs a warning via self.logger when one is configured.

Init-time clearing of already-set instance fields (UiPathChat(temperature=0.5))
is intentionally out of scope for this PR — tracked separately.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Refactor: populate model_details in model_post_init instead of lazy o…
22227fa 
…n first invoke

Direct instantiation (UiPathChat(model="opus47")) now resolves
model_details eagerly during pydantic's post-init hook. get_available_models
is class-cached inside the settings layer, so at most one discovery HTTP
call fires per process regardless of how many chat models are built.

Also guards against overwriting an explicitly-provided model_details
(the factory's fast path), and adds a test for that case.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Refactor: extract sampling-strip helpers + move model_details resolut…
b60296a 
…ion to UiPathBaseLLMClient

- New _sampling.py module holds DISABLED_SAMPLING_PARAMS,
  should_skip_sampling, and strip_disabled_sampling_kwargs. Keeps the knowledge
  of which flags mean what in one place instead of spread across wrappers.
- UiPathBaseChatModel's four generate/stream wrappers now delegate to a thin
  _strip_sampling method that calls the module helper.
- model_details resolution moves from UiPathBaseChatModel.model_post_init to a
  @model_validator(mode="after") on UiPathBaseLLMClient, so embedding
  wrappers get the same eager-resolve behavior. get_available_models stays
  class-cached, so this remains at most one network call per process.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Refactor: move sampling helpers to core utils + resolve model_details…
a2c7c6e 
… via field_validator

- New src/uipath/llm_client/utils/sampling.py in the core package exports
  DISABLED_SAMPLING_PARAMS, should_skip_sampling, and
  strip_disabled_sampling_kwargs. These are framework-agnostic and fit the
  existing core utils pattern (one file per concern).
- Langchain's utils.py re-exports them, so the public import path
  uipath_langchain_client.utils.strip_disabled_sampling_kwargs is preserved.
- model_details resolution is now a @field_validator("model_details",
  mode="after") collocated with the field declaration. It reads
  already-validated client_settings and model_name off info.data and calls
  get_model_info, instead of living in a separate @model_validator method.
- Core version 1.9.9 -> 1.10.0 with changelog entry; langchain's core-dep
  floor bumped to >=1.10.0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Nit: tighten the model_details field_validator
b944390 
Keep field_validator (default_factory with data arg breaks LangChain's
internal zero-arg call at langchain_core/utils/pydantic.py). Rename the
method to _resolve and inline the fetch so the validator body is one
short try/except block.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Revert to @model_validator for model_details + inline strip_disabled_…
b074e1c 
…sampling_kwargs calls

- Switch back from @field_validator to @model_validator(mode="after") so
  the resolver uses typed self.* attributes instead of untyped
  info.data[...] lookups.
- Drop the _strip_sampling helper method on UiPathBaseChatModel; call
  strip_disabled_sampling_kwargs directly in _generate/_agenerate/_stream/
  _astream since there's no shared computation to hoist.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Drop sampling + model_family re-exports from langchain utils.py; impo…
df1c6e2 
…rt from core directly

Callers now import from the core modules where the code actually lives
(uipath.llm_client.utils.sampling, uipath.llm_client.utils.model_family)
rather than going through the langchain utils.py re-export layer. The
re-export for these two was noise — they weren't langchain-specific and
the indirection hid the real source.

Exceptions and RetryConfig still re-export through langchain utils.py
because they're widely re-used across many langchain client files.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Chore: drop from __future__ import annotations across the repo
f0ff54b 
Python 3.11+ is the minimum, so the postponed-evaluation import is noise.
Three files had it; none used TYPE_CHECKING-only imports that depended on
lazy annotations, so removing it is safe. pyright, ruff, and pytest all
remain green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Generalize via disabled_params + mode="before" validator (init-time c…
21ea27b 
…learing, langchain-openai compatible)

Replace the sampling-specific interface with the langchain-openai
disabled_params convention:

- Core utils: rename strip_disabled_sampling_kwargs -> strip_disabled_kwargs
  (operates on {name: None | [values]} spec, same as langchain-openai).
  Add disabled_params_from_model_details to derive the spec from a
  modelDetails dict (today: shouldSkipTemperature = the full sampling set).
  Add is_disabled_value helper.
- UiPathBaseLLMClient: add disabled_params field; swap the
  @model_validator(mode="after") for a single @model_validator(mode="before")
  that (1) resolves model_details (caller-provided wins, else
  client_settings.get_model_info), (2) derives disabled_params when the
  caller didn't pass one, (3) pops matching keys from values BEFORE pydantic
  field validation. This is the same pattern langchain-openai uses for
  o1/GPT-5 temperature at base.py:1093-1123, so init-time clearing happens
  without any __pydantic_fields_set__ hackery — the field simply never
  enters model_fields_set.
- Runtime stripping in UiPathBaseChatModel's four wrappers delegates to
  strip_disabled_kwargs(disabled_params=self.disabled_params, ...).
- Drop the unused disabled_params field declaration on UiPathChat (now
  inherited from UiPathBaseLLMClient).
- Tests cover: init-time clearing (fields never enter model_fields_set,
  popped from model_kwargs too), user-provided disabled_params wins over
  derivation, disabled_params value-list semantics, factory forwarding,
  discovery fallback, swallowed errors, and logger-gated warnings.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Switch disabled_params validator from mode=before to mode=after
a73fb7d 
Same observable behavior; the after version is ~9 LOC shorter and uses
typed self.* access (self.client_settings.get_model_info(self.model_name,
byo_connection_id=self.byo_connection_id)) instead of the mode=before
raw-dict alias juggling. The clear step uses object.__setattr__ +
self.__pydantic_fields_set__.discard — one line per cleared field —
which keeps UiPathChat._default_params's model_fields_set filter happy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Trim validator to metadata resolution only; merge user-provided disab…
1ca32c9 
…led_params

- Rename the hook from _resolve_disabled_params to _finalize_model_metadata
  to reflect that it owns both model_details and disabled_params resolution
  (not just the disabled_params side).
- Drop the field-clearing (self.temperature = None +
  __pydantic_fields_set__.discard) and model_kwargs-clearing steps. The
  validator is now purely data: resolve model_details, derive
  disabled_params, merge with caller-provided (user keys win).
- Runtime invoke-time stripping still handles .invoke(temperature=...).
  Init-time values set on the instance (UiPathChat(..., temperature=0.5))
  remain in model_fields_set and flow into _default_params — tracked as a
  follow-up in the langchain CHANGELOG.
- Tests swap init-time-clearing assertions for merge-semantics cases:
  derived ∪ user, narrowing-override, no-derivation fallback.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Rename metadata hook to setup_model_info
283819e 
Matches the existing setup_* convention on the other validators
(setup_uipath_client, setup_api_flavor_and_version) and ties the name
to the get_model_info call it consumes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cosminacho and others added 2 commits April 23, 2026 21:06
@cosminacho @claude
Add OpenAI / Azure interop tests for disabled_params merging
62a6a0f 
Pins down how our setup_model_info validator composes with
langchain-openai's native disabled_params conventions:

- UiPathChatOpenAI derives disabled_params from model_details
  (gateway shouldSkipTemperature) like our normalized UiPathChat does.
- User-supplied disabled_params merge with the derived set; user keys
  win on conflict (e.g. narrowing temperature to a value-list spec).
- UiPathAzureChatOpenAI's native auto-init of
  {"parallel_tool_calls": None} (for non-gpt-4o models) runs BEFORE our
  validator in MRO order. Our merge then treats that as a
  caller-provided value and preserves it alongside the derived sampling
  set. Neither convention is lost.
- End-to-end: runtime .invoke() strip on UiPathChatOpenAI honors the
  merged set — drops both derived sampling kwargs AND
  user-supplied parallel_tool_calls.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho @claude
Nit: use settings= (pydantic alias) instead of client_settings= in ne…
5d026ef 
…w tests

Pyright can't see the field-name alias on UiPath*ChatOpenAI constructor
calls — it only sees the declared alias "settings". Switch the six new
OpenAI interop tests to settings= to match what pyright expects. Runtime
behavior is identical (validate_by_name + validate_by_alias both on).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@cosminacho cosminacho merged commit a9cffaf into main Apr 23, 2026
8 checks passed
@cosminacho cosminacho deleted the fix/skip-disabled-sampling-params branch April 23, 2026 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DragosBobolea DragosBobolea Awaiting requested review from DragosBobolea DragosBobolea is a code owner

@cristipufu cristipufu Awaiting requested review from cristipufu cristipufu is a code owner

@ionmincu ionmincu Awaiting requested review from ionmincu ionmincu is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cosminacho