Simplify max_output_tokens handling in LLM classes #9296
Merged
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
- Remove complex default-setting logic in init_model_info() - Keep max_output_tokens as None if not explicitly set - Use conditional parameter inclusion in completion calls - Update tests to expect None values instead of computed defaults This simplifies the codebase by removing unnecessary complexity in determining max_output_tokens defaults.
Remove unnecessary conditional dictionary building. Since litellm.completion accepts Optional[int] = None for both max_completion_tokens and max_tokens parameters, we can simply pass None directly instead of conditionally including the parameters. This makes the code much cleaner and easier to understand.
The code is self-explanatory and doesn't need verbose comments.
The assertions are clear without explanatory comments.
1 task
enyst
reviewed
Jun 23, 2025
| model in self.config.model | ||
| for model in ['claude-3-7-sonnet', 'claude-3.7-sonnet'] | ||
| ): | ||
| self.config.max_output_tokens = 64000 # litellm set max to 128k, but that requires a header to be set |
There was a problem hiding this comment.
I think at least this one needs a quick test with 3.7. I’m not at my computer, but I can do that in a bit
There was a problem hiding this comment.
I confirm it works well. I think this was a litellm issue at some point in time, it must have been fixed, and we forgot this code here.
|
Re: the complex logic here. |
enyst
approved these changes
Jun 23, 2025
|
Thank you! |
xingyaoww
added a commit
that referenced
this pull request
Jun 25, 2025
This reverts commit 1e33624.
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
This PR simplifies the handling of
max_output_tokensin OpenHands LLM classes by removing complex default-setting logic and keeping the value asNonewhen not explicitly set.Changes Made
Core Changes
init_model_info()that attempted to determine default values formax_output_tokensbased on various conditionsmax_output_tokensisNone, it remainsNoneinstead of being set to computed defaultsmax_output_tokensonly when it's notNoneFiles Modified
openhands/llm/llm.py: Removed lines 489-507 containing complex default determination logic, added conditional parameter inclusionopenhands/llm/async_llm.py: Added conditionalmax_tokensparameter inclusionopenhands/llm/streaming_llm.py: Added conditionalmax_tokensparameter inclusiontests/unit/test_llm.py: Updated test expectations to expectNonevalues instead of computed defaultsTesting
Benefits
max_output_tokensis not setBackward Compatibility
This change maintains backward compatibility - the LLM providers will use their own defaults when
max_output_tokensis not provided, which is the expected behavior.References
max_output_tokenshandlingNoneasNoneinstead of complex default computationFixes #9227
@neubig can click here to continue refining the PR
To run this PR locally, use the following command: