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

chore: Moving Milvus client to PyMilvus #4907

Merged
merged 4 commits into from
Jan 8, 2025
Merged

Conversation

franciscojavierarceo
Copy link
Member

@franciscojavierarceo franciscojavierarceo commented Jan 8, 2025

What this PR does / why we need it:

This PR moves the Milvus Online Store implementation to the PyMilvus Client instead of using Connections, in addition to some other minor changes.

More specifically:

  1. sdk/python/feast/feature_store.py:
    • Changed the default value of the distance_metric parameter in the retrieve_online_documents method to "L2".
  2. sdk/python/feast/infra/online_stores/milvus_online_store/milvus.py:
    • Replaced the usage of pymilvus.orm.connections with MilvusClient for managing Milvus connections.
    • Added support for username and password configurations in MilvusOnlineStoreConfig.
    • Updated the MilvusOnlineStore class to use MilvusClient for creating and managing collections, including:
      • Creating collections with a composite key.
      • Preparing and creating indexes for vector fields.
      • Handling authentication and connection establishment using username and password.
      • Refactoring methods like _connect, _get_collection, online_write_batch, update, teardown, and retrieve_online_documents to use the new client-based approach.
  3. sdk/python/tests/integration/feature_repos/universal/online_store/milvus.py:
    • Replaced MilvusContainer with DockerContainer from testcontainers.core.
    • Added Docker client initialization and log waiting utility for Milvus container readiness.
  4. sdk/python/tests/integration/online_store/test_universal_online.py:
    • Re-enabled the previously commented-out integration test for retrieving online documents using Milvus.

Which issue(s) this PR fixes:

Another one in service of #4364

Misc

N/A

Signed-off-by: Francisco Javier Arceo <[email protected]>
@franciscojavierarceo franciscojavierarceo marked this pull request as ready for review January 8, 2025 20:47
@franciscojavierarceo franciscojavierarceo requested a review from a team as a code owner January 8, 2025 20:47
Copy link
Contributor

@redhatHameed redhatHameed left a comment

Choose a reason for hiding this comment

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

/lgtm

@franciscojavierarceo franciscojavierarceo merged commit 5f9b5b5 into master Jan 8, 2025
28 checks passed
dharmisha pushed a commit to nishantgaurav-dev/feast that referenced this pull request Jan 15, 2025
* chore: Moving Milvus client to PyMilvus

Signed-off-by: Francisco Javier Arceo <[email protected]>

* linted and switched implementation to pymilvus

Signed-off-by: Francisco Javier Arceo <[email protected]>

* adding updates for integration configuration

Signed-off-by: Francisco Javier Arceo <[email protected]>

* removing drop statement

Signed-off-by: Francisco Javier Arceo <[email protected]>

---------

Signed-off-by: Francisco Javier Arceo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants