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

Update Conversational Chat output_parser.py to support more final response cases #4539

Closed

Conversation

treppers
Copy link

@treppers treppers commented May 11, 2023

In heavily utilizing the conversational chat agent, I found that it didn't account for the many potential outputs that the agent could respond with.

The use-cases this now supports are:

  • Instances when it doesn't include the json after the markdown escape
  • Instances when the response includes special characters aren't JSON escaped
  • Instances when the response runs out of tokens and which then results in a malformed JSON response

Fixes # 2567
Fixes # 3448

Community members can review the PR once tests pass. Tag maintainers/contributors who might be interested:

@hwchase17
@mbchang

response = json.loads(cleaned_output)
try:
response = json.loads(cleaned_output)
except: # Response isn't JSON!

Choose a reason for hiding this comment

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

It's considered bad practice to 'catch the universal Exception. JSON decoder errors will typically return a ValueError, which includes the JSONDecodeError child class.

Copy link
Contributor

@hwchase17 hwchase17 left a comment

Choose a reason for hiding this comment

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

lets add test cases for this

@hwchase17 hwchase17 added the needs test PR needs to be updated with tests label May 18, 2023
@treppers
Copy link
Author

good idea. where's the best place to put them?

@treppers
Copy link
Author

@hwchase17 added a test for output_parser, but wasn't sure where to put it so for now just dumped it into the same directory. please let me know where to move it.

i merged with the latest release on master and also added a specific exception for JSON decoding.

it is slightly opinionated because it assumes the developer wants to see an output if tokens get exhausted or if the completion responds without any markdown which in my experience only happens when it is the final answer.

@baskaryan
Copy link
Collaborator

stale, @treppers feel free to ping if you return to it

@baskaryan baskaryan closed this Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs test PR needs to be updated with tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants