Closed SmartManoj closed 3 months ago
@CodiumAI-Agent /review
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪ |
🧪 No relevant tests |
🔒 No security concerns identified |
⚡ Key issues to review Hardcoded Dependency The code assumes the presence of specific attributes (`max_input_tokens` and `max_output_tokens`) in the `config` object only when the model starts with 'ollama'. This could lead to issues if the config does not include these attributes or if the model naming convention changes. Missing Validation There is no validation to check if `max_input_tokens` or `max_output_tokens` are integers or within a valid range before using them in the calculation. This could lead to runtime errors or incorrect configurations. |
@CodiumAI-Agent /ask
This could lead to issues if the config does not include these attributes or if the model naming convention changes.
it ensures its values.
There is no validation to check if
max_input_tokens
ormax_output_tokens
are integers
mypy ensures that a range is not needed here.
This could lead to issues if the config does not include these attributes or if the model naming convention changes. it ensures its values. > There is no validation to check if
max_input_tokens
ormax_output_tokens
are integers mypy ensures that a range is not needed here.
The concerns raised in the PR questions are valid. Let's address them one by one:
Missing Attributes in Config:
max_input_tokens
or max_output_tokens
are not present in the configuration, the code will raise an AttributeError
.Model Naming Convention:
self.config.model.startswith('ollama')
might not work as intended.Validation of max_input_tokens
and max_output_tokens
:
max_input_tokens
and max_output_tokens
are integers.Here is an improved version of the code with these considerations:
if self.config.model.startswith('ollama'):
max_input_tokens = getattr(self.config, 'max_input_tokens', None)
max_output_tokens = getattr(self.config, 'max_output_tokens', None)
if max_input_tokens is not None and max_output_tokens is not None:
if isinstance(max_input_tokens, int) and isinstance(max_output_tokens, int):
litellm.OllamaConfig.num_ctx = max_input_tokens + max_output_tokens
else:
raise ValueError("max_input_tokens and max_output_tokens must be integers")
else:
raise AttributeError("max_input_tokens and max_output_tokens must be defined in the config")
getattr(self.config, 'max_input_tokens', None)
and getattr(self.config, 'max_output_tokens', None)
are used to safely retrieve the attributes. If they do not exist, None
is returned.isinstance(max_input_tokens, int) and isinstance(max_output_tokens, int)
ensures that both values are integers.This approach ensures that the code is more robust and less prone to runtime errors.
https://github.com/OpenDevin/OpenDevin/pull/2927
fixes https://github.com/OpenDevin/OpenDevin/issues/2924
fixes https://github.com/OpenDevin/OpenDevin/issues/3287