Skip to content

feat: Add document saver class and support save/load/delete user journeys. #14

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Feb 6, 2024
Merged

feat: Add document saver class and support save/load/delete user journeys. #14

merged 17 commits into from
Feb 6, 2024

Conversation

loeng2023
Copy link
Contributor

Add document saver class and support save/load/delete user journeys.

In detail:

  • Load documents via default table
  • Load documents via custom table/metadata
  • Load documents via custom page content columns
  • Load documents via custom metadata columns
  • Initialize MySQLDocumentSaver with existing table
  • Initialize custom MySQLDocumentSaver table & add documents with metadata
  • Delete documents

@loeng2023 loeng2023 requested a review from a team as a code owner January 31, 2024 00:41
@loeng2023 loeng2023 changed the title Doc saver feat: Add document saver class and support save/load/delete user journeys. Jan 31, 2024
@jackwotherspoon jackwotherspoon removed the request for review from a team January 31, 2024 13:37
jackwotherspoon
jackwotherspoon requested changes Jan 31, 2024
View reviewed changes
Copy link
Contributor

@jackwotherspoon jackwotherspoon left a comment

Choose a reason for hiding this comment

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

First quick pass, overall looks great 👏

@jackwotherspoon jackwotherspoon added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 31, 2024
@jackwotherspoon
Copy link
Contributor

Added do not merge label for now since this PR is quite large. Let's wait for the MySQLChatMessageHistory PR to get merged into main first before merging this into our staging branch.

@loeng2023
Copy link
Contributor Author

@jackwotherspoon @totoleon Added unit tests and confirmed text spilter is no-op. Code is ready for review.

totoleon
totoleon approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@totoleon totoleon left a comment

Choose a reason for hiding this comment

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

Overall LGTM with the comments resolved.

src/langchain_google_cloud_sql_mysql/mysql_loader.py

self._table = self.engine._load_document_table(self.table_name)

def add_documents(self, docs: List[Document]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do we want to do handle a single item as input as well as list? For example starts with

def add_documents(self, docs: Union[Document, List[Document]]):
    if isinstance(docs, Document):
        docs = [docs]

Same for delete

Base automatically changed from staging to main February 2, 2024 22:04
@jackwotherspoon jackwotherspoon removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 2, 2024
@jackwotherspoon
Copy link
Contributor

Removed do not merge label and switched base to main since this is a large enough PR to go right to main

@product-auto-label product-auto-label bot added the api: cloudsql-mysql Issues related to the googleapis/langchain-google-cloud-sql-mysql-python API. label Feb 6, 2024
@loeng2023 loeng2023 changed the base branch from main to staging February 6, 2024 18:07
@loeng2023 loeng2023 merged commit c36c7c1 into staging Feb 6, 2024
@loeng2023 loeng2023 deleted the doc-saver branch February 6, 2024 18:09
loeng2023 added a commit that referenced this pull request Feb 12, 2024
@loeng2023
feat: Add document saver class and support save/load/delete user jour…
0b53b29 
…neys. (#14)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackwotherspoon jackwotherspoon jackwotherspoon requested changes

@totoleon totoleon totoleon approved these changes

Assignees
No one assigned
Labels
api: cloudsql-mysql Issues related to the googleapis/langchain-google-cloud-sql-mysql-python API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@loeng2023 @jackwotherspoon @totoleon