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

Use CREWAI_PROMPT_FILE environment variable. #777

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mbarnathan
Copy link

CrewAI initializes several I18N instances deep within the library at package initialization time, using the default prompts.
Despite the ability to specify prompt files within a Crew, the defaults will still be used in several places throughout the
workflow of calling that Crew. Overriding the defaults consistently throughout the package currently requires monkeypatching.

This change will allow overriding the default prompt file location in the $CREWAI_PROMPT_FILE environment variable, allowing consistency throughout the library.

CrewAI initializes several I18N instances deep within the library
at package initialization time, using the default prompts.
This makes overriding the defaults consistently throughout the
package very difficult, requiring monkeypatching.

This change will allow overriding the default prompt file location
in the $CREWAI_PROMPT_FILE environment variable, allowing
consistency throughout the library.
@joaomdmoura
Copy link
Collaborator

Despite the ability to specify prompt files within a Crew, the defaults will still be used in several places throughout the
workflow of calling that Crew

Oh interesting I didn't realize that was the case, wouldn't that still be a problem even if we use the ENV var thought given the current changes? I might be missing something it's 1am lol

@mbarnathan
Copy link
Author

mbarnathan commented Jul 6, 2024

Most manually initialized objects can be passed a custom I18N object, but the main problem was the CrewAgentExecutor import calling into Prompts with the default I18N object (I think this is occurring through the memory function). This is being initialized in the agents package's __ init __.py, which prevents getting an override in via a parameter early. You're either stuck with layers of deep overrides of core Crew classes such as CrewAgentExecutor or something like an environment variable or monkeypatching I18N that applies globally.

Copy link

This PR is stale because it has been open for 45 days with no activity.

@joaomdmoura
Copy link
Collaborator

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

Code Review Comment for PR #777: Use CREWAI_PROMPT_FILE Environment Variable

Overview

This pull request introduces an important enhancement to the CrewAI library by allowing configuration of the prompt file location through the CREWAI_PROMPT_FILE environment variable. This update not only improves flexibility but also reflects good practices in configuration management. Below are my insights and recommendations gathered from the changes in the modified files.

File: src/crewai/utilities/i18n.py

Positive Aspects

  • Type Hints and Pydantic Models: The use of type hints adds clarity to the code, making it easier to understand and maintain.
  • Documentation: The Field descriptions are clear and assist in understanding the purpose of each configuration.
  • Backward Compatibility: The updates respect the existing functionality, which is crucial for maintaining user trust.

Recommendations

  1. Error Handling Enhancement

    • Current implementation lacks robust error handling when opening files. To improve the stability, consider this refactor:
    try:
        with open(prompts_path, "r", encoding='utf-8') as f:
            self._prompts = json.load(f)
    except FileNotFoundError:
        raise ValueError(f"Prompt file not found at: {prompts_path}")
    except json.JSONDecodeError:
        raise ValueError(f"Invalid JSON format in prompt file: {prompts_path}")
  2. Environment Variable Access Safety

    • The current approach can lead to confusion. A safer way to handle the environment variable would be:
    prompts_path = os.getenv("CREWAI_PROMPT_FILE", "").strip()
    if prompts_path and not os.path.isfile(prompts_path):
        raise ValueError(f"Environment variable CREWAI_PROMPT_FILE points to invalid path: {prompts_path}")
  3. Code Documentation

    • Including docstrings for classes and methods significantly enhances understanding. For instance:
    class I18N(BaseModel):
        """
        Internationalization support for CrewAI prompts.
        """
        @model_validator(mode="after")
        def load_prompts(self) -> "I18N":
            """
            Load prompts from the configured source.
            Raises ValueError if the prompt file is invalid or cannot be loaded.
            """

File: tests/utilities/test_i18n.py

Positive Aspects

  • Test Coverage: Good coverage ensures that the new functionality is tested.
  • Environment Cleanup: Proper handling of environment variables in tests to prevent side effects.

Recommendations

  1. Test Structure Enhancement

    • Adding setup and teardown methods would improve the organization of tests:
    @pytest.fixture
    def prompt_file_path():
        return os.path.join(os.path.dirname(__file__), "prompts.json")
    
    @pytest.fixture
    def env_cleanup():
        old_env = os.environ.get("CREWAI_PROMPT_FILE")
        yield
        if old_env:
            os.environ["CREWAI_PROMPT_FILE"] = old_env
        else:
            os.environ.pop("CREWAI_PROMPT_FILE", None)
  2. Additional Test Cases

    • It's crucial to cover edge cases. Consider implementing tests for error conditions:
    def test_invalid_env_path(env_cleanup):
        os.environ["CREWAI_PROMPT_FILE"] = "nonexistent_file.json"
        with pytest.raises(ValueError, match=".*invalid path.*"):
            i18n = I18N()
            i18n.load_prompts()

General Recommendations

  • Configuration Constants: Introduce a constants file for environment variable names to avoid hardcoding:

    # src/crewai/constants.py
    ENV_PROMPT_FILE = "CREWAI_PROMPT_FILE"
    DEFAULT_PROMPT_PATH = "../translations/en.json"
  • Logging for Debugging: Implement logging to keep track of the loading process:

    import logging
    logger = logging.getLogger(__name__)
    logger.debug(f"Loading prompts from: {prompts_path}")
  • Enhanced Type Hints: Utilize more specific type hints for greater clarity:

    prompt_file: Optional[Union[str, Path]] = Field(default=None, description="Path to the prompt_file file to load...")

Conclusion

These improvements will enhance the code's robustness, maintainability, and developer experience while preserving the original functionality.

This PR reflects a significant step towards flexible configuration management in the CrewAI library, aligning with best practices for user experience and maintainability. I recommend considering the above suggestions for refining this implementation further.


Please feel free to reach out for further clarifications or discussions regarding the suggestions!

@pythonbyte
Copy link
Collaborator

Hey @mbarnathan thanks for the Pull Request.

Since there have been quite a few changes to the project since July 6, could you please take a moment to check if this PR is still relevant?

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants