Fix #2740: Prevent IndexError in ollama_pt() with empty messages

[devin-ai-integration]

Fix for Issue #2740: Prevent IndexError in ollama_pt() with empty messages

Description

This PR fixes an IndexError that occurs in LiteLLM's ollama_pt() function when an empty messages list is passed from CrewAI.

The issue happens because the ollama_pt() function tries to access elements in the messages list without first checking if the list is empty or properly structured. This PR adds validation in CrewAI's LLM.call() method to ensure that messages lists are not empty before passing them to LiteLLM.

Changes

• Added validation in LLM.call() to check for empty messages lists and None values before event emission
• Added additional validation in _prepare_completion_params() as a safeguard
• Added tests to verify that the validation works correctly for both empty lists and None values

Testing

• Added new tests in tests/test_empty_messages.py to verify that the validation works correctly
• All tests for our specific fix pass

Link to Devin run

https://app.devin.ai/sessions/9ee9a316a71e402d9f51f79cc5c871d1

Requested by: Joe Moura ([email protected])

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[joaomdmoura]

Disclaimer: This review was made by a crew of AI Agents.

Code Review for PR #2741

Overview

This pull request addresses issue #2740 by adding validation to prevent IndexError in the ollama_pt() function when handling empty message lists in LiteLLM. Changes have been made to two files: src/crewai/llm.py and tests/test_empty_messages.py.

Changes in src/crewai/llm.py

Positive Aspects:

• Input Validation: The implementation of input validation checks against empty messages in _prepare_completion_params and call methods is a significant improvement for the robustness of the system.
• Clear Error Messages: The validation process provides informative error messages that will assist any developer using the function.

Suggested Improvements:

1. Code Duplication:

   • The same validation logic appears in both methods. To reduce redundancy and improve maintainability, consider extracting the validation into a private helper method as follows:

   def _validate_messages(self, messages):
       """Validate that messages list is not empty."""
       if messages is None or (isinstance(messages, list) and len(messages) == 0):
           raise ValueError("Messages list cannot be empty. At least one message is required.")

2. Type Hints:

   • Type hints are absent. Adding them can enhance readability and error-checking capabilities:

   def _prepare_completion_params(
       self,
       messages: Union[str, List[Dict[str, str]]],
       **kwargs: Any
   ) -> Dict[str, Any]:

3. Comment Numbering:

   • Update comment numbering in your code to accurately reflect the validation check, perhaps using more descriptive section names for better clarity.

Changes in tests/test_empty_messages.py

Positive Aspects:

• Test Coverage: There is good coverage testing both empty and None message scenarios.
• Clarity: The test function names and docstrings provide clear insights into the intentions of each test case.

Suggested Improvements:

1. Enhancing Test Cases:

   • Expand the test cases to cover potential edge scenarios:

   def test_edge_cases_messages_validation():
       llm = LLM(model="gpt-3.5-turbo")
       
       # Test empty string message
       with pytest.raises(ValueError, match="Messages list cannot be empty"):
           llm.call(messages="")
       
       # Test list with empty dict
       with pytest.raises(ValueError, match="Messages list cannot be empty"):
           llm.call(messages=[{}])

2. Mock Testing for Event Emission:

   • Mock tests that validate event emissions with invalid inputs could enhance the testing coverage:

   def test_event_emission_with_empty_messages():
       llm = LLM(model="gpt-3.5-turbo")
       
       with patch('crewai.llm.crewai_event_bus.emit') as mock_emit:
           with pytest.raises(ValueError):
               llm.call(messages=[])
           
           # Ensure no event was emitted for invalid input
           mock_emit.assert_not_called()

General Recommendations:

1. Custom Exception Class:

   • Introduce a custom exception for better clarity in error handling:

   class EmptyMessagesError(ValueError):
       """Raised when the messages list is empty or None."""
       pass

2. Documentation Updates:

   • Ensure all relevant methods have updated docstrings that accurately describe the input and exceptions that can arise, as follows:

   def call(self, messages: Union[str, List[Dict[str, str]]], **kwargs: Any) -> str:
       """
       Call the LLM with the given messages.
       Args:
           messages: A string or list of message dictionaries. Cannot be empty or None.
           **kwargs: Additional arguments for the LLM call.
       Raises:
           ValueError: If messages are empty or None
           ...
       """

3. Add Logging:

   • Logging for validation failures can aid debugging:

   import logging
   
   logger = logging.getLogger(__name__)
   
   def _validate_messages(self, messages):
       if messages is None or (isinstance(messages, list) and len(messages) == 0):
           logger.debug("Message validation failed: empty messages list")
           raise ValueError("Messages list cannot be empty. At least one message is required.")

Summary

Overall, the changes effectively address the reported IndexError issue while enhancing input validation significantly. The code quality is strong but could be improved by implementing the suggested refactoring and testing strategies. This will ensure a more maintainable, robust, and error-resistant codebase as the project evolves.

[devin-ai-integration]

Closing due to inactivity for more than 7 days.