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

Refactor OpenAI exception handling from instruction parser #328

Merged
merged 8 commits into from
Sep 7, 2023

Conversation

saum7800
Copy link
Collaborator

@saum7800 saum7800 commented Sep 6, 2023

Description

Currently the OpenAIInstructionParser handles OpenAI exceptions from its own class. With this change, the exception is handled as required directly from the ChatGPTAgent. Refactoring the exception handling is a step in the direction of converting the OpenAIInstructionParser to a more general InstructionParser class that can be passed any Generative Agent (OpenAI, cohere, local hf models, etc).

Key Changes

  • handle_openai_error returns the handled error instead of the number of API calls
  • OpenAIInstructionParser checks for the response being an exception or an actual response and continues the same logic as previously specified. This pattern will be extendable to future models.
  • prompt_parser test modified to mock the exact function that would be throwing the error in use.

Copy link
Collaborator

@neubig neubig left a comment

Choose a reason for hiding this comment

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

Hey @saum7800 , apologies for the hassle, but #324 probably introduced a breaking change, so could you merge and make any necessary edits to this PR?

@krrishdholakia
Copy link
Contributor

oh - what was the breaking change? Happy to take a look as well @neubig

@neubig
Copy link
Collaborator

neubig commented Sep 6, 2023

No worries @krrishdholakia , probably just a minor conflict: https://github.com/neulab/prompt2model/pull/328/conflicts

@saum7800
Copy link
Collaborator Author

saum7800 commented Sep 7, 2023

ah yes, just a minor conflict, will update my PR

@krrishdholakia
Copy link
Contributor

Ok, sounds good. Thanks for the merge!

@saum7800
Copy link
Collaborator Author

saum7800 commented Sep 7, 2023

Updated the PR. Feel free to review whenever you get a chance @viswavi / @neubig

@saum7800 saum7800 requested a review from neubig September 7, 2023 04:02
Copy link
Collaborator

@neubig neubig left a comment

Choose a reason for hiding this comment

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

I have one suggestion!

prompt2model/prompt_parser/instr_parser.py Show resolved Hide resolved
Copy link
Collaborator

@neubig neubig left a comment

Choose a reason for hiding this comment

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

Thanks, this was much clearer!

@neubig neubig merged commit 072f36f into main Sep 7, 2023
8 checks passed
@neubig neubig deleted the handle_exception_in_agent_saumya branch September 7, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants