Fix: json decoder issue(s)

[exy02]

Background

This PR helps address multiple issues regarding json decode issues (e.g. #1407 , #4091, #4052, etc)

Changes

1. Cleaned up update_running_summary() logic

• Pre-filter new_events for only non-'user' events
• Each event was not being manipulated at the list level, adjusted logic to modify each event within new_events
• Added try/except logic to handle json load errors and log said errors appropriately
• Implemented .pop() in lieu of .remove(), index-based removal ensures consistent deletion of each target event
• NOTE: Need a double check to confirm '"thoughts":' is the correct identifier when scanning event["content"]

2. Minor change to improve get_newly_trimmed_messages() (I can move this to a separate PR if needed)

• Index slicing seems more appropriate here rather than list comprehension

Documentation

Added in-code comments and left existing in-code unchanged

Test Plan

Tested locally by isolating the affected portion of code (used existing test scripts to help generate sample data and verify logic)

PR Quality Checklist

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

[vercel[bot]]

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
docs	⬜️ Ignored (Inspect)			May 13, 2023 1:06am

[exy02]

NOTE: Maybe worth moving the for-loop to a new function? This will make unit testing much easier in the long-run. Example:

def cleanup_new_events(new_events:List[Dict[str,str]]) -> List[Dict[str,str]]:
    """ """
    new_events = [event for event in new_events if event["role"].lower() != "user"]
    # Replace "assistant" with "you". This produces much better first person past tense results.
    for i, event in enumerate(new_events):
        if event["role"].lower() == "assistant":
            new_events[i]["role"] = "you"

            # Remove "thoughts" dictionary from "content"
            #TODO: Need a double check here to confirm '"thoughts":' is the correct identifier.
            if '"thoughts":' in event["content"]:
                try:
                    content_dict = json.loads(event["content"])
                    del content_dict["thoughts"]
                    new_events[i]["content"] = json.dumps(content_dict)
                except:
                    error_str = f"ERROR: invalid json format when attempting to remove 'thoughts' content from event: {event}"
                    logger.error(error_str)
                    continue
        elif event["role"].lower() == "system":
            new_events[i]["role"] = "your computer" 
    return new_events

[anonhostpi]

Duplicate efforts: #3996, #3923

You can find more on https://github.com/anonhostpi/AUTOGPT.TRACKERS/blob/main/TOPICS/0017.BUGS/JSON.md

[anonhostpi]

Definitely needs to be fixed. Just referencing other similar PRs

[exy02]

Duplicate efforts: #3996, #3923

You can find more on https://github.com/anonhostpi/AUTOGPT.TRACKERS/blob/main/TOPICS/0017.BUGS/JSON.md

Thanks @anonhostpi for linking the related/relevant PRs;

Note: Post-review, it looks like both #3996 and #3923 attempt to resolve the error via try/except. However, both PRs only resolve part of the issue.

Local testing suggests elif event["role"] == "user": new_events.remove(event) was causing unanticipated effects on the function as whole (hence why I removed .remove() logic and reverted .pop() logic). Instead list comprehension was implemented to pre-filter the events and exclude all "user" events.

Though I do like the use of except json.decoder.JSONDecodeError observed in both related PRs, which can be added this PR if needed.

[github-actions[bot]]

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

[Pwuts]

@erik-megarad

[Pwuts]

Thanks for this, we definitely had some issues with this component in the beginning. The "summary memory" was re-implemented by #4208, and as far as I can see that also addresses the issues fixed by your PR. As such, I'm closing this. With a big thanks for the submission, and an apology for not taking it in earlier!