refactor: refactored how tools are loaded, to enable loading external tools

[ErikBjare]

Important

Refactor tool loading to support external tools with a dynamic registry and add functionality to load tools from external Python files.

• Behavior:
  • Refactor tool loading to use ToolSpec.get_tools() instead of all_tools list in cli.py and tools/__init__.py.
  • Add load_from_file() in base.py to load tools from external Python files.
• ToolSpec Class:
  • Add get_tool() and get_tools() class methods in base.py for tool registry management.
  • Modify __post_init__() to register tools in a global _tools dictionary.
• Initialization:
  • Update init_tools() in tools/__init__.py to use ToolSpec.get_tools().
  • Ensure python_tool is initialized last by sorting tools in init_tools().
• Misc:
  • Remove tool_append from imports in tools/__init__.py.
  • Update docstrings in base.py for clarity.
  • Add lock parameter to LogManager.load() in logmanager.py.

^{This description was created by for c75c4bb. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 4ac38d0 in 1 minute and 1 seconds

More details

• Looked at 199 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. gptme/tools/base.py:507

• Draft comment:
  Use logger instead of print for logging.

logger.info(f"Loaded tools {tools_new} from {path}")

• Reason this comment was not posted:
  Confidence changes required: 50%
  The load_from_file function uses print for logging, which is not a best practice. It should use the logger for consistency and better control over logging levels.

2. gptme/tools/__init__.py:75

• Draft comment:
  Redundant check for tool in loaded_tools. This check is already performed after checking availability, so it can be removed here.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The init_tools function in gptme/tools/__init__.py has a redundant check for tool in loaded_tools. This check is performed twice, once before checking availability and once after. The first check can be removed.

3. gptme/tools/base.py:184

• Draft comment:
  The get_tool method is not used in the provided code. Consider removing it if it's not used elsewhere in the codebase.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The get_tool method in ToolSpec class is not used anywhere in the provided code. It might be unnecessary unless used elsewhere in the codebase.

Workflow ID: wflow_78BrJLpDAG7VQpCF

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 69.69697% with 10 lines in your changes missing coverage. Please review.

Project coverage is 73.56%. Comparing base (a40f538) to head (c75c4bb).

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
gptme/tools/base.py	52.38%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #313      +/-   ##
==========================================
- Coverage   73.75%   73.56%   -0.19%     
==========================================
  Files          68       68              
  Lines        4949     4975      +26     
==========================================
+ Hits         3650     3660      +10     
- Misses       1299     1315      +16     

Flag	Coverage Δ
anthropic/claude-3-haiku-20240307	71.65% <69.69%> (-0.18%)	⬇️
openai/gpt-4o-mini	70.49% <69.69%> (-0.31%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 7969910 in 33 seconds

More details

• Looked at 187 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. gptme/tools/base.py:147

• Draft comment:
  Using a global variable _tools for tool registration can lead to potential issues in larger applications, such as unexpected side effects or difficulties in testing. Consider encapsulating this within a class or using a more controlled registry pattern.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The use of a global variable for tool registration can lead to potential issues in larger applications, such as unexpected side effects or difficulties in testing. A more encapsulated approach would be beneficial.

2. gptme/tools/base.py:507

• Draft comment:
  Use logger instead of print for logging to maintain consistent logging practices.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The load_from_file function uses print for logging, which is not ideal for production code. It should use the logger for consistent logging practices.

3. gptme/tools/base.py:503

• Draft comment:
  Consider adding error handling for the dynamic import in load_from_file to handle potential import errors gracefully.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The load_from_file function should handle potential import errors gracefully, especially when dealing with dynamic imports from external files.

Workflow ID: wflow_X5r8xJlJ2mA7P9Tp

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 803c1a8 in 26 seconds

More details

• Looked at 228 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. gptme/tools/base.py:507

• Draft comment:
  Consider using logger.info instead of print for better logging practices.

    logger.info(f"Loaded tools {tools_new} from {path}")

• Reason this comment was not posted:
  Confidence changes required: 50%
  The load_from_file function in base.py uses print to output loaded tools, which is not ideal for production code. It should use logging instead.

2. gptme/tools/base.py:502

• Draft comment:
  Consider removing the script directory from sys.path after importing to avoid potential import issues.

    sys.path.remove(str(script_dir))

• Reason this comment was not posted:
  Confidence changes required: 50%
  The load_from_file function in base.py appends the script directory to sys.path without removing it later. This could lead to potential issues with module imports elsewhere in the application.

3. gptme/logmanager.py:258

• Draft comment:
  Ensure that the lock parameter is handled correctly to avoid potential issues in multi-threaded environments. Consider documenting the implications of setting lock=False.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The load method in LogManager class in logmanager.py has a potential issue with the lock parameter. The default value is True, but it might be better to explicitly handle cases where locking is not desired, especially in multi-threaded environments.

Workflow ID: wflow_SBckJGMXXoILpJ2L

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on c75c4bb in 31 seconds

More details

• Looked at 246 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. gptme/tools/base.py:507

• Draft comment:
  Consider using logger.info instead of print for logging loaded tools.

    logger.info(f"Loaded tools {tools_new} from {path}")

• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a new method load_from_file in base.py to load tools from external Python files. However, the method uses print to output loaded tools, which is not ideal for production code. It would be better to use logging for such messages.

2. gptme/tools/base.py:508

• Draft comment:
  Consider using a loop instead of a list comprehension with assignment for clarity.

    new_tools = []
    for tool_name in tools_new:
        tool = ToolSpec.get_tool(tool_name)
        if tool:
            new_tools.append(tool)
    return new_tools

• Reason this comment was not posted:
  Confidence changes required: 50%
  The load_from_file function in base.py returns a list of ToolSpec objects, but it uses a list comprehension with an assignment inside, which is not a common practice and can be confusing. It would be clearer to use a loop instead.

3. gptme/tools/__init__.py:79

• Draft comment:
  Simplify the sorting of tools by using a key function directly.

        sorted(ToolSpec.get_tools().values(), key=lambda tool: tool.name == "python")

• Reason this comment was not posted:
  Confidence changes required: 50%
  The init_tools function in tools/__init__.py uses a lambda function to sort tools, which can be simplified by using a key function directly. This improves readability.

4. gptme/logmanager.py:258

• Draft comment:
  Ensure that the lock parameter is handled correctly when lock is False to avoid unexpected behavior if the lock file is already locked by another process.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The load method in LogManager class in logmanager.py has been updated to include a lock parameter. However, the method does not handle the case where lock is False and the lock file is already locked by another process. This could lead to unexpected behavior.

Workflow ID: wflow_X4gIltXiekCqoqnb

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.