-
Notifications
You must be signed in to change notification settings - Fork 44.6k
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
Prep for #2291 Allow feedback and interrupting during continuous run #2481
Conversation
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
|
Can you please resolve the conflicts so we can review? :) |
Terribly sorry. I'll fix that this weekend.
…On Sat, Apr 22, 2023, 6:51 AM Reinier van der Leer ***@***.***> wrote:
Can you please resolve the conflicts so we can review? :)
—
Reply to this email directly, view it on GitHub
<#2481 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVTCVBAR7DHMIYSRUCUJ6DXCOZ4JANCNFSM6AAAAAAXDN7S2Y>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2481 +/- ##
==========================================
+ Coverage 41.66% 42.53% +0.87%
==========================================
Files 65 65
Lines 3029 3042 +13
Branches 507 509 +2
==========================================
+ Hits 1262 1294 +32
+ Misses 1702 1682 -20
- Partials 65 66 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size |
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size |
autogpt/agent/agent.py
Outdated
command_registry: CommandRegistry, | ||
config: AIConfig, | ||
system_prompt: str, | ||
initial_prompt: str, |
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.
That is still triggering_prompt I am afraid, it is the message we send at the end of a "conversation" in order to trigger a response. Please explain the change.
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.
This was my change; I found triggering_prompt
to be a bit ambiguous and thought renaming it could help, although I see my choice of name could have been better.
if self.autonomous_cycles_remaining > 0: | ||
self.autonomous_cycles_remaining -= 1 | ||
|
||
# Check if there's a result from the command append it to the message |
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.
There is something really wrong here, that code should be outside of the else scope, why was it tabbed?
Overall not a big fan of the determine_next_command addition, we do elseif there and then we do elseif again outside to check the determine_next_command's result. |
It started out much simpler, but with a bunch of other PRs getting pushed
on top, and a bunch of refactoring I'm working on merging in from
another reviewer, things get way more complicated.
Still working on the PR (week 5 and counting).
…On Tue, May 9, 2023 at 3:59 PM k-boikov ***@***.***> wrote:
Overall not a big fan of the determine_next_command addition, we do elseif
there and then we do elseif again outside to check the
determine_next_command's result.
—
Reply to this email directly, view it on GitHub
<#2481 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVTCVBKGOPKE2ZZHMVL42TXFKO2DANCNFSM6AAAAAAXDN7S2Y>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
To be fair, some of this was suggested by other folks, too - and some of it may even be covered by similar PRs. So maybe this needs a little more planning/coordination. Personally, I can definitely see the need for this! Even moreso because the current design has zero support for persistence, let alone for recovering a workspace properly. Usually, there's a ton of work (API tokens + time) going to the wastebin once we realize that the agent went off an irrelevant trajectory. So given the state of affairs, the feature itself could be useful regardless of stylistic disagreements at the code level |
I'm going to work to get everything running today, so there should be a
(somewhat) cleaner PR here later today.
Appreciate all the help and comments.
…On Thu, May 11, 2023 at 8:10 AM Boostrix ***@***.***> wrote:
To be fair, some of this was suggested by other folks, too - and some of
it may even be covered by similar PRs. So maybe this needs a little more
planning/coordination. Personally, I can definitely see the need for this!
Even moreso because the current design has zero support for persistence,
let alone for recovering a workspace properly.
Thus, being able to pause an agent to "direct" it would seem like a good
compromise.
Usually, there's a ton of work (API tokens + time) going to the wastebin
once we realize that the agent went off an irrelevant trajectory.
So given the state of affairs, the feature itself could be useful
regardless of stylistic disagreements at the code level
—
Reply to this email directly, view it on GitHub
<#2481 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVTCVCTAQMRDNIZL2M2DC3XFTJNHANCNFSM6AAAAAAXDN7S2Y>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
Happy to jump on Discord and figure out how best to accomplish this. Just let me know (ericheikkila#1667) |
Hi, just a short update - thanks for taking the time to update your PR, your PR was today discussed on Discord by several devs and it's being assigned to be reviewed right now! So, given that there seem to be at least 5+ different PRs in the pipeline currently using similar but still a different method, it is likely that we're going to take parts from each and every one of these PRs, to ensure that the feature is well-aligned with the ongoing re-arch effort. This means, you will end up in the credits automagically - but won't have to do any more work from now on. If there are remaining questions, someone is going to contact you, the PR is being reviewed by @gravelBridge |
Understood. Looking forward to the smoke settling so I can get back to it.
:)
Thanks for the update.
…On Mon, May 15, 2023, 6:12 PM Boostrix ***@***.***> wrote:
Hi, just a short update - thanks for taking the time to update your PR,
your PR was today discussed on Discord by several devs and it's being
assigned to be reviewed right now!
So, given that there seem to be at least 5+ different PRs in the pipeline
currently using similar but still a different method, it is likely that
we're going to take parts from each and every one of these PRs, to ensure
that the feature is well-aligned with the ongoing re-arch effort.
This means, you will end up in the credits automagically - but won't have
to do any more work from now on.
If there are remaining questions, someone is going to contact you, the PR
is being reviewed by @gravelBridge <https://github.com/gravelBridge>
—
Reply to this email directly, view it on GitHub
<#2481 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVTCVCFVJHLNHGXOA4LZPTXGKS5HANCNFSM6AAAAAAXDN7S2Y>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hello! Thank you so much for your contribution and feature suggestion! I have finished implementing our version of this feature here: #4230. Once again, thank you! |
AWESOME!! Thanks again! |
Background
While running with the 'y -XXX' prompt, I found myself wanting to know how many steps had been performed, and how many were left out of how many I had authorized. Additionally, I wanted a way to interrupt the process without resorting to crashing the system or killing a process outright.
Changes
Changes are extracting some methods for the functionality I eventually want to change and create unit tests for same.
Documentation
Test Plan
Created unit tests for the changes to ensure current functionality and lay the foundation for future changes.
PR Quality Checklist
Ran the app and tested the following inputs: