-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Add docstrings to utility methods in agent.py #3035
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
base: main
Are you sure you want to change the base?
Conversation
Disclaimer: This review was made by a crew of AI Agents. Code Review for crewAIInc/crewAI PR #3035 — Adding Missing Docstrings to Utility Methods in
|
Disclaimer: This review was made by a crew of AI Agents. Code Review Comments for PR #3035OverviewThis pull request makes significant improvements to the documentation of utility methods in
Positive Aspects
Areas for ImprovementWhile the pull request makes commendable improvements, there are some areas that could be enhanced further: 1. get_delegation_tools()Improvement Suggestions:
Example: def get_delegation_tools(self, agents: List[BaseAgent]) -> List[BaseTool]:
"""
Retrieve delegation tools configured with the provided agents.
Args:
agents (List[BaseAgent]): A list of agents to be used for delegation.
Returns:
List[BaseTool]: Tools associated with delegation through the given agents.
Raises:
ValueError: If agents list is empty or contains invalid agent types.
""" 2. get_multimodal_tools()Improvement Suggestions:
Example: def get_multimodal_tools(self) -> Sequence[BaseTool]:
"""
Retrieve tools used for handling multimodal inputs like images.
Returns:
Sequence[BaseTool]: Currently includes AddImageTool for image handling capabilities.
Note:
This method currently only supports image handling through AddImageTool.
""" 3. get_code_execution_tools()Improvement Suggestions:
Example: def get_code_execution_tools(self) -> Optional[List[BaseTool]]:
"""
Retrieve tools for code execution.
Returns:
List[BaseTool] | None: List of code execution tools if available,
otherwise None if crewai_tools is not installed.
Raises:
ImportError: When crewai_tools package is not installed.
Note:
Requires crewai_tools package to be installed for functionality.
""" 4. get_output_converter()Improvement Suggestions:
Example: def get_output_converter(
self,
llm: BaseLLM,
text: str,
model: str,
instructions: str
) -> Converter:
"""
Create a Converter object for processing model outputs.
Args:
llm (BaseLLM): The language model instance to be used for conversion.
Returns:
Converter: Configured Converter object.
""" Historical Context from Related PRsPrevious discussions in pull requests regarding documentation have highlighted the necessity for:
In particular, PRs that have focused on documenting return types and error handling consistently underscore the value this brings to code maintainability and comprehension. General Recommendations
SummaryThe improvements in this PR are a solid step toward establishing clearer documentation standards within the codebase. By addressing the areas for improvement, particularly around exception handling and usage examples, we can further elevate the quality and maintainability of the documentation. I recommend implementing these suggestions in follow-up PRs to uphold and enhance the documentation quality moving forward. |
Thank you so much for the detailed and constructive reviews! I’m glad the improvements were helpful. I’ll go ahead and make a follow-up PR implementing the raised suggestions — including refining the return types, adding exception details, and enhancing docstring clarity for conditional dependencies. That way, this PR can stay clean and focused. Let me know if you’d prefer the changes here instead — I’m happy to update either way. |
Hi again 👋 Just following up to see if it's okay to proceed with a follow-up PR for the suggested docstring refinements (like exception handling, improved types, etc.). Happy to make those changes in a separate PR — just wanted to confirm before opening it. Thanks again for your time and feedback! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhyeyinf great changes!
However can you add the type hints also?
Thank you so much for the feedback! 🙏 Please let me know if there’s anything else you'd like me to adjust — happy to make further changes if needed. |
Thanks again for the review and guidance! 🙌 Let me know if there's anything else you'd like me to improve — happy to iterate further. |
Hi @lucasgomide 👋 Just following up on this — I’ve addressed the Let me know if there's anything else you'd like me to update. Otherwise, if everything looks good, this should be ready for merge. Thanks again! 😊 |
@dhyeyinf I'm sorry about delay to reply but now you have to resolve some conflicts. Please tag me when you did it |
CrewAI has been using https://mintlify.com/ to serve its docs
…Inc#3030) Co-authored-by: Tony Kipkemboi <iamtonykipkemboi@gmail.com>
* Added Union of List of Task, None, NotSpecified * Seems like a flaky test * Fixed run time issue * Fixed Linting issues * fix pydantic error * aesthetic changes --------- Co-authored-by: Lucas Gomide <lucaslg200@gmail.com>
…AIInc#3023) * feat: support to initialize a tool from defined Tool attributes * fix: ensure Agent is able to load a list of Tools dynamically
* Add Oxylabs tools * Review updates * Review updates --------- Co-authored-by: Tony Kipkemboi <iamtonykipkemboi@gmail.com>
) * test: add tests to test get_crews * feat: improve Crew search while resetting their memories Some memories couldn't be reset due to their reliance on relative external sources like `PDFKnowledge`. This was caused by the need to run the reset memories command from the `src` directory, which could break when external files weren't accessible from that path. This commit allows the reset command to be executed from the root of the project — the same location typically used to run a crew — improving compatibility and reducing friction. * feat: skip cli/templates folder while looking for Crew * refactor: use console.print instead of print
* docs: added Maxim support for Agent Observability * enhanced the maxim integration doc page as per the github PR reviewer bot suggestions * Update maxim-observability.mdx * Update maxim-observability.mdx - Fixed Python version, >=3.10 - added expected_output field in Task - Removed marketing links and added github link * added maxim in observability * updated the maxim docs page * fixed image paths * removed demo link --------- Co-authored-by: Tony Kipkemboi <iamtonykipkemboi@gmail.com> Co-authored-by: Lucas Gomide <lucaslg200@gmail.com>
* docs: add pt-br translations Powered by a CrewAI Flow https://github.com/danielfsbarreto/docs_translator * Update mcp/overview.mdx brazilian docs Its en-US counterpart was updated after I did a pass, so now it includes the new section about @crewbase
…tools per agent (crewAIInc#3065) * feat: enhance CrewBase MCP tools support to allow selecting multiple tools per agent * docs: clarify how to access MCP tools * build: upgrade crewai-tools
This change aims to be more generic, so we don’t have to constantly reflect all available LLM options suggested by the CLI when creating a crew.
Co-authored-by: Lucas Gomide <lucaslg200@gmail.com>
…eation (crewAIInc#3060) * fix: normalize project names by stripping trailing slashes in crew creation - Strip trailing slashes from project names in create_folder_structure - Add comprehensive tests for trailing slash scenarios - Fixes crewAIInc#3059 The issue occurred because trailing slashes in project names like 'hello/' were directly incorporated into pyproject.toml, creating invalid package names and script entries. This fix silently normalizes project names by stripping trailing slashes before processing, maintaining backward compatibility while fixing the invalid template generation. Co-Authored-By: João <joao@crewai.com> * trigger CI re-run to check for flaky test issue Co-Authored-By: João <joao@crewai.com> * fix: resolve circular import in CLI authentication module - Move ToolCommand import to be local inside _poll_for_token method - Update test mock to patch ToolCommand at correct location - Resolves Python 3.11 test collection failure in CI Co-Authored-By: João <joao@crewai.com> * feat: add comprehensive class name validation for Python identifiers - Ensure generated class names are always valid Python identifiers - Handle edge cases: names starting with numbers, special characters, keywords, built-ins - Add sanitization logic to remove invalid characters and prefix with 'Crew' when needed - Add comprehensive test coverage for class name validation edge cases - Addresses GitHub PR comment from lucasgomide about class name validity Fixes include: - Names starting with numbers: '123project' -> 'Crew123Project' - Python built-ins: 'True' -> 'TrueCrew', 'False' -> 'FalseCrew' - Special characters: 'hello@world' -> 'HelloWorld' - Empty/whitespace: ' ' -> 'DefaultCrew' - All generated class names pass isidentifier() and keyword checks Co-Authored-By: João <joao@crewai.com> * refactor: change class name validation to raise errors instead of generating defaults - Remove default value generation (Crew prefix/suffix, DefaultCrew fallback) - Raise ValueError with descriptive messages for invalid class names - Update tests to expect validation errors instead of default corrections - Addresses GitHub comment feedback from lucasgomide about strict validation Co-Authored-By: João <joao@crewai.com> * fix: add working directory safety checks to prevent test interference Co-Authored-By: João <joao@crewai.com> * fix: standardize working directory handling in tests to prevent corruption Co-Authored-By: João <joao@crewai.com> * fix: eliminate os.chdir() usage in tests to prevent working directory corruption - Replace os.chdir() with parent_folder parameter for create_folder_structure tests - Mock create_folder_structure directly for create_crew tests to avoid directory changes - All 12 tests now pass locally without working directory corruption - Should resolve the 103 failing tests in Python 3.12 CI Co-Authored-By: João <joao@crewai.com> * fix: remove unused os import to resolve lint failure - Remove unused 'import os' statement from test_create_crew.py - All tests still pass locally after removing unused import - Should resolve F401 lint error in CI Co-Authored-By: João <joao@crewai.com> * feat: add folder name validation for Python module names - Implement validation to ensure folder_name is valid Python identifier - Check that folder names don't start with digits - Validate folder names are not Python keywords - Sanitize invalid characters from folder names - Raise ValueError with descriptive messages for invalid cases - Update tests to validate both folder and class name requirements - Addresses GitHub comment requiring folder names to be valid Python module names Co-Authored-By: João <joao@crewai.com> * fix: correct folder name validation logic to match test expectations - Fix validation regex to catch names starting with invalid characters like '@#/' - Ensure validation properly raises ValueError for cases expected by tests - Maintain support for valid cases like 'my.project/' -> 'myproject' - Address lucasgomide's comment about valid Python module names Co-Authored-By: João <joao@crewai.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: João <joao@crewai.com> Co-authored-by: Lucas Gomide <lucaslg200@gmail.com>
* Adding Nebius to docs Submitting this PR on behalf of Nebius AI Studio to add Nebius models to the CrewAI documentation. I tested with the latest CrewAI + Nebius setup to ensure compatibility. cc @tonykipkemboi * updated LiteLLM page --------- Co-authored-by: Lucas Gomide <lucaslg200@gmail.com>
When creating a Crew via the CLI and selecting the Azure provider, the generated .env file had environment variables in lowercase. This commit ensures that all environment variables are written in uppercase.
* Updated LiteLLM dependency. This moves to the latest stable release. Critically, this includes a fix from BerriAI/litellm#11563 which is required to use grok-3-mini with crewAI. * Ran `uv sync` as requested.
a2bd276
to
4ccdc6f
Compare
Hi @lucasgomide, the branch is now successfully rebased and up to date with Let me know if there's anything else you'd like me to adjust. Thanks again for all your help! 🙏 |
What this PR does
This pull request adds missing docstrings to several utility methods in
src/crewai/agent.py
to improve code readability and developer understanding.Updated Methods
get_delegation_tools(self, agents)
get_multimodal_tools(self)
get_code_execution_tools(self)
get_output_converter(self, llm, text, model, instructions)
These docstrings follow PEP 257 standards and are aimed at helping contributors and users better understand the codebase.
Why this is important
Improved documentation supports open-source maintainability and makes it easier for new contributors to onboard. These utility functions previously lacked docstrings, which could lead to confusion about their purpose or return types.
Additional Notes
No functionality was changed. The file passes
ruff
andblack
formatting checks.Please feel free to suggest any improvements — I’d be happy to make changes if needed. 🙂