-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add CodexBackup class #32
base: main
Are you sure you want to change the base?
Conversation
if self._primary_system is not None: | ||
self._backup_handler( | ||
codex_response=codex_response, | ||
primary_system=self._primary_system, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I was creating a RAG system for a production app, I think I'd be much more likely to implement handling replacement of original response either just within the chat method or as a separate instance method of the class. Without this CodexBackup
class, I'd probably do something along the lines of:
class RAGChatWithCodexBackup(RAGChat):
def __init__(self, client: OpenAI, assistant_id: str, codex_access_key: str):
super().__init__(client, assistant_id)
self._codex_project = Project.from_access_key(access_key)
def replace_latest_message(self, new_message: str) -> None:
<code from your handle_backup_for_openai_assistants method>
...
def chat(self, user_message: str) -> str:
response = super().chat(user_message)
codex_response: str | None = None
if is_bad_response(response=response, query=user_message):
codex_response = self._codex_project.query(user_message)
if codex_response is None:
return response
self.replace_latest_message(codex_response)
return codex_response
Very unlikely that I'd define a method that fits the expected signature for _backup_handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the CodexBackup
class you've defined without providing backup_handler
and primary_system
, I'd probably end up with the following:
class RAGChatWithCodexBackup(RAGChat):
def __init__(self, client: OpenAI, assistant_id: str, codex_access_key: str):
super().__init__(client, assistant_id)
self._codex_backup = CodexBackup.from_project(Project.from_access_key(codex_access_key))
def replace_latest_message(self, new_message: str) -> None:
<code from your handle_backup_for_openai_assistants method>
...
def chat(self, user_message: str) -> str:
response = super().chat(user_message)
backup_response: str | None = self._codex_backup.run(response=response, query=user_message)
if backup_response is not None and backup_response != response:
self.replace_latest_message(backup_response)
return backup_response
return response
which does save a couple lines of code, but not much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering:
a) Whether it's worth trying to do the backup_handler
stuff within this class (maybe could modify the expected function signature to allow for class instance methods).
b) Whether it's worth having this class right now (doesn't really save much code). But could potentially make the second example a little cleaner by modifying CodexBackup.run()
to return a pair of backup_response, codex_used
and then could just do if codex_used: self.replace_latest_message(backup_response)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed on slack. Will leave this implementation for now to avoid extra work of updating tutorials before soft launch deadline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm putting this PR on hold then.
Moving the code into the tutorial now.
tlm: Optional[TLM] = None, | ||
is_bad_response_kwargs: Optional[dict[str, Any]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for tlm
being a separate argument but the rest of the arguments for is_bad_response
being grouped into is_bad_response_kwargs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be is_bad_response_config: BadResponseDetectionConfig
. You're right, the tlm
argument (and fallback_answer
) should be fetched from the config instead.
if self._primary_system is not None: | ||
self._backup_handler( | ||
codex_response=codex_response, | ||
primary_system=self._primary_system, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed on slack. Will leave this implementation for now to avoid extra work of updating tutorials before soft launch deadline.
Followup to #31.
Files copied from #11.
Specifically, from: 49f9a9d