feat(provider): keep reasoning effort pass-through consistent across OpenAI and Anthropic providers#285
Conversation
…VIDIA#283) Signed-off-by: Rod Boev <rod.boev@gmail.com>
|
Could you please expand this PR to support the native Anthropic provider paths as part of this PR, rather than leaving them out of scope?
Could we translate the common SkillSpector setting into the native Anthropic request format in this PR?
Otherwise, the same environment variable silently has no effect when users switch from an OpenAI-compatible provider to a native Anthropic provider. References: |
|
FYI, I edited the comment: Let's remove Bedrock from scope for this PR since it seems Bedrock has a wide range of such parameter names including We can come back to this support in a followup PR or future work based on community feedback. @rodboev Do you agree with that approach? |
keshprad
left a comment
There was a problem hiding this comment.
Requesting changes to include native Anthropic support in this PR. Please map SKILLSPECTOR_REASONING_EFFORT for both AnthropicProvider and AnthropicProxyProvider, validate the Anthropic-specific accepted values at the provider boundary, and add coverage confirming that the proxy rewrite preserves output_config.effort. See the linked conversation comment for the detailed rationale and references.
…hs (NVIDIA#283) Signed-off-by: Rod Boev <rod.boev@gmail.com>
|
Agreed on keeping Bedrock out of this PR. I limited the expansion to the two native Anthropic paths covered by the formal review.
I left Bedrock parsing, translation, tests, and documentation out of this PR. Its provider-specific reasoning controls can be handled separately if that support is requested later. CLI provider behavior also stays outside this change. |
|
[P1] Could we remove the Anthropic-specific validation and keep The behavior should be consistent across providers:
Maintaining an Anthropic-specific allowlist in SkillSpector creates an unnecessary capability matrix that can become stale as models and supported effort levels evolve. It is also inconsistent with the OpenAI-compatible path, which already passes non-empty values through. Please remove the local Anthropic enum validation and its invalid-value tests. Retain coverage for trimming, pass-through, omission when unset or blank, and preservation of |
|
[P2] Add the new variable to the
Add the variable to that fixture’s cleanup tuple. |
|
Two small changes and this looks good to me. Thanks @rodboev! |
|
I'll make I'll remove the Anthropic accepted-value and invalid-value tests, keep the trimming and omission coverage, keep the proxy preservation check, and add I'll also remove the Anthropic-specific support wording from the docs and PR description. Bedrock stays out of this PR. |
Summary
SkillSpector now treats
SKILLSPECTOR_REASONING_EFFORTas one provider-neutral pass-through setting across its OpenAI-compatible and native Anthropic provider paths. Non-empty values are trimmed and forwarded unchanged, and unset or blank input preserves provider defaults.Closes #283
Implements the maintainer-requested Anthropic expansion from the July 20 review.
Root cause
The shared OpenAI-compatible constructor already forwarded
SKILLSPECTOR_REASONING_EFFORT, but the native Anthropic providers used separate constructor paths and the previous rework added an Anthropic-only validation branch. That made the same environment variable behave differently by provider, andtest_llm_utilsalso failed to clear the new setting from inherited process state.Diff Notes
SKILLSPECTOR_REASONING_EFFORToutput_config.effortSKILLSPECTOR_REASONING_EFFORTto thetest_llm_utilsenvironment cleanupScope
This change covers the shared OpenAI-compatible path plus the two native Anthropic providers. SkillSpector does not validate provider- or model-specific support for non-empty values locally. Bedrock and CLI-provider behavior stay out of scope for this PR.
Verification
uv sync --all-extraspassed; checked 151 packages.python -m pytest tests/unit/test_providers.py tests/unit/test_anthropic_proxy_provider.py; pytest reported98 passed, 9 skipped.python -m pytest tests/unit/test_llm_utils.py::TestCredentialResolution::test_get_chat_model_returns_native_anthropic_client; pytest reported1 passed.uv run ruff check src/skillspector/providers/chat_models.py src/skillspector/providers/anthropic/provider.py src/skillspector/providers/anthropic_proxy/provider.py tests/unit/test_providers.py tests/unit/test_anthropic_proxy_provider.py tests/unit/test_llm_utils.pypassed withAll checks passed!.uv run ruff format --check src/skillspector/providers/chat_models.py src/skillspector/providers/anthropic/provider.py src/skillspector/providers/anthropic_proxy/provider.py tests/unit/test_providers.py tests/unit/test_anthropic_proxy_provider.py tests/unit/test_llm_utils.pypassed with6 files already formatted.