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 Cohere retriever #19771

Merged
merged 6 commits into from Mar 31, 2024

Conversation

giannis2two
Copy link
Contributor

  • Replace source_documents with documents
  • Pass documents as a named arg vs keyword
  • Make parsed_docs more robust
  • Fix edge case of doc page_content being None

@efriis efriis added the partner label Mar 29, 2024
@efriis efriis self-assigned this Mar 29, 2024
Copy link

vercel bot commented Mar 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Mar 29, 2024 9:30pm

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. Ɑ: retriever Related to retriever module 🔌: cohere Primarily related to Cohere integrations 🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature labels Mar 29, 2024
query: str,
*,
run_manager: CallbackManagerForRetrieverRun,
documents: Optional[List[Dict[str, str]]] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

ooc why are we passing documents into the retriever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's being passed in through **kwargs either way (example here at the bottom). I am pulling it out here to make a check for the connectors. The Cohere SDK throws an error when both a connector and documents are specified, so passing None for connectors if documents are specified.

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Mar 31, 2024
@baskaryan baskaryan merged commit 8cf1d75 into langchain-ai:master Mar 31, 2024
21 checks passed
marlenezw pushed a commit to marlenezw/langchain that referenced this pull request Apr 2, 2024
* Replace `source_documents` with `documents`
* Pass `documents` as a named arg vs keyword
* Make `parsed_docs` more robust
* Fix edge case of doc page_content being `None`
hinthornw pushed a commit that referenced this pull request Apr 26, 2024
* Replace `source_documents` with `documents`
* Pass `documents` as a named arg vs keyword
* Make `parsed_docs` more robust
* Fix edge case of doc page_content being `None`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature 🔌: cohere Primarily related to Cohere integrations lgtm PR looks good. Use to confirm that a PR is ready for merging. partner Ɑ: retriever Related to retriever module size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants