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

Fix: re-add service to fix integration with LangChain. #603

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

dblock
Copy link
Member

@dblock dblock commented Nov 17, 2023

Description

Re-add .service, which is used by LangChain to figure out whether we're talking to AOSS. This broke in #547.

Issues Resolved

Closes #600

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@navneet1v
Copy link

navneet1v commented Nov 17, 2023

@dblock its not necessary related to AOSS. Even in AOS/OSS, if user is asking for OpenSearch service to generate the ids for the data, os client should not send _id in the bulk request.

@dblock
Copy link
Member Author

dblock commented Nov 17, 2023

@dblock its not necessary related to AOSS. Even in AOS/OSS, if user is asking for OpenSearch service to generate the ids for the data, os client should not send _id in the bulk request.

The code is pretty explicit :)

def _is_aoss_enabled(http_auth: Any) -> bool:
    """Check if the service is http_auth is set as `aoss`."""
    if (
        http_auth is not None
        and hasattr(http_auth, "service")
        and http_auth.service == "aoss"
    ):
        return True
    return False

Signed-off-by: dblock <dblock@amazon.com>
@navneet1v
Copy link

@dblock its not necessary related to AOSS. Even in AOS/OSS, if user is asking for OpenSearch service to generate the ids for the data, os client should not send _id in the bulk request.

The code is pretty explicit :)

def _is_aoss_enabled(http_auth: Any) -> bool:
    """Check if the service is http_auth is set as `aoss`."""
    if (
        http_auth is not None
        and hasattr(http_auth, "service")
        and http_auth.service == "aoss"
    ):
        return True
    return False

This code is explicit, because AOSS doesn't support _id. But given that client is generic and AOSS support _id for search collection. So if we make this change then search collections will break.

@dblock

This is the reason why I am saying the expectation from opensearch-py is if user has not provided _id, client should not add of its own whether it is AOS, OSS or AOSS.

@dblock
Copy link
Member Author

dblock commented Nov 17, 2023

This is the reason why I am saying the expectation from opensearch-py is if user has not provided _id, client should not add of its own whether it is AOS, OSS or AOSS.

Agreed. It does not. Am I missing anything?

@dblock dblock merged commit 7509a15 into opensearch-project:main Nov 17, 2023
54 checks passed
@dblock dblock deleted the fix-langchain branch November 17, 2023 21:09
roma2023 pushed a commit to roma2023/opensearch-py that referenced this pull request Dec 28, 2023
…rch-project#603)

Signed-off-by: dblock <dblock@amazon.com>
Signed-off-by: roma2023 <romasaparhan19@gmail.com>
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.

[BUG] LangChain OpenSearchVectorSearch broken with 2.4.1 w/AOSS
3 participants