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

refactor: work on programmatic interface, self-reviewing agent #199

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

Conversation

ErikBjare
Copy link
Owner

@ErikBjare ErikBjare commented Oct 15, 2024

  • refactored LogManager into mutable manager and immutable Log dataclass
  • added wip treeofthought script

TODO

  • Manually test confirmation refactor

Important

Refactor logging system with new Log dataclass and add tree-branching conversation script.

  • Refactor LogManager:
    • Introduced Log dataclass in logmanager.py for immutable message handling.
    • Updated LogManager to use Log for managing conversation logs.
    • Removed prepare_messages from LogManager, added as standalone function.
  • Scripts:
    • Added treeofthoughts.py for tree-branching conversation evaluation.
  • Imports and Usage:
    • Updated imports and usage of LogManager and Log in chat.py, cli.py, commands.py, server/api.py, and tools/chats.py.
    • Replaced Conversation with ConversationMeta in cli.py and list_user_messages.py.

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

Copy link

@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 36d1b7f in 1 minute and 29 seconds

More details
  • Looked at 857 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. gptme/logmanager.py:6
  • Draft comment:
    Ensure that the replace method is imported from dataclasses to avoid potential NameError issues.
from dataclasses import dataclass, field, replace
  • Reason this comment was not posted:
    Comment did not seem useful.
2. gptme/server/api.py:113
  • Draft comment:
    Ensure manager.log is not empty before accessing manager.log.messages to prevent potential AttributeError.
if manager.log:
    msgs = prepare_messages(manager.log.messages)
else:
    msgs = []
  • Reason this comment was not posted:
    Comment did not seem useful.
3. scripts/treeofthoughts.py:23
  • Draft comment:
    Ensure that _step always yields valid Message objects to prevent runtime errors when appending to log.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_La8vX5FjzlZhjaLR


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.


# don't save backup branch if undoing a command
if not self[-1].content.startswith("/"):
if not self.log[-1].content.startswith("/"):
Copy link

Choose a reason for hiding this comment

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

Add a check to ensure self.log is not empty before accessing self.log[-1] to prevent potential IndexError.

Suggested change
if not self.log[-1].content.startswith("/"):
if self.log and not self.log[-1].content.startswith("/"):

@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
80 1 79 0
View the top 1 failed tests by shortest run time
tests.test_cli test_fileblock
Stack Traces | 0.659s run time
args = ['--name', 'test-58859-test_fileblock', '/impersonate ```patch hello/hello.py\n<<<<<<< ORIGINAL\nprint("hello")\n=======\nprint("hello world")\n>>>>>>> UPDATED\n```']
runner = <click.testing.CliRunner object at 0x7f940fd8fb80>

    @pytest.mark.slow
    def test_fileblock(args: list[str], runner: CliRunner):
        args_orig = args.copy()
    
        # tests saving with a ```filename.txt block
        tooluse = ToolUse("save", ["hello.py"], "print('hello')")
        args.append(f"/impersonate {tooluse.to_output()}")
        print(f"running: gptme {' '.join(args)}")
        result = runner.invoke(gptme.cli.main, args)
        assert result.exit_code == 0
    
        # read the file
        with open("hello.py") as f:
            content = f.read()
        assert content == "print('hello')\n"
    
        # test append
        args = args_orig.copy()
        tooluse = ToolUse("append", ["hello.py"], "print('world')")
        args.append(f"/impersonate {tooluse.to_output()}")
        print(f"running: gptme {' '.join(args)}")
        result = runner.invoke(gptme.cli.main, args)
        assert result.exit_code == 0
    
        # read the file
        with open("hello.py") as f:
            content = f.read()
        assert content == "print('hello')\nprint('world')\n"
    
        # test write file to directory that doesn't exist
        tooluse = ToolUse("save", ["hello/hello.py"], 'print("hello")')
        args = args_orig.copy()
        args.append(f"/impersonate {tooluse.to_output()}")
        print(f"running: gptme {' '.join(args)}")
        result = runner.invoke(gptme.cli.main, args)
        assert result.exit_code == 0
    
        # test patch on file in directory
        patch = '<<<<<<< ORIGINAL\nprint("hello")\n=======\nprint("hello world")\n>>>>>>> UPDATED'
        tooluse = ToolUse("patch", ["hello/hello.py"], patch)
        args = args_orig.copy()
        args.append(f"/impersonate {tooluse.to_output()}")
        print(f"running: gptme {' '.join(args)}")
        result = runner.invoke(gptme.cli.main, args)
        assert result.exit_code == 0
    
        # read the file
>       with open("hello/hello.py") as f:
E       FileNotFoundError: [Errno 2] No such file or directory: 'hello/hello.py'

.../gptme/tests/test_cli.py:182: FileNotFoundError

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

scripts/treeofthoughts.py Outdated Show resolved Hide resolved
Copy link

@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 e63d525 in 33 seconds

More details
  • Looked at 295 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. scripts/treeofthoughts.py:25
  • Draft comment:
    Consider handling exceptions like subprocess.CalledProcessError to ensure robustness when running subprocess commands.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of subprocess.run without handling potential exceptions can lead to unhandled errors if the command fails. It's a good practice to handle exceptions like subprocess.CalledProcessError to ensure robustness.
2. scripts/treeofthoughts.py:31
  • Draft comment:
    Consider handling exceptions like subprocess.CalledProcessError to ensure robustness when running subprocess commands.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of subprocess.run without handling potential exceptions can lead to unhandled errors if the command fails. It's a good practice to handle exceptions like subprocess.CalledProcessError to ensure robustness.
3. scripts/treeofthoughts.py:39
  • Draft comment:
    Consider handling exceptions like subprocess.CalledProcessError to ensure robustness when running subprocess commands.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of subprocess.run without handling potential exceptions can lead to unhandled errors if the command fails. It's a good practice to handle exceptions like subprocess.CalledProcessError to ensure robustness.
4. scripts/treeofthoughts.py:97
  • Draft comment:
    Consider handling exceptions like subprocess.CalledProcessError to ensure robustness when running subprocess commands.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of subprocess.run without handling potential exceptions can lead to unhandled errors if the command fails. It's a good practice to handle exceptions like subprocess.CalledProcessError to ensure robustness.
5. scripts/treeofthoughts.py:126
  • Draft comment:
    Consider handling exceptions like subprocess.CalledProcessError to ensure robustness when running subprocess commands.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of subprocess.run without handling potential exceptions can lead to unhandled errors if the command fails. It's a good practice to handle exceptions like subprocess.CalledProcessError to ensure robustness.
6. scripts/treeofthoughts.py:187
  • Draft comment:
    Consider handling exceptions like subprocess.CalledProcessError to ensure robustness when running subprocess commands.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of subprocess.run without handling potential exceptions can lead to unhandled errors if the command fails. It's a good practice to handle exceptions like subprocess.CalledProcessError to ensure robustness.
7. gptme/chat.py:164
  • Draft comment:
    Use Union[Log, List[Message]] for type hinting instead of Log | list[Message] for better clarity and compatibility.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The current implementation of the step function in gptme/chat.py uses Log | list[Message] as a type hint for the log parameter. This can be improved for clarity and type safety by using Union[Log, List[Message]].

Workflow ID: wflow_PxtchXuYJyA1Sxt8


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

@ErikBjare ErikBjare mentioned this pull request Oct 15, 2024
@ErikBjare ErikBjare force-pushed the dev/programmatic-api-and-treeofthoughts branch from e63d525 to 76b9abf Compare October 15, 2024 12:52
Copy link

@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 76b9abf in 48 seconds

More details
  • Looked at 295 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. scripts/treeofthoughts.py:47
  • Draft comment:
    Use triple backticks for code blocks in Python strings.
        context += f"

{f}\n"

- **Reason this comment was not posted:** 
Comment looked like it was already resolved.

</details>

<details>
<summary>2. <code>scripts/treeofthoughts.py:25</code></summary>

- **Draft comment:** 
Consider adding exception handling for subprocess.run to handle potential errors from the git command.
- **Reason this comment was not posted:** 
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant because subprocess.run is used in the new functions, and it can raise exceptions. Adding exception handling could improve the robustness of the code. However, the current usage does not use check=True, so the main concern would be handling FileNotFoundError. The comment is actionable and suggests a clear improvement to the code.
The comment does not specify which exceptions to handle or how to handle them, which might make it less actionable. Additionally, if the subprocess.run calls are expected to always succeed in the given environment, exception handling might be unnecessary.
Even if the environment is controlled, handling potential exceptions can prevent unexpected crashes and improve code robustness. The comment is a general suggestion that can be refined by the developer.
Keep the comment as it suggests a valid improvement to handle potential errors from subprocess.run, which is used in the new code.

</details>

<details>
<summary>3. <code>scripts/treeofthoughts.py:97</code></summary>

- **Draft comment:** 
Consider adding exception handling for subprocess.run to handle potential errors from the make command.
- **Reason this comment was not posted:** 
Marked as duplicate.

</details>

<details>
<summary>4. <code>scripts/treeofthoughts.py:187</code></summary>

- **Draft comment:** 
Consider adding exception handling for subprocess.run to handle potential errors from the git command.
- **Reason this comment was not posted:** 
Marked as duplicate.

</details>

<details>
<summary>5. <code>scripts/treeofthoughts.py:204</code></summary>

- **Draft comment:** 
Ensure that the Log class supports the pop method or use an appropriate method to remove the last message.
- **Reason this comment was not posted:** 
Comment did not seem useful.

</details>


Workflow ID: <workflowid>`wflow_jBkuZtgeBxNvQ5Nc`</workflowid>

</details>


----
You can customize Ellipsis with :+1: / :-1: [feedback](https://docs.ellipsis.dev/review), review rules, user-specific overrides, `quiet` mode, and [more](https://docs.ellipsis.dev/config).

…mation and simplifying confirmation support in server
Copy link

@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 11df31e in 38 seconds

More details
  • Looked at 862 lines of code in 13 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. gptme/tools/save.py:9
  • Draft comment:
    The ask_execute function is no longer used and should be removed to clean up the code.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The ask_execute function is no longer used in gptme/tools/save.py and should be removed to clean up the code.
2. gptme/tools/save.py:80
  • Draft comment:
    The ask_execute function is no longer used and should be removed to clean up the code.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The ask_execute function is no longer used in gptme/tools/save.py and should be removed to clean up the code.

Workflow ID: wflow_MWodEYP7BDsjI8Z0


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/python.py Outdated Show resolved Hide resolved
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
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