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

Add CodexBackup class #32

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add CodexBackup class #32

wants to merge 3 commits into from

Conversation

elisno
Copy link
Member

@elisno elisno commented Feb 11, 2025

Followup to #31.


Files copied from #11.

Specifically, from: 49f9a9d

@elisno elisno requested a review from axl1313 February 11, 2025 22:53
Comment on lines +109 to +113
if self._primary_system is not None:
self._backup_handler(
codex_response=codex_response,
primary_system=self._primary_system,
)
Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Collaborator

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)

Copy link
Collaborator

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.

Copy link
Member Author

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.

Comment on lines +46 to +47
tlm: Optional[TLM] = None,
is_bad_response_kwargs: Optional[dict[str, Any]] = None,
Copy link
Collaborator

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?

Copy link
Member Author

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.

Comment on lines +109 to +113
if self._primary_system is not None:
self._backup_handler(
codex_response=codex_response,
primary_system=self._primary_system,
)
Copy link
Collaborator

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.

@jwmueller jwmueller marked this pull request as draft February 13, 2025 06:28
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.

2 participants