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

langchain_aws:documentdb_vectorstore with async capabilities #30

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Alonoparag
Copy link

Description

Adding documentdb_vectorestore from langchain_community, and adding async capabilities to it.

Dependencies

  • motor = "^3.3"
  • pymongo = "^4.6.3"

Twitter handle

@Sunshineallon

@Alonoparag
Copy link
Author

@3coins Would you kindly review?

@3coins
Copy link
Collaborator

3coins commented Apr 30, 2024

@Alonoparag
Thanks for submitting this change. This is a very thorough PR, great job on adding tests. I will do a more detailed review later this week, can you fix the CI errors.

Comment on lines 25 to 48
if TYPE_CHECKING:
from langchain_core.vectorstores import (
VectorStore, # noqa: F401
)

from langchain_aws.vectorstores.documentdb import (
DocumentDBVectorSearch, # noqa: F401
)
__all__ = [
"DocumentDBVectorSearch",
]

_module_lookup = {
"DocumentDBVectorSearch": "langchain_community.vectorstores.documentdb",
}


def __getattr__(name: str) -> Any:
if name in _module_lookup:
module = importlib.import_module(_module_lookup[name])
return getattr(module, name)
raise AttributeError(f"module {__name__} has no attribute {name}")


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 need for this setup?

Copy link
Author

@Alonoparag Alonoparag May 2, 2024

Choose a reason for hiding this comment

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

@3coins I copied the setup of langchain_community/vectorstores/__init__.py, but AFAIK it's redundant.
CI errors should be resolved with latest commits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case, we can just use a regular import for the DocumentDBVectorSearch, and remove the rest of the code.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@3coins 3coins left a comment

Choose a reason for hiding this comment

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

@Alonoparag
Thanks for fixing the CI. There seems to be some references to langchain_community, please remove and update them to refer to the current package. Also, make sure you are able to run and execute the integration tests successfully.

Comment on lines 25 to 48
if TYPE_CHECKING:
from langchain_core.vectorstores import (
VectorStore, # noqa: F401
)

from langchain_aws.vectorstores.documentdb import (
DocumentDBVectorSearch, # noqa: F401
)
__all__ = [
"DocumentDBVectorSearch",
]

_module_lookup = {
"DocumentDBVectorSearch": "langchain_community.vectorstores.documentdb",
}


def __getattr__(name: str) -> Any:
if name in _module_lookup:
module = importlib.import_module(_module_lookup[name])
return getattr(module, name)
raise AttributeError(f"module {__name__} has no attribute {name}")


Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case, we can just use a regular import for the DocumentDBVectorSearch, and remove the rest of the code.

Example:
. code-block:: python

from langchain_community.vectorstores import DocumentDBVectorSearch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update this to refer to langchain_aws. There are other similar references in the PR, please correct those.

Comment on lines 62 to 63
cd libs/community
make test TEST_FILE=tests/integration_tests/vectorstores/test_documentdb.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs an update to refer to langchain-aws. Were you able to run these tests and confirm these run successfully after copying the files here?

Copy link
Author

Choose a reason for hiding this comment

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

Changed

@3coins
Copy link
Collaborator

3coins commented May 3, 2024

@efriis
Would the integration tests here need any env setup to run successfully in the CI and during release?

@efriis
Copy link
Member

efriis commented May 3, 2024

@efriis Would the integration tests here need any env setup to run successfully in the CI and during release?

They definitely would! You can add any secrets to the Actions secrets in this repo, and you have to make sure they get passed through during the integration testing step in _release.yml

Happy to help if you need!

@Alonoparag
Copy link
Author

@3coins Seems like the integration tests runner did not install motor.
How would you usually instruct it to install? rerun poetry lock?

@Alonoparag
Copy link
Author

@3coins the missing packages (motor, pymongo) are marked as extra. Can we change the CI file to install extras when the test is compiled?

@3coins
Copy link
Collaborator

3coins commented May 21, 2024

@Alonoparag
Trying to find the right POC in the DocumentDB team to review this, also evaluating what's the best way to handle the infrastructure creation for the integration tests. Thanks for your patience.

@mohittalele
Copy link

any update on this pr ?

@Alonoparag
Copy link
Author

@mohittalele Still waiting for approval

@Alonoparag
Copy link
Author

@3coins Any updates? The CI currently fails due to unrelated code (Bedrock). I've been using the added vectorstore myself in production since April, and I see no reason why it shouldn't be added to langchain-aws

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.

5 participants