Skip to content

feat(providers): add LLM sampling controls - #427

Open
deepujain wants to merge 1 commit into
NVIDIA:mainfrom
deepujain:feat/334-llm-temperature
Open

feat(providers): add LLM sampling controls#427
deepujain wants to merge 1 commit into
NVIDIA:mainfrom
deepujain:feat/334-llm-temperature

Conversation

@deepujain

Copy link
Copy Markdown
Contributor

Summary

  • add optional SKILLSPECTOR_TEMPERATURE sampling control for hosted providers
  • add optional SKILLSPECTOR_SEED forwarding for OpenAI-compatible and Azure OpenAI providers
  • validate configured values before model construction and preserve provider defaults when unset or blank
  • document provider support and the limits of reproducibility controls

This addresses the sampling-control portion of #334. Provider/model attribution in generated reports is intentionally unchanged because that requires a separate report-schema decision.

Validation

  • pytest tests/unit/test_providers.py tests/unit/test_anthropic_proxy_provider.py tests/unit/test_new_providers.py tests/unit/test_bedrock_provider.py tests/unit/test_constants.py -q — 176 passed, 9 skipped
  • pytest -m 'not integration and not provider' tests/ -q — 2804 passed, 13 skipped, 38 deselected, 4 xfailed
  • ruff check src tests
  • ruff format --check src tests
  • git diff --check

Risk

  • unset or blank controls preserve existing provider-default request behavior
  • temperature is constrained to the portable 0..1 range; seed support remains provider/model dependent
  • Anthropic, Anthropic proxy, and Bedrock receive temperature but never the OpenAI-specific seed
  • CLI providers are unchanged because their subprocess interfaces own sampling behavior

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]\n\nApproved. Sampling controls are validated centrally and mapped appropriately across OpenAI-compatible, Azure, Anthropic, Bedrock, and proxy providers, with unsupported seed parameters omitted where required. Current required checks pass.

@deepujain
deepujain force-pushed the feat/334-llm-temperature branch 2 times, most recently from 6148d41 to ccae0e7 Compare August 24, 2026 18:17
@rng1995
rng1995 enabled auto-merge (squash) August 24, 2026 18:29
Signed-off-by: Deepak Jain <deepujain@gmail.com>
auto-merge was automatically disabled August 24, 2026 18:53

Head branch was pushed to by a user without write access

@deepujain
deepujain force-pushed the feat/334-llm-temperature branch from bb495fe to 705bc1d Compare August 24, 2026 18:53
@deepujain

Copy link
Copy Markdown
Contributor Author

Resolved the conflict with current main by retaining both the merged output-language configuration and this PR’s sampling controls. Rebased to signed head 705bc1d; focused provider tests passed (176 passed, 9 skipped), the contributor suite passed (2,833 passed, 13 skipped, 38 deselected, 4 xfailed), and repository-wide Ruff checks are clean. GitHub checks are rerunning; ready for re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants