Skip to content

Conversation

@srini047
Copy link
Contributor

@srini047 srini047 commented May 30, 2025

Related Issues

Proposed Changes:

Added run_async conversion for CohereDocumentEmbedder

How did you test it?

Added a separate test case for testing asyc feature.

Notes for the reviewer

To be merged after #1873 since dependencies are present here.

Checklist

@srini047 srini047 requested a review from a team as a code owner May 30, 2025 19:38
@srini047 srini047 requested review from vblagoje and removed request for a team May 30, 2025 19:38
@github-actions github-actions bot added integration:cohere type:documentation Improvements or additions to documentation labels May 30, 2025
@srini047 srini047 force-pushed the run_async_cohere_document_embedder branch 4 times, most recently from dd06724 to 49399c0 Compare May 30, 2025 20:45
@srini047 srini047 force-pushed the run_async_cohere_document_embedder branch from 49399c0 to 9077fbc Compare May 30, 2025 20:47
@sjrl
Copy link
Contributor

sjrl commented Jun 2, 2025

Hey @vblagoje let's wait for this PR to be reviewed and merged before making a release so we can include the run_async for the CohereTextEmbedder (which was added here) and CohereDocumentEmbedder in one go.

@srini047
Copy link
Contributor Author

srini047 commented Jun 3, 2025

Hey @vblagoje let's wait for this PR to be reviewed and merged before making a release so we can include the run_async for the CohereTextEmbedder (which was added here) and CohereDocumentEmbedder in one go.

Have raised PRs for Ollama integration as well. Both document and text embedder in case that's in eye for the next release.

@srini047 srini047 force-pushed the run_async_cohere_document_embedder branch from a8e1575 to 1caee03 Compare June 11, 2025 08:33
Copy link
Member

@vblagoje vblagoje left a comment

Choose a reason for hiding this comment

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

Hey @srini047 left some initial feedback - minor issues, but worth addressing imho

@srini047
Copy link
Contributor Author

Hey @srini047 left some initial feedback - minor issues, but worth addressing imho

Fixed it @vblagoje

@srini047 srini047 force-pushed the run_async_cohere_document_embedder branch from fc2900b to 4abb192 Compare June 11, 2025 10:32
@srini047 srini047 requested a review from vblagoje June 11, 2025 12:46
vblagoje
vblagoje previously approved these changes Jun 12, 2025
Copy link
Member

@vblagoje vblagoje 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, thanks for this contribution @srini047

@vblagoje vblagoje self-requested a review June 12, 2025 13:00
@vblagoje vblagoje dismissed their stale review June 12, 2025 13:00

missed an issue

Copy link
Member

@vblagoje vblagoje left a comment

Choose a reason for hiding this comment

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

@srini047 a small favour, while we are at it:

  • initialize AsyncClient in init, don't use asserts raise meaningful errors (see other components)
  • in both run methods use async client
    • in run sync use asyncio to bridge
    • in run async use current approach
  • Flag with log warn use_async_client param as deprecated and ignore it

This is a legacy code before we have async method and we might as well remove it now 👏

@srini047
Copy link
Contributor Author

srini047 commented Jun 12, 2025

@srini047 a small favour, while we are at it:

  • initialize AsyncClient in init, don't use asserts raise meaningful errors (see other components)

  • in both run methods use async client

    • in run sync use asyncio to bridge
    • in run async use current approach
  • Flag with log warn use_async_client param as deprecated and ignore it

This is a legacy code before we have async method and we might as well remove it now 👏

@vblagoje
[EDIT] Addressed the first concern.
For the second/third point I think its better to address it seperately because this logic is also present in the CohereTextEmbedder as well. So I feel a refactor of Cohere integration on the whole in this sync/async area should be better. WDYT?

@vblagoje
Copy link
Member

Yes, very good point let's just resolve the test failures now and integrate this one

@srini047 srini047 requested a review from vblagoje June 13, 2025 16:48
@srini047 srini047 force-pushed the run_async_cohere_document_embedder branch from e68d8d7 to 73e94ac Compare June 13, 2025 17:05
@anakin87 anakin87 changed the title feat: add run_async support for CohereDocumentEmbedder feat: add async support for CohereDocumentEmbedder Jun 16, 2025
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

@srini047 thanks again for the contribution!

I'm now merging this PR and then rebasing #1946 on your work.

@anakin87 anakin87 dismissed vblagoje’s stale review June 16, 2025 14:35

We agreed on this

@anakin87 anakin87 merged commit f823609 into deepset-ai:main Jun 16, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration:cohere type:documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add run_async to CohereDocumentEmbedder

4 participants