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

Pr branch #2312

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

Pr branch #2312

wants to merge 5 commits into from

Conversation

Vidit-Ostwal
Copy link
Contributor

Fixes #2307

Build on top of #2309

Mainly we need to handle both type of crew initialization,

Direct instance for example

### Handles when crew is direclty initialized 
### my_crew = Crew(...)  # A direct instance of Crew

Or function or class which throws a crew object

### Handles when a function or a class throws back a crew instance
# # Function with a crew method attached
# def get_my_crew():
#     pass
# get_my_crew.crew = lambda: Crew(...)

# # OR

# # Class with a crew method
# class CrewFactory:
#     @staticmethod
#     def crew():
#         return Crew(...)
        
# my_factory = CrewFactory

caike and others added 4 commits March 7, 2025 11:42
Changes back to how it was being done before.
Fixes crewAIInc#2307
When not initiated, the function should raise
the "memory system is not initialized" RuntimeError.
@joaomdmoura
Copy link
Collaborator

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

Code Review for CrewAI PR #2312

Overview

This PR introduces significant changes focusing on crew object handling, improvements to the memory system, and enhancements to CLI utilities. Below are detailed insights and recommendations based on the review of modified files.

1. File: src/crewai/cli/utils.py

Key Changes

  • Modified crew object detection logic
  • Enhanced handling for various crew initialization patterns
  • Adjusted logging statements for clarity

Findings

  1. Inconsistent Error Handling: The error handling implemented is quite generic; this can potentially hide specific issues during runtime.
  2. Redundant Print Statements: The use of print statements is inconsistent and could benefit from a more structured logging approach.
  3. Missing Type Hints: Several function parameters lack appropriate type annotations, hindering readability and maintainability.

Recommendations

  • Error Handling: Refine your exception handling by targeting specific exceptions rather than using a broad catch-all approach. This aids in diagnosing issues effectively.
try:
    if callable(attr) and hasattr(attr, "crew"):
        crew_instance = attr().crew()
        return crew_instance
except AttributeError as e:
    logger.warning(f"Invalid crew attribute structure: {e}")
except TypeError as e:
    logger.warning(f"Crew instantiation error: {e}")
except Exception as e:
    logger.error(f"Unexpected error processing crew: {e}")
  • Logging Enhancements: Replace print statements with a structured logging system. This unifies logging practices across the codebase.
import logging

logger = logging.getLogger(__name__)

def get_crew(crew_path: str = "crew.py", require: bool = False) -> Crew | None:
    logger.info(f"Searching for crew in {crew_path}")
    # ... rest of the code

2. File: src/crewai/crew.py

Key Changes

  • Improvements to memory system initialization checks have been made
  • The access patterns for memory systems were modified

Findings

  1. Memory System Access: The implementation could be fortified with stricter initialization checks to ensure reliability.
  2. Insufficient Documentation: The lack of documentation around memory type validation could lead to confusion regarding acceptable inputs.

Recommendations

  • Robust Validation and Documentation: Enhance validation for memory systems and thoroughly document expected behaviors, raising specific exceptions to clarify errors.
class Crew:
    def _reset_specific_memory(self, memory_type: str) -> None:
        """
        Reset a specific memory system.
        
        Args:
            memory_type: Type of memory to reset ('long', 'short', 'entity', 'knowledge', 'kickoff_outputs')
            
        Raises:
            ValueError: If invalid memory_type provided
            RuntimeError: If memory system not initialized
        """
        # ... existing logic ...

General Improvement Insights

  1. Documentation: Comprehensive docstrings should be added to all new and modified functions, including usage examples and expected exceptions.
  2. Type Safety: Implement stricter type checking throughout the code and add runtime validation for crucial parameters.
  3. Testing: Introduce unit tests specifically for new crew detection logic and edge cases around memory system functions.
  4. Code Organization: Consider modularizing the crew detection logic into a dedicated utility class to improve code clarity and separation of concerns.

Summary of Necessary Enhancements

  1. Transition from print statements to a unified logging strategy.
  2. Implement detailed error handling with specific exceptions.
  3. Add type hints and validation comprehensively.
  4. Enrich documentation across modified files.
  5. Ensure unit tests cover new functionalities and edge cases.

These enhancements will significantly improve the maintainability, reliability, and debugging capabilities of the code, preserving its essential functionality.


The above analysis synthesizes critical reviews of the modified files in PR #2312. Enhancements in logging, error handling, and documentation practices will fortify the codebase against potential pitfalls and ensure a more robust structure moving forward.

Copy link
Collaborator

@bhancockio bhancockio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Vidit-Ostwal if you could show an example of this working and then address the comments below, that would be awesome!

# my_factory = CrewFactory
if callable(attr) and hasattr(attr, "crew"):
print(
f"Found valid crew function in attribute '{attr_name}' at {crew_os_path}."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't usually like to add print statements unless they are necessary.

@@ -273,6 +273,30 @@ def get_crew(crew_path: str = "crew.py", require: bool = False) -> Crew | None:
for attr_name in dir(module):
attr = getattr(module, attr_name)
try:
### Handles when a function or a class throws back a crew instance
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't usually add this many comments and it looks like the comments are double nested or something.

"entity": (self._entity_memory, "entity"),
"knowledge": (self.knowledge, "knowledge"),
"kickoff_outputs": (self._task_output_handler, "task output"),
"long": (getattr(self, "_long_term_memory", None), "long term"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! Good fix!

@Vidit-Ostwal
Copy link
Contributor Author

Sure @bhancockio , will look into these.

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] errors on "crewai reset-memories" CLI command
4 participants