-
Notifications
You must be signed in to change notification settings - Fork 6
docs: add docloader codelab #18
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
Conversation
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.
quick first pass, havent looked at notebook yet
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.
@loeng2023 A few things on my second pass.
- I think we want to simplify this notebook and try to remove as many SQLAlchemy engine execution statements as possible. Ideally all
.execute
commands happen internally in our library and users in this notebook only use our APIs. - Right now document saver and loader are quite intermingled. We should clean this up so that we follow the outline given by Averi. Basic usage should cover all DocLoader APIs and only show "load via table name" and "load via query". All document saver API ref and examples should be moved to
Advanced Usage
docs/document_loader.ipynb
Outdated
"cell_type": "markdown", | ||
"metadata": {}, | ||
"source": [ | ||
"Save langchain documents with `MySQLDocumentSaver.add_documents(<documents>)`." |
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.
based on Averi's notebook doc Basic Usage
should only cover the following:
- Setting up connection object, if needed
- Load via table name
- Load via query
Document saver should be in Advanced Usage
.
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.
But we cannot load documents without saving them first. Moving saver into basic usage makes the demo experience smoother.
e1c8dc7
to
3176d57
Compare
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.
LGTM, two small nits in my previous comment
597f9fc
to
fd4b678
Compare
https://github.com/googleapis/langchain-google-cloud-sql-mysql-python/blob/doc-loader-doc/docs/document_loader.ipynb