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

feat: allow to load tools from external modules #336

Merged
merged 6 commits into from
Jan 6, 2025

Conversation

jrmi
Copy link
Contributor

@jrmi jrmi commented Dec 15, 2024

@ErikBjare this MR is ready for review. It allows to load contributed tools from any python module. Read the added documentation for more information. Let me know what you think.


Important

This pull request enables loading tools from external Python modules by updating configuration and tool initialization logic.

  • Behavior:
    • Allow loading tools from external Python modules specified in TOOL_MODULES in config.rst.
    • Update chat.py and cli.py to use TOOL_FORMAT and TOOL_MODULES from configuration.
    • Tools can be enabled/disabled by default and have a load priority.
  • Tool Initialization:
    • Refactor init_tools() in tools/__init__.py to discover tools from specified modules using _discover_tools().
    • Add disabled_by_default and load_priority attributes to ToolSpec in tools/base.py.
  • Misc:
    • Update computer.py, python.py, rag.py, and subagent.py to use new tool initialization logic.
    • Add examples and instructions for new tool loading capabilities in config.rst.

This description was created by Ellipsis for 8c06068. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 8c06068 in 1 minute and 13 seconds

More details
  • Looked at 415 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. gptme/tools/__init__.py:75
  • Draft comment:
    Consider providing a default value for TOOL_MODULES in case the environment variable is not set to avoid unexpected behavior.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. gptme/cli.py:192
  • Draft comment:
    Ensure that the init_tools function is called with the tool_modules parameter to maintain consistent tool loading behavior.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. gptme/chat.py:78
  • Draft comment:
    Ensure that the init_tools function is called with the tool_modules parameter to maintain consistent tool loading behavior.
  • Reason this comment was not posted:
    Marked as duplicate.
4. gptme/tools/__init__.py:64
  • Draft comment:
    Consider providing a default value for TOOL_MODULES in case the environment variable is not set to avoid unexpected behavior.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_FahKdYWbNHfz9C4F


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

@jrmi jrmi marked this pull request as draft December 15, 2024 18:15
@jrmi jrmi force-pushed the 72-add-external-tool-support branch 9 times, most recently from 7cfed4f to 3c8d8ed Compare December 21, 2024 11:47
@jrmi jrmi marked this pull request as ready for review December 21, 2024 11:47
@jrmi
Copy link
Contributor Author

jrmi commented Dec 21, 2024

Ready for review.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 3c8d8ed in 1 minute and 27 seconds

More details
  • Looked at 1172 lines of code in 22 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. gptme/tools/__init__.py:87
  • Draft comment:
    Store the result of get_available_tools() in a variable and reuse it to avoid multiple calls.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests an optimization that is already effectively implemented via the global _available_tools cache. The function get_available_tools() only computes the tools once when _available_tools is None, and returns the cached value afterward. Storing the result in a local variable would not provide any additional performance benefit.
    Maybe there could be thread safety concerns with the global variable that would make local caching beneficial? Or maybe the global could be modified between loop iterations?
    The code is using a simple global cache pattern that is appropriate for this use case. The global is only modified in clear_tools() which is not called during the loop execution. Thread safety would be a separate concern requiring different solutions.
    The comment should be deleted because it suggests an optimization that is already effectively implemented through the global _available_tools cache.
2. gptme/tools/python.py:227
  • Draft comment:
    Add a type hint for the loaded_tools parameter in the init function for better readability and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The init function in gptme/tools/python.py is missing a type hint for its parameter loaded_tools. Adding a type hint would improve code readability and maintainability.
3. gptme/tools/rag.py:115
  • Draft comment:
    Add a type hint for the args parameter in the init function for better readability and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The init function in gptme/tools/rag.py is missing a type hint for its parameter args. Adding a type hint would improve code readability and maintainability.
4. gptme/tools/subagent.py:163
  • Draft comment:
    Add a type hint for the tool_format parameter in the examples function for better readability and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The init function in gptme/tools/subagent.py is missing a type hint for its parameter tool_format. Adding a type hint would improve code readability and maintainability.
5. gptme/tools/rag.py:112
  • Draft comment:
    Add a type hint for the args parameter in the init function for better readability and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The init function in gptme/tools/rag.py is missing a type hint for its parameter args. Adding a type hint would improve code readability and maintainability.

Workflow ID: wflow_7AZWSEbuwoLaaiIM


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

gptme/tools/__init__.py Outdated Show resolved Hide resolved
@jrmi jrmi force-pushed the 72-add-external-tool-support branch from 3c8d8ed to 7c65f0d Compare December 21, 2024 13:37
@jrmi
Copy link
Contributor Author

jrmi commented Dec 21, 2024

This MR:

  • allows to configure the python modules path loaded at startup
  • improves encapsulation of the loaded_tool variable
  • adds a disabled_by_default flag to handle tools that shouldn't be activated without explicit action
  • moves python function registration inside the python tool itself
  • helps to keep the tool configuration in subagents
  • add tests for a 96% coverage of the tool/__init__.py file
  • improve patch tool support for tool format

@jrmi jrmi force-pushed the 72-add-external-tool-support branch 6 times, most recently from 84995a5 to 3992661 Compare December 21, 2024 15:29
Copy link
Owner

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

Very nice, I had something just like this in mind!

Lots of nice improvements in this PR, happy to try it and merge it once the conflicts are resolved :)

gptme/tools/base.py Outdated Show resolved Hide resolved
gptme/tools/base.py Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
tests/test_tools.py Outdated Show resolved Hide resolved
@jrmi jrmi force-pushed the 72-add-external-tool-support branch 2 times, most recently from 46f2863 to 860157b Compare December 21, 2024 15:49
@jrmi
Copy link
Contributor Author

jrmi commented Dec 21, 2024

Very nice, I had something just like this in mind!

Lots of nice improvements in this PR, happy to try it and merge it once the conflicts are resolved :)

Great! Thanks. I've resolved the conflicts.

@jrmi
Copy link
Contributor Author

jrmi commented Dec 21, 2024

I made my first useful contributed tool today using a gptme-agent and the tool format. I made it using gpt-o4-mini model:

import os
import requests
from gptme.tools import ToolSpec, Parameter, ToolUse
from gptme.message import Message

PUSHOVER_USER_KEY = os.getenv('PUSHOVER_USER_KEY')
PUSHOVER_API_TOKEN = os.getenv('PUSHOVER_API_TOKEN')

def execute(content: str | None, args: list[str] | None, kwargs: dict[str, str] | None, confirm):
    if not PUSHOVER_USER_KEY or not PUSHOVER_API_TOKEN:
        return Message('system', "PUSHOVER_USER_KEY or PUSHOVER_API_TOKEN env variables are not set")
        
    if content is not None and args is not None:
        title = args[0]
        message = content
    elif kwargs is not None:
        title = kwargs.get('title', 'No title')
        message = kwargs.get('message', 'No message')
    else:
        return Message('system', "Tool call failed")

    url = "https://api.pushover.net/1/messages.json"
    payload = {
        "token": PUSHOVER_API_TOKEN,
        "user": PUSHOVER_USER_KEY,
        "message": message,
        "title": title,
    }
    try:
        response = requests.post(url, data=payload, timeout=30)

        if response.status_code == 200:
            return Message('system', "Notification sent successfully")
        else:
            return Message('system', "The notification couldn't be sent")
    except Exception as e:
        return Message('system', f"Something went wrong while sending the notification: {e}")
    
def examples(tool_format):
    return f"""
> User: Send me a notification.
> Assistant:
{ToolUse("send_notification", ["This is a test notification!"], "Success").to_output(tool_format)}
> System: Notification sent successfully.
> Assistant: The notification has been sent.
""".strip()

tool = ToolSpec(
    name="notification",
    desc="Sends a notification via Pushover.",
    instructions="Use this tool to send notifications to the user's Pushover account.",
    examples=examples,
    execute=execute,
    block_types=["notification"],
    parameters=[
        Parameter(
            name="message",
            type="string",
            description="The message to send in the notification.",
            required=True,
        ),
        Parameter(
            name="title",
            type="string",
            description="The title of the notification.",
            required=False,
        ),
    ],
)

So great!!!!

@jrmi jrmi force-pushed the 72-add-external-tool-support branch 3 times, most recently from 2310c48 to 7078371 Compare January 4, 2025 15:18
@jrmi jrmi force-pushed the 72-add-external-tool-support branch from 7078371 to 42aaf58 Compare January 4, 2025 16:04
docs/custom_tool.rst Outdated Show resolved Hide resolved
docs/custom_tool.rst Outdated Show resolved Hide resolved
docs/custom_tool.rst Outdated Show resolved Hide resolved
docs/custom_tool.rst Outdated Show resolved Hide resolved
docs/config.rst Outdated Show resolved Hide resolved
gptme/tools/__init__.py Outdated Show resolved Hide resolved
tests/test_tools.py Outdated Show resolved Hide resolved
gptme/tools/__init__.py Outdated Show resolved Hide resolved
gptme/tools/__init__.py Outdated Show resolved Hide resolved
@jrmi jrmi force-pushed the 72-add-external-tool-support branch from 8142b84 to 9e2af72 Compare January 5, 2025 17:48
@jrmi jrmi requested a review from ErikBjare January 5, 2025 18:10
Comment on lines +5 to +8
------------
In gptme, a custom tool allows you to extend the functionality of the assistant by
defining new tools that can be executed.
This guide will walk you through the process of creating and registering a custom tool.
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should go into a bit more detail about the options people have when wanting to make custom tools, and the differences.

While we didn't have custom tools before, it was easy to write scripts which effectively served as tools when included in the context. I've tended to build tools like that for gptme agents like @TimeToBuildBob, and it works really well.

You can for example write Python scripts with uv script dependencies and use a #!/usr/bin/env -S uv run shebang line to make the scripts self-contained with isolated dependencies. Or use any other programming language (doesn't have to be Python).

Not quite sure how to write it out. I think scripts are preferable in many situations since they can be run and tested independent of gptme.

But you need to write a custom tool for:

  • attaching images or other files to messages
  • get included in the tools section of the system prompt
  • being usable without the shell tool (although functions still depend on ipython...)

I could do this in a separate PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's true, I haven't considered that option and we should add this to the documentation as it's simpler to implement a new tool that way.

With custom tool you can also replace an existing tool with a slightly (or heavily) modified one if you want.

Copy link
Owner

Choose a reason for hiding this comment

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

Doing so in #391

Copy link
Owner

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

Alright, LGTM!

Really happy with this :)

docs/config.rst Outdated Show resolved Hide resolved
@ErikBjare ErikBjare merged commit dd99731 into ErikBjare:master Jan 6, 2025
5 of 7 checks passed
@ErikBjare
Copy link
Owner

Very nice, great work! 🥇

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.

2 participants