feat: BigtableChatMessageHistory implementation #6
Conversation
docs/chat_message_history.ipynb
Outdated
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "!pip install google-cloud-bigtable" |
There was a problem hiding this comment.
| "!pip install google-cloud-bigtable" | |
| "%pip install langchain-google-bigtable" |
| self, | ||
| instance_id: str, | ||
| table_id: str, | ||
| session_id: Optional[str] = None, |
There was a problem hiding this comment.
session id is required
product-auto-label
bot
added
the
api: bigtable
docs/chat_message_history.ipynb
Outdated
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "To run this notebook, you will need a Google Cloud Project, a Bigtable instance, and Google credentials." |
There was a problem hiding this comment.
Can we provide some links to how to create or set these things?
docs/chat_message_history.ipynb
Outdated
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "## Setting up" |
There was a problem hiding this comment.
nit: In Cloud Docs we call this "Before you begin" -- should we be consistent here?
docs/chat_message_history.ipynb
Outdated
| ] | ||
| }, | ||
| { | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "## Pre-reqs" | ||
| "## Setting up the schema\n", |
There was a problem hiding this comment.
| "## Setting up the schema\n", | |
| "## Initializing schema\n", |
docs/chat_message_history.ipynb
Outdated
| "## Pre-reqs" | ||
| "## Setting up the schema\n", | ||
| "\n", | ||
| "The chat history will be written to a column called `history` in a column family called `langchain`. If this column family does not exist in your table, you will need to call init_schema:" |
There was a problem hiding this comment.
Does this imply the table should be already created? Or will init_schema all create the table?
There was a problem hiding this comment.
The table is not created by init_schema, only the column family. I don't mind creating the table as well, though, but at some point we said we shouldn't. I have no preference :)
There was a problem hiding this comment.
Table created in ctor.
| client: Optional[bigtable.Client] = None, | ||
| ) -> None: | ||
| self.client = ( | ||
| (client or bigtable.Client(admin=True)) |
There was a problem hiding this comment.
Can we memoize the bigtable Client between multiple integrations?
| def clear(self) -> None: | ||
| """Clear session memory from DB""" | ||
| row_key_prefix = self.session_id | ||
| self.client.drop_by_prefix(row_key_prefix, timeout=200) |
There was a problem hiding this comment.
should timeout be configurable?
There was a problem hiding this comment.
dropped it altogether. It probably shouldn't be configurable, as this operation should be quick enough.
docs/chat_message_history.ipynb
Outdated
| ] | ||
| }, | ||
| { | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "## Basic Usage" | ||
| "## Cleaning up\n", |
There was a problem hiding this comment.
Would it flow better to add this section to the bottom of "Basic Usage"? It would then be initialize ChatMessageHistory -> add messages -> show messages -> clear messages This way you don't need to initialize another class object and can just use the one you already have...
message_history.clear()
docs/chat_message_history.ipynb
Outdated
| ")\n", | ||
| "\n", |
There was a problem hiding this comment.
Can we show the initialization of the table here?
| def __get_default_client(self) -> bigtable.Client: | ||
| global default_client | ||
| if default_client is None: | ||
| default_client = bigtable.Client(admin=True) | ||
| return default_client |
There was a problem hiding this comment.
nit: I don't think this should be attached to the class -- I think we want to use it in all classes
There was a problem hiding this comment.
Will be done after this PR is merged
| self.session_id = session_id | ||
| self.__init_schema() |
There was a problem hiding this comment.
We should expose this to the customer and ask them to do it. It can still be a class method -- but should probably handle create table as well.
docs/chat_message_history.ipynb
Outdated
| "init_schema(\n", | ||
| " instance_id=\"my-instance\",\n", | ||
| " table_id=\"my-table\",\n", | ||
| " client=bigtable.Client(...),\n", |
There was a problem hiding this comment.
nit: Probably best to recommend declaring the client ahead of time, so it's hinted at it should be reused.
| default_client: Optional[bigtable.Client] = None | ||
|
|
||
|
|
||
| def init_schema( |
There was a problem hiding this comment.
suggest renaming function -- "create_chat_history_table" or similar
ron-gal
requested review from
averikitsch
and removed request for
averikitsch
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕