feat: BigtableLoader and BigtableSaver implementation #5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
product-auto-label
bot
added
the
api: bigtable
kurtisvg
requested changes
Feb 6, 2024
docs/document_loader.ipynb
Outdated
| "## Pre-reqs" | ||
| "## Setting up\n", | ||
| "\n", | ||
| "To run this notebook, you will need a Google Cloud Project, a Bigtable instance, and Google credentials." |
There was a problem hiding this comment.
Should include link to instructions at least.
docs/document_loader.ipynb
Outdated
| ] | ||
| }, | ||
| { | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "### Load from table" | ||
| "You can fetch the documents by calling the `load` method of the loader. It will return a list with all the documents. If you want to avoid this blocking call, you can call `lazy_load` method that returns an Iterator." |
There was a problem hiding this comment.
I think we should encourage lazy_load -- load is only around for legacy reasons IIUC
docs/document_loader.ipynb
Outdated
Comment on lines
107
to
110
| "docs_iterator = loader.lazy_load()\n", | ||
| "for doc in docs_iterator:\n", | ||
| " print(doc)\n", | ||
| " break" |
There was a problem hiding this comment.
nit: should probably just be: for doc in loader.lazy_load()
kurtisvg
requested changes
Feb 7, 2024
Comment on lines
48
to
63
| class MetadataMapping: | ||
| def __init__( | ||
| self, | ||
| column_family: str, | ||
| column_name: str, | ||
| metadata_key: str, | ||
| encoding: Encoding, | ||
| custom_encoding_func: Callable[[Any], bytes] = _not_implemented, | ||
| custom_decoding_func: Callable[[bytes], Any] = _not_implemented, | ||
| ): | ||
| self.column_family = column_family | ||
| self.column_name = column_name | ||
| self.metadata_key = metadata_key | ||
| self.encoding = encoding | ||
| self.custom_encoding_func = custom_encoding_func | ||
| self.custom_decoding_func = custom_decoding_func |
There was a problem hiding this comment.
nit: could we just use a dataclass here? https://docs.python.org/3/library/dataclasses.html
|
|
||
| from langchain_google_bigtable.document_loader import BigtableLoader, BigtableSaver | ||
|
|
||
| __all__ = ["BigtableLoader", "BigtableSaver"] |
There was a problem hiding this comment.
Do we need to expose Encoding + Metadata mapping here?
| metadata_mappings: Optional. The array of mappings that maps from Bigtable columns to keys on the metadata dictionary, including the encoding to use when mapping from Bigtable bytes to a python type. | ||
| """ | ||
| self.client = ( | ||
| (client or bigtable.Client(admin=True)) |
There was a problem hiding this comment.
should this be using the default client?
kurtisvg
approved these changes
Feb 7, 2024
| .table(table_id) | ||
| ) | ||
| if content_encoding not in SUPPORTED_DOC_ENCODING: | ||
| raise ValueError(f"{content_encoding} not in {SUPPORTED_DOC_ENCODING}") |
There was a problem hiding this comment.
nit:
Suggested change
| raise ValueError(f"{content_encoding} not in {SUPPORTED_DOC_ENCODING}") | |
| raise ValueError(f"content_encoding '{content_encoding}' not supported for content (must be {SUPPORTED_DOC_ENCODING}") |
There was a problem hiding this comment.
Done in follow up PR
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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> 馃