Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Handle no-crew case in reset-memories command #2124

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

devin-ai-integration[bot]
Copy link
Contributor

Fixes #2123

Description

The reset-memories command now handles the case when no crew exists gracefully. This fixes the error 'No crew found' when running 'crewai reset-memories -a'.

Changes

  • Modified RAGStorage to handle initialization without crew
  • Added test for resetting memories without crew

Testing

  • Added new test case for resetting memories without crew
  • Verified existing tests pass
  • Manually tested reset-memories command

Link to Devin run: https://app.devin.ai/sessions/afb204a782ad4c16987e7240a967a803
Requested by: Joe

- Modify RAGStorage to handle initialization without crew
- Add test for resetting memories without crew
- Fixes #2123

Co-Authored-By: Joe Moura <[email protected]>
Copy link
Contributor Author

🤖 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
Copy link
Collaborator

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

Code Review Comment for PR #2124

Overview

This PR aims to enhance the reset-memories command by introducing functionality for scenarios where no crew exists, along with additional test coverage and improvements in the RAG storage initialization. Below are some observations and recommendations based on the modifications in three primary files.

1. src/crewai/cli/reset_memories_command.py

Positive Aspects:

  • Excellent error handling structure that gracefully manages different states of crew existence.
  • Clear separation of concerns, providing a structured approach to handle the crew vs. no-crew scenarios.
  • Comprehensive tests covering all memory types, ensuring robust functionality.

Suggested Improvements:

  1. Import Organization
    Move the imports to the top level to enhance readability and follow Python conventions.

    from crewai.memory.short_term.short_term_memory import ShortTermMemory
    from crewai.memory.entity.entity_memory import EntityMemory
    from crewai.memory.long_term.long_term_memory import LongTermMemory
    from crewai.utilities.task_output_storage_handler import TaskOutputStorageHandler
    from crewai.knowledge.storage.knowledge_storage import KnowledgeStorage
  2. DRY Principle Violation
    To avoid redundancy, create a shared helper function for resetting memories.

    def _reset_all_memories():
        ShortTermMemory().reset()
        EntityMemory().reset()
        LongTermMemory().reset()
        TaskOutputStorageHandler().reset()
        KnowledgeStorage().reset()

2. src/crewai/memory/storage/rag_storage.py

Positive Aspects:

  • Implements effective handling of the no-crew scenario with fallback logic.
  • Code structure is clean and modular, making use of helper methods effectively.

Suggested Improvements:

  1. Code Organization
    Extract the agent name processing into a dedicated method for better modularity.

    def _get_agents_string(self, crew):
        return "no_crew" if not crew else "_".join([self._sanitize_role(agent.role) for agent in crew.agents])
  2. Documentation
    Add docstrings to clarify the behavior, particularly in no-crew scenarios.

    class RAGStorage(BaseStorage):
        """
        RAG Storage implementation that handles both crew and no-crew scenarios.
        
        Args:
            type: Type of storage
            allow_reset: Whether storage can be reset
            embedder_config: Configuration for embeddings
            crew: Crew instance or None for no-crew scenario
            path: Custom storage path
        """

3. tests/cli/cli_test.py

Positive Aspects:

  • Demonstrates comprehensive coverage for testing.
  • Clear structure and effective use of mocking.

Suggested Improvements:

  1. Test Organization
    Group related test cases into a test class for clarity.

    class TestResetMemoriesCommand:
        @mock.patch("crewai.cli.reset_memories_command.get_crew")
        def test_reset_all_memories(self, mock_get_crew, runner):
            ...
  2. Test Documentation
    Adding docstrings to the tests can improve readability and understanding.

    def test_reset_all_memories_no_crew(self, *mocks):
        """
        Validate that resetting all memories correctly functions when no crew exists.
        Should reset all memory types individually.
        """

General Recommendations:

  • Error Handling: Improve error reporting and consider adding logging for better debugging.
  • Type Hints: Introduce type hints to enhance code readability and maintenance.
  • Documentation: Expand inline comments and update README to reflect new functionalities accurately.
  • Testing: Consider testing edge cases and adding integration tests to broaden coverage.

Conclusion

The implementation is solid and addresses the core requirements effectively. The proposed improvements focus on enhancing code quality, organization, and documentation. Once these changes are made, the PR should be ready for merging.

devin-ai-integration bot and others added 2 commits February 13, 2025 19:10
- Move imports to top level
- Add helper function for resetting memories
- Add type hints and docstrings
- Improve test organization

Co-Authored-By: Joe Moura <[email protected]>
- Add Logger class usage
- Add descriptive error messages
- Add edge case test for memory reset failure
- Add docstrings to RAGStorage methods

Co-Authored-By: Joe Moura <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] crewai reset-memories -a throws an error
1 participant