Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

Fixes #2473

Problem

The logger was not working in FastAPI projects after upgrading from version 0.95 to 0.108.

Solution

Modified the Printer class to use sys.stdout.write and sys.stdout.flush instead of print(), which is more compatible with asynchronous environments like FastAPI.

Testing

Added tests to verify the logger works in both synchronous and FastAPI contexts.

Link to Devin run: https://app.devin.ai/sessions/ad473870f1c44e1ea7f4c391e6e35f26
User: Joe Moura (joao@crewai.com)

@devin-ai-integration
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 #2474

Overview

This Pull Request addresses logger functionality problems observed in FastAPI projects after the upgrade to version 0.108. The modifications made to the printer utility and the addition of comprehensive test coverage represent great strides towards ensuring the reliability and maintainability of the logging system.

src/crewai/utilities/printer.py Changes

Improvements:

  • Refactoring: The refactor of print methods to format methods enhances clarity and maintainability.
  • Explicit stdout Handling: Adding explicit handling with sys.stdout improves the robustness of output operations.
  • Code Organization: The restructured code promotes better readability and organization.

Identified Issues:

  1. Code Duplication: Several format strings appear to be repeated across methods, which can lead to maintenance overhead.
  2. Missing Type Hints: Several new methods lack return type hints, which can hinder type checking and IDE support.
  3. Old-Style Formatting: The usage of implicit string formatting (e.g., "%s" % value) is discouraged in favor of more modern approaches.

Recommendations:

  1. Eliminate Duplication with Color Enum: Introduce an Enum for text colors, reducing repeated code patterns:

    from enum import Enum
    
    class Color(Enum):
        PURPLE = "\033[95m"
        RED = "\033[91m"
        # remain colors ...
  2. Add Type Hints to methods to ensure better clarity and type safety:

    def _format_bold_purple(self, content: str) -> str:
        return self._format_text(content, Color.PURPLE, bold=True)
  3. Use f-strings for improved readability and performance when printing:

    def print(self, content: str, color: Optional[str] = None) -> None:
        output = self._format_color(content, color) if color else content
        sys.stdout.write(f"{output}\n")

Tests Overview

New Test Files Analysis

tests/utilities/test_fastapi_logger.py

  • Strengths: Comprehensive integration tests for FastAPI setups ensure that logger functionality adheres to expectations.

  • Suggestions:

    1. Incorporate async test cases to better match FastAPI's architecture.
    2. Include tests for error scenarios to validate robustness.
    @pytest.mark.asyncio
    async def test_async_logger_in_fastapi():
        async with AsyncClient(app=self.app, base_url="http://test") as ac:
            response = await ac.get("/")
            assert response.status_code == 200

tests/utilities/test_logger.py

  • Strengths: Good test coverage with clear organization and stdout handling.
  • Suggestions:
    • Implement parameterized tests:

      @pytest.mark.parametrize("log_level,message", [
          ("info", "Info message"),
          ("error", "Error message")
      ])
      def test_multiple_log_levels(self, log_level, message):
          self.logger.log(log_level, message)
    • Add tests to ensure thread safety, considering that logging may be accessed in a concurrent context:

      def test_thread_safety():
          def log_message(message: str):
              self.logger.log("info", message)
          # threading logic here ...

Overall Recommendations

  1. Documentation: Ensure all new methods and classes have accompanying docstrings.
  2. Error Handling: Add error handling for IO operations within the logger to prevent runtime exceptions.
  3. Buffering: Consider output buffering for efficient logging during high-frequency log entries.
  4. Flexible Configuration: Introduce customizable color settings based on environment variables, enhancing usability across different setups.

In summary, the changes introduced by this PR are solid and address current logging issues. Incorporating the above suggestions will further enhance the system's robustness and maintainability, thereby improving developer experience in the long term.

@devin-ai-integration
Copy link
Contributor Author

Closing due to inactivity for more than 7 days.

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]Why does the logger not work in my project using the FastAPI framework after upgrading to version 0.108? This issue did not occur in version 0.95.

2 participants