You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The code adds model-specific behavior lists but still has hardcoded model names in some places. For example, line 106-108 still have model-specific adjustments without checking against the new lists.
Some models appear in multiple lists which could lead to maintenance issues if model capabilities change. Consider implementing a more structured approach with model capability flags instead of separate lists.
The local variable stream is modified but never used afterward. This modification has no effect on the function's behavior since the stream parameter is only used to initially set completion_params["stream"].
# Model-specific adjustments
if self.model in self.no_support_streaming_models:
- stream = False
completion_params["stream"] = False
completion_params["max_completion_tokens"] = 2 * self.max_tokens
# completion_params["reasoning_effort"] = "high"
completion_params.pop("max_tokens", None) # Remove 'max_tokens' if present
Apply this suggestion
Suggestion importance[1-10]: 5
__
Why: The suggestion correctly identifies that the local variable stream is modified but never used afterward. Removing this unnecessary assignment improves code cleanliness without affecting functionality. This is a minor improvement to code quality.
✅ Prevent token limit overflowSuggestion Impact:The commit addressed the token limit concern by refactoring how max_tokens is handled. Instead of hardcoding max_tokens in the method parameter, it's now a class attribute that can be set during initialization. This changes how max_completion_tokens is calculated, though it doesn't add the explicit limit check suggested.
code diff:
- def __init__(self, model: str, api_base: str = "", enable_retry=True):+ def __init__(+ self, model: str, api_base: str = "", enable_retry=True, max_tokens=16384+ ):
"""
Initializes an instance of the AICaller class.
@@ -49,19 +51,19 @@
self.model = model
self.api_base = api_base
self.enable_retry = enable_retry
+ self.max_tokens = max_tokens
self.user_message_only_models = USER_MESSAGE_ONLY_MODELS
self.no_support_temperature_models = NO_SUPPORT_TEMPERATURE_MODELS
self.no_support_streaming_models = NO_SUPPORT_STREAMING_MODELS
@conditional_retry # You can access self.enable_retry here
- def call_model(self, prompt: dict, max_tokens=16384, stream=True):+ def call_model(self, prompt: dict, stream=True):
"""
Call the language model with the provided prompt and retrieve the response.
Parameters:
prompt (dict): The prompt to be sent to the language model.
- max_tokens (int, optional): The maximum number of tokens to generate in the response. Defaults to 16384.
stream (bool, optional): Whether to stream the response or not. Defaults to True.
Returns:
@@ -90,7 +92,7 @@
"messages": messages,
"stream": stream, # Use the stream parameter passed to the method
"temperature": 0.2,
- "max_tokens": max_tokens,+ "max_tokens": self.max_tokens,
}
# Remove temperature for models that don't support it
@@ -101,7 +103,7 @@
if self.model in self.no_support_streaming_models:
stream = False
completion_params["stream"] = False
- completion_params["max_completion_tokens"] = 2*max_tokens+ completion_params["max_completion_tokens"] = 2 * self.max_tokens
The code doubles max_tokens for max_completion_tokens without considering model limits. This could exceed model context windows. Add a check to ensure max_completion_tokens doesn't exceed model-specific limits.
if self.model in self.no_support_streaming_models:
stream = False
completion_params["stream"] = False
- completion_params["max_completion_tokens"] = 2*max_tokens+ max_completion = min(2*max_tokens, 32768) # Example limit, adjust per model+ completion_params["max_completion_tokens"] = max_completion
# completion_params["reasoning_effort"] = "high"
completion_params.pop("max_tokens", None) # Remove 'max_tokens' if present
[Suggestion has been applied]
Suggestion importance[1-10]: 8
__
Why: The suggestion addresses a critical issue where doubling max_tokens could exceed model context limits, potentially causing runtime errors. Adding a safety cap is important for preventing failures.
Medium
General
Fix stream parameter inconsistency
The code modifies the stream parameter after it's already been used in completion_params. Move the stream parameter modification before setting completion_params to maintain consistency.
+# Adjust stream parameter based on model support+if self.model in self.no_support_streaming_models:+ stream = False+
completion_params = {
"model": self.model,
"messages": messages,
- "stream": stream, # Use the stream parameter passed to the method+ "stream": stream, # Use the adjusted stream parameter
"temperature": 0.2,
"max_tokens": max_tokens,
}
-# Model-specific adjustments-if self.model in self.no_support_streaming_models:- stream = False- completion_params["stream"] = False-
Suggestion importance[1-10]: 7
__
Why: The suggestion improves code reliability by fixing a logical flow issue where the stream parameter is modified after being used, which could lead to inconsistent behavior in streaming functionality.
Medium
Learned best practice
Add validation for numeric calculations to prevent potential issues with negative or zero values
The calculation of max_completion_tokens should validate that max_tokens is positive to avoid potential issues with negative or zero values. Add a validation check before the multiplication.
KennyDizi
changed the title
RefactoringAICaller class for better using reasoning models
[WIP RefactoringAICaller class for better using reasoning models
Feb 18, 2025
KennyDizi
changed the title
[WIP RefactoringAICaller class for better using reasoning models
[WIP] RefactoringAICaller class for better using reasoning models
Feb 18, 2025
@EmbeddedDevops1 I've adjusted the PR and ran the test command ./tests_integration/test_all.sh; it looks good on my end. However, would you please enable the pipeline for this PR again? Let's see.
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
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.
PR Type
Enhancement, Tests
Description
Introduced model-specific behavior lists to
AICaller.USER_MESSAGE_ONLY_MODELS,NO_SUPPORT_TEMPERATURE_MODELS, andNO_SUPPORT_STREAMING_MODELS.Refactored
call_modelto dynamically adapt parameters based on model capabilities.temperatureorstreamfor certain models.Enhanced test coverage for
AICaller.Added a new module-level initialization for model-specific lists in
cover_agent/__init__.py.Changes walkthrough 📝
AICaller.py
Refactored `AICaller` for model-specific behaviorcover_agent/AICaller.py
call_modelto adapt to model constraints.__init__.py
Added model-specific behavior listscover_agent/init.py
USER_MESSAGE_ONLY_MODELSlist for user-message-only models.NO_SUPPORT_TEMPERATURE_MODELSlist for models withouttemperature support.
NO_SUPPORT_STREAMING_MODELSlist for models without streamingsupport.
test_AICaller.py
Enhanced test coverage for `AICaller`tests/test_AICaller.py