Add support for default llm kwargs for all models. #1303
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
These kwargs can be passed to the constructor of the LLM object and will then be used for each generate call. Arguments passed via the generate_* methods will override the defaults. The integ tests verify that the default arguments do in fact get applied by greatly restricting the max output tokens.
There was a problem hiding this comment.
Pull Request Overview
Adds support for default LLM keyword arguments that are merged with per-call overrides across all model implementations.
- Introduces
default_llm_kwargsparameter in the baseLLMclass and all subclasses. - Implements
_merge_llm_kwargsto combine defaults with method-specific kwargs. - Updates all
generate,generate_async, andgenerate_batchmethods to merge defaults before invoking the provider, and adds unit/integration tests to verify behavior.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/sycamore/sycamore/llms/llms.py | Added default_llm_kwargs to base LLM and _merge_llm_kwargs helper. |
| lib/sycamore/sycamore/llms/openai.py | Passed default_llm_kwargs through constructor and merged in all generate methods. |
| lib/sycamore/sycamore/llms/gemini.py | Added default_llm_kwargs support and merged defaults in metadata, generate, and async methods. |
| lib/sycamore/sycamore/llms/bedrock.py | Added default_llm_kwargs support and merged defaults in metadata method. |
| lib/sycamore/sycamore/llms/anthropic.py | Added default_llm_kwargs support and merged defaults in metadata, generate, and batch methods. |
| lib/sycamore/sycamore/tests/unit/transforms/test_base_llm.py | Extended fake LLM in tests to capture merged kwargs. |
| lib/sycamore/sycamore/tests/unit/llms/test_llms.py | New test for _merge_llm_kwargs. |
| lib/sycamore/sycamore/tests/integration/llms/*.py | Integration tests for each provider to assert max_tokens (or provider equivalent) are enforced. |
Comments suppressed due to low confidence (4)
lib/sycamore/sycamore/llms/bedrock.py:63
- [nitpick] Remove this
printor replace it with alogging.debug/logging.infocall to avoid side effects in library code.
print(f"cache return {ret}")
lib/sycamore/sycamore/llms/llms.py:26
- The class docstring should be updated to document the new
default_llm_kwargsconstructor parameter and describe its behavior.
"""Abstract representation of an LLM instance. and should be subclassed to implement specific LLM providers."""
lib/sycamore/sycamore/llms/openai.py:16
- Add
default_llm_kwargsto theArgs:section of the class docstring so consumers know they can pass default call parameters.
"""An LLM interface to OpenAI models."""
lib/sycamore/sycamore/tests/unit/llms/test_llms.py:62
- [nitpick] Consider adding a corresponding unit test for the
generate_asyncpath (usingFakeLLM.generate_async) to ensure defaults are merged there as well.
def test_merge_llm_kwargs():
| async def generate_async(self, *, prompt: RenderedPrompt, llm_kwargs: Optional[dict] = None) -> str: | ||
| from anthropic import RateLimitError, APIConnectionError | ||
|
|
||
| self._merge_llm_kwargs(llm_kwargs) |
There was a problem hiding this comment.
In generate_async, _merge_llm_kwargs is called but its return value is not assigned back to llm_kwargs, so default arguments are never applied. It should be llm_kwargs = self._merge_llm_kwargs(llm_kwargs).
Suggested change
| self._merge_llm_kwargs(llm_kwargs) | |
| llm_kwargs = self._merge_llm_kwargs(llm_kwargs) |
Copilot uses AI. Check for mistakes.
HenryL27
approved these changes
May 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
These kwargs can be passed to the constructor of the LLM object and will then be used for each generate call. Arguments passed via the generate_* methods will override the defaults.
The integ tests verify that the default arguments do in fact get applied by greatly restricting the max output tokens.