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: added think tool to let agent query a reasoner #416

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ErikBjare
Copy link
Owner

@ErikBjare ErikBjare commented Jan 22, 2025

TODO

  • Send full conversation context (different mode/tool?)
  • Karpathy-style consortium

Important

Adds think tool to enhance reasoning on complex problems using powerful LLMs, with model specification and file content integration.

  • New Feature:
    • Adds think tool in gptme/tools/think.py to enhance reasoning on complex problems using more powerful LLMs.
    • Supports specifying a model for reasoning via model parameter.
  • Functionality:
    • execute() function processes reasoning requests, reads file contents, and sends them to a specified model.
    • Logs model usage and handles file reading errors.
  • Tool Specification:
    • ToolSpec defines tool parameters and usage instructions, including the need to specify files in requests.
  • Future Enhancements:
    • TODOs for implementing a consortium approach for aggregating responses from multiple models.

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

execute=execute,
instructions="""
Use this tool when you need help with complex reasoning or problem-solving.
Wrap your thinking request in a code block with the language tag: `think`
Copy link
Owner Author

Choose a reason for hiding this comment

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

Shouldn't use markdown specific format instructions.

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 1674ef7 in 42 seconds

More details
  • Looked at 134 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. gptme/tools/think.py:63
  • Draft comment:
    Avoid using a broad Exception catch. Specify the exception type to handle specific errors and avoid masking other issues.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    In this case, the broad exception handling is actually reasonable because: 1) It's just for logging a warning, not silently swallowing errors 2) There are multiple things that could go wrong when reading files (FileNotFound, Permission, etc) 3) The code gracefully continues either way 4) This is auxiliary functionality for extracting context, not core business logic.
    The comment does raise a valid general coding practice. Broad exception handling can mask bugs and make debugging harder.
    While generally true, in this specific case the broad exception is appropriate given it's just for logging warnings about optional file reading that shouldn't block the main functionality.
    The comment should be deleted as the current exception handling is appropriate for this specific use case where we want to catch all possible file reading errors and continue.
2. gptme/tools/think.py:94
  • Draft comment:
    Ensure response is not None and has a content attribute before accessing it to avoid potential AttributeError.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_D7dWjKZL9GvV2S7I


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

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 19.60784% with 41 lines in your changes missing coverage. Please review.

Project coverage is 69.37%. Comparing base (db212bd) to head (7a740b7).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
gptme/tools/think.py 19.60% 41 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #416      +/-   ##
==========================================
- Coverage   70.09%   69.37%   -0.73%     
==========================================
  Files          70       71       +1     
  Lines        5849     5900      +51     
==========================================
- Hits         4100     4093       -7     
- Misses       1749     1807      +58     
Flag Coverage Δ
anthropic/claude-3-haiku-20240307 68.13% <19.60%> (-0.48%) ⬇️
openai/gpt-4o-mini 67.50% <19.60%> (-0.64%) ⬇️

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.

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. Incremental review on 713ada9 in 47 seconds

More details
  • Looked at 118 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. gptme/tools/think.py:82
  • Draft comment:
    Setting files_block to False here is redundant as the loop will naturally continue to the next line.
            pass
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code sets files_block to False after processing a file, which is unnecessary since the loop will continue to the next line anyway.

Workflow ID: wflow_1ojtTqGl3udP8Iay


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/think.py Outdated Show resolved Hide resolved
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! Incremental review on 8bf6e92 in 35 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. gptme/tools/think.py:158
  • Draft comment:
    The type should be str to match Python's type system, not string.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    This is a Parameter object used for tool specification, not a Python type hint. Looking at the Parameter class import from .base, without seeing its implementation, we can't be certain whether it expects "str" or "string". The change from "str" to "string" was intentional, suggesting "string" might be the correct format for this tool specification system.
    I don't have access to the Parameter class implementation to verify which string format it expects. The original author might have changed it to "string" for a specific reason.
    Without seeing the Parameter class implementation, we can't be confident that "str" is the correct choice. The intentional change to "string" suggests it might be the desired format.
    Delete the comment since we don't have strong evidence that "str" is the correct choice, and the change to "string" was likely intentional.

Workflow ID: wflow_ekTbi8MefuQZrQ41


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

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! Incremental review on 7a740b7 in 21 seconds

More details
  • Looked at 24 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. gptme/tools/think.py:49
  • Draft comment:
    The change to args[0].split("=", 1)[-1] is a good improvement for handling strings with multiple '=' characters.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change from args[0].split("=")[1] to args[0].split("=", 1)[-1] is a good improvement. It ensures that the split operation only occurs once, which is more efficient and prevents potential errors if the string contains multiple '=' characters.
2. gptme/tools/think.py:157
  • Draft comment:
    The change to explicitly use type="string" and description="..." improves clarity and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change in the Parameter initialization improves clarity by explicitly naming the fields type and description. This is a good practice for readability and maintainability.

Workflow ID: wflow_LmYZCCsTEGepTg3O


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

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