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

Fix side effects on message history #3619

Merged
merged 4 commits into from
May 1, 2023
Merged

Fix side effects on message history #3619

merged 4 commits into from
May 1, 2023

Conversation

AbTrax
Copy link
Contributor

@AbTrax AbTrax commented May 1, 2023

Background

An issue was caused by the update_running_summary function, which was directly modifying the new_events list passed as an argument. Since the new_events list is a part of the full_message_history, any modifications made to the new_events list would also affect the original full_message_history.

Changes

Create a copy of the new_events list: To avoid modifying the original new_events list and full_message_history, a copy of the new_events list was created using new_events.copy(), and the copy was named updated_new_events. Then added updated_new_events to the prompt.

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 May 1, 2023

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview May 1, 2023 1:06pm

@github-actions github-actions bot added the size/m label May 1, 2023
@AbTrax AbTrax marked this pull request as ready for review May 1, 2023 11:35
richbeales
richbeales previously approved these changes May 1, 2023
@vercel vercel bot temporarily deployed to Preview May 1, 2023 12:06 Inactive
@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (d8968ae) 59.98% compared to head (21c2910) 60.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3619      +/-   ##
==========================================
+ Coverage   59.98%   60.01%   +0.02%     
==========================================
  Files          69       69              
  Lines        3099     3101       +2     
  Branches      513      445      -68     
==========================================
+ Hits         1859     1861       +2     
  Misses       1109     1109              
  Partials      131      131              
Impacted Files Coverage Δ
autogpt/memory_management/summary_memory.py 88.23% <100.00%> (+0.73%) ⬆️

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

@Pwuts Pwuts added bug Something isn't working high priority labels May 1, 2023
@Pwuts Pwuts self-assigned this May 1, 2023
@github-actions github-actions bot added size/m and removed size/s labels May 1, 2023
@Pwuts
Copy link
Member

Pwuts commented May 1, 2023

  • copy -> deepcopy
  • Improved code readability

Copy link
Member

@Pwuts Pwuts left a comment

Choose a reason for hiding this comment

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

LGTM

@Pwuts Pwuts changed the title Full message history fix Fix side effects on message history May 1, 2023
@Pwuts Pwuts merged commit 34261a1 into Significant-Gravitas:master May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority size/m
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants