Skip to content

Fix memory by adding it only when context window full #3469

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 28, 2023
Merged

Fix memory by adding it only when context window full #3469

merged 2 commits into from
Apr 28, 2023

Conversation

waynehamadi
Copy link
Contributor

@waynehamadi waynehamadi commented Apr 28, 2023 •
edited
Loading

Background

Currently the memory is added at the cycle number 2. it doesn't make sense because we don't need memory if the context window is small.

Changes

add the memory only when the context window is full. We happen to have a while loop that does exactly that, so I just inserted the memory in there.

Also, we only want to add memory when we hit a json reply, not a command system or user input.
When we hit an assistant reply, we look at the message after it and we see if it's a user input or Command result.
We then append the assistant reply and user input or command result

Documentation

Test Plan

PR Quality Checklist

  • My pull request is atomic and focuses on a single change.
  • I have thoroughly tested my changes with multiple different prompts.
  • I have considered potential risks and mitigations for my changes.
  • I have documented my changes clearly and comprehensively.
  • I have not snuck in any "extra" small tweaks changes

@vercel
Copy link

vercel bot commented Apr 28, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Apr 28, 2023 7:47pm

@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from bb8c356 to 442b2d6 Compare April 28, 2023 15:00
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from 442b2d6 to 46e040a Compare April 28, 2023 15:05
@github-actions
Copy link
Contributor

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

1 similar comment
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from 954ff6c to afe1b69 Compare April 28, 2023 15:10
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from afe1b69 to fb2de7b Compare April 28, 2023 15:13
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from fb2de7b to a60160c Compare April 28, 2023 15:20
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from a60160c to e286c23 Compare April 28, 2023 15:27
@github-actions
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Apr 28, 2023 •
edited
Loading

Codecov Report

Patch coverage: 90.32% and project coverage change: +0.42 🎉

Comparison is base (3b74d21) 56.46% compared to head (2610675) 56.88%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3469      +/-   ##
==========================================
+ Coverage   56.46%   56.88%   +0.42%     
==========================================
  Files          66       67       +1     
  Lines        3018     3043      +25     
  Branches      507      509       +2     
==========================================
+ Hits         1704     1731      +27     
+ Misses       1175     1174       -1     
+ Partials      139      138       -1
Impacted Files Coverage Δ
autogpt/llm/chat.py 65.16% <50.00%> (-0.35%) ⬇️
autogpt/json_utils/utilities.py 60.00% <83.33%> (+12.00%) ⬆️
autogpt/agent/agent.py 44.20% <100.00%> (-0.80%) ⬇️
autogpt/memory_management/store_memory.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from e286c23 to f561fb4 Compare April 28, 2023 16:19
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from f561fb4 to 34ce877 Compare April 28, 2023 17:17
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from 34ce877 to db5e2bf Compare April 28, 2023 17:18
@github-actions
Copy link
Contributor

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

1 similar comment
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from eaa3239 to 6983af5 Compare April 28, 2023 17:38
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from 6983af5 to a70bfd1 Compare April 28, 2023 17:40
@github-actions
Copy link
Contributor

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

@collijk collijk added bug Something isn't working function: memory labels Apr 28, 2023
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from ca9e911 to 19f3318 Compare April 28, 2023 17:51
@github-actions
Copy link
Contributor

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

2 similar comments
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

p-i-
p-i- reviewed Apr 28, 2023
View reviewed changes
@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from d42fdae to 6587d5c Compare April 28, 2023 18:47
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from 6587d5c to 139cb39 Compare April 28, 2023 18:49
@github-actions
Copy link
Contributor

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

@waynehamadi
Copy link
Contributor Author

Ok I applied the fixes @p-i-

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from 139cb39 to 10f5857 Compare April 28, 2023 18:59
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from 10f5857 to 5a0e13c Compare April 28, 2023 19:16
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from 5a0e13c to ab04020 Compare April 28, 2023 19:18
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from ab04020 to e893785 Compare April 28, 2023 19:22
@github-actions
Copy link
Contributor

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

Pwuts
Pwuts previously requested changes Apr 28, 2023
View reviewed changes
@waynehamadi waynehamadi force-pushed the feature/memory-not-needed-initially branch from e893785 to 2610675 Compare April 28, 2023 19:47
@github-actions
Copy link
Contributor

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

@waynehamadi waynehamadi requested a review from Pwuts April 28, 2023 19:56
@p-i- p-i- dismissed Pwuts’s stale review April 28, 2023 20:05

To get it out the door

@p-i- p-i- merged commit aa3e37a into Significant-Gravitas:master Apr 28, 2023
waynehamadi
waynehamadi commented Apr 28, 2023
View reviewed changes
autogpt/chat.py Outdated
@@ -126,6 +126,7 @@ def chat_with_ai(
[message_to_add], model
)
if current_tokens_used + tokens_to_add > send_token_limit:
save_memory_trimmed_from_context_window(full_message_history, next_message_to_add_index, permanent_memory)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

you will see above there is a while loop while next_message_to_add_index >= 0:

so basicallly if current_tokens_used + tokens_to_add > send_token_limit:
this means the context window is full.

autogpt/chat.py Outdated
memory_to_add = format_memory(thoughts_memory["content"], next_message["content"])
permanent_memory.add(memory_to_add)

next_message_to_add_index -= 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we just say "add to memory as long as you see a json reply". And we add the json reply but also the command or user input, like we did before

tests/integration/test_memory_management.py Outdated
def expected_permanent_memory():
return """Assistant Reply: {
"thoughts": {
"text":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

flake8 W291 was complaining about a space here but this is necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p-i- p-i- p-i- approved these changes

@richbeales richbeales richbeales approved these changes

@Pwuts Pwuts Awaiting requested review from Pwuts

Assignees
No one assigned
Labels
bug Something isn't working function: memory size/xl
Projects
AutoGPT development kanban
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@waynehamadi @p-i- @richbeales @Pwuts @collijk