tests: verify LLM forwards all attributes to LiteLLM (exposes missing custom_llm_provider) #981
Conversation
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
…ssing custom_llm_provider) Adds a unit test that patches litellm.completion and asserts both sampling options (temperature/top_p/top_k, max tokens) and transport-level options (base_url, api_version, timeout, drop_params, seed, custom_llm_provider) are forwarded from LLM to LiteLLM. The test currently fails on main due to custom_llm_provider not being passed through. Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
…last Adds tests/sdk/llm/test_llm_responses_attr_forwarding.py which patches litellm.responses and verifies forwarding of transport-level params and responses options. The check for custom_llm_provider (known missing) is placed at the end so earlier regressions are visible first. Co-authored-by: openhands <openhands@all-hands.dev>
…est; make Responses API patch return minimal typed response This reorders the known failing assertion in the chat-completions test and fixes the Responses test to return a minimal ResponsesAPIResponse so other kwargs assertions can execute. Co-authored-by: openhands <openhands@all-hands.dev>
…ures; keep custom_llm_provider assertion last - Use gpt-5-mini to ensure uses_responses_api=True - Include a system message so instructions is non-empty - Leave known failing custom_llm_provider assertion last Co-authored-by: openhands <openhands@all-hands.dev>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
Contributor
|
[Automatic Post]: It has been a while since there was any activity on this PR. @enyst, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Add a unit test that validates all relevant LLM attributes are forwarded to LiteLLM when invoking completion(). The test patches openhands.sdk.llm.llm.litellm_completion and inspects the kwargs it receives.
What it tests
Expected outcome
This test is expected to fail on current main due to custom_llm_provider not being forwarded to LiteLLM (see discussion in PR #963). The failure is intentional to surface the gap.
Why this is useful
This ensures regression coverage so any attributes defined on LLM that should be visible to LiteLLM are actually passed through, not only the ones handled specially in select_chat_options().
Notes
Co-authored-by: openhands openhands@all-hands.dev
@enyst can click here to continue refining the PR