+
Skip to content

Conversation

baitsguy
Copy link
Contributor

Ran locally and logged the opposite condition to make sure it's working

@baitsguy baitsguy requested a review from austintlee January 30, 2025 19:22
@baitsguy baitsguy marked this pull request as ready for review January 30, 2025 19:22
Copy link

@austintlee austintlee left a comment

Choose a reason for hiding this comment

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

Looks good. A couple of suggestions.

response = super().search(**kwargs)
shards = response.get("_shards", {})
if shards.get("total") != shards.get("successful"):
logger.error(f"OpenSearch query skipped shards: {response}")

Choose a reason for hiding this comment

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

It might be helpful to fish out "query" from kwargs (if it's present) and log it along with the response? Do you think we need to log the entire response payload? It could be big.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave this in for now while we're debugging so we know which results actually got back. If it happens frequently enough and we handle it we can clean

@baitsguy baitsguy enabled auto-merge (squash) January 30, 2025 21:24
import logging
from typing import Optional, Any

from opensearchpy import OpenSearch
Copy link
Collaborator

Choose a reason for hiding this comment

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

here's the unconditional import


@requires_modules("opensearchpy", extra="opensearch")
def query(self, query: OpenSearchQuery) -> OpenSearchQueryResult:
from opensearchpy import OpenSearch
Copy link
Collaborator

Choose a reason for hiding this comment

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

prob need to keep the import sycamore.connectors.opensearch.utils here instead of at the top

llm: Optional[Union[LLM, str]] = None,
query_plan_strategy: Optional[QueryPlanStrategy] = None,
):
from opensearchpy import OpenSearch
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here

@classmethod
@requires_modules("opensearchpy", extra="opensearch")
def from_client_params(cls, params: BaseDBReader.ClientParams) -> "OpenSearchReaderClient":
from opensearchpy import OpenSearch
Copy link
Collaborator

Choose a reason for hiding this comment

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

also here

Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
@baitsguy baitsguy merged commit 1a33a86 into main Jan 31, 2025
13 of 15 checks passed
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.

3 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载