Msudhir/add vector update functionality #14

Manisha4 · 2023-08-07T23:05:35Z

What this PR does / why we need it:

Added updated functionality to MilvusOnlineStore. The update functionality is able to delete and create a collection.
Relevant tests added as well

Which issue(s) this PR fixes:

Fixes #

Signed-off-by: Danny C <d.chiao@gmail.com>

update Catalog argument in athena connector Signed-off-by: Gyumin Lee <t1100394@T1100394PM01.local> Co-authored-by: Gyumin Lee <t1100394@T1100394PM01.local>

Signed-off-by: Danny C <d.chiao@gmail.com>

ensure correct precedence with the two operators Signed-off-by: Ben Fletcher <ben.fletcher@ft.com>

Signed-off-by: Jiwon Park <bakjeeone@hotmail.com>

…east-dev#3677) Bumps [tough-cookie](https://github.com/salesforce/tough-cookie) from 4.0.0 to 4.1.3. - [Release notes](https://github.com/salesforce/tough-cookie/releases) - [Changelog](https://github.com/salesforce/tough-cookie/blob/master/CHANGELOG.md) - [Commits](salesforce/tough-cookie@v4.0.0...v4.1.3) --- updated-dependencies: - dependency-name: tough-cookie dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [tough-cookie](https://github.com/salesforce/tough-cookie) from 4.0.0 to 4.1.3. - [Release notes](https://github.com/salesforce/tough-cookie/releases) - [Changelog](https://github.com/salesforce/tough-cookie/blob/master/CHANGELOG.md) - [Commits](salesforce/tough-cookie@v4.0.0...v4.1.3) --- updated-dependencies: - dependency-name: tough-cookie dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…east-dev#3630) * sql.py data_sources.data_source_name String(255) Extend the limit of the data_source_name field from 50 to 255. Signed-off-by: Ross Donnachie <code@radonn.co.za>

…east-dev#3680) feat: Optimize bytes processed when retrieving entity df schema to 0 Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>

…tore.plan() on python (feast-dev#3640) * fix! KeyError: __dummy on entityless fv Signed-off-by: williamfoschiera <william.foschiera@buser.com.br> * fix! join_keys typing. Signed-off-by: williamfoschiera <william.foschiera@buser.com.br> --------- Signed-off-by: williamfoschiera <william.foschiera@buser.com.br> Co-authored-by: williamfoschiera <william.foschiera@buser.com.br>

Bumps [protobufjs](https://github.com/protobufjs/protobuf.js) from 7.1.1 to 7.2.4. - [Release notes](https://github.com/protobufjs/protobuf.js/releases) - [Changelog](https://github.com/protobufjs/protobuf.js/blob/master/CHANGELOG.md) - [Commits](protobufjs/protobuf.js@protobufjs-v7.1.1...protobufjs-v7.2.4) --- updated-dependencies: - dependency-name: protobufjs dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…east-dev#3675) Bumps [protobufjs](https://github.com/protobufjs/protobuf.js) from 7.1.2 to 7.2.4. - [Release notes](https://github.com/protobufjs/protobuf.js/releases) - [Changelog](https://github.com/protobufjs/protobuf.js/blob/master/CHANGELOG.md) - [Commits](protobufjs/protobuf.js@protobufjs-v7.1.2...protobufjs-v7.2.4) --- updated-dependencies: - dependency-name: protobufjs dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [semver](https://github.com/npm/node-semver) from 6.3.0 to 6.3.1. - [Release notes](https://github.com/npm/node-semver/releases) - [Changelog](https://github.com/npm/node-semver/blob/v6.3.1/CHANGELOG.md) - [Commits](npm/node-semver@v6.3.0...v6.3.1) --- updated-dependencies: - dependency-name: semver dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…-dev#3679) Bumps [semver](https://github.com/npm/node-semver) from 6.3.0 to 6.3.1. - [Release notes](https://github.com/npm/node-semver/releases) - [Changelog](https://github.com/npm/node-semver/blob/v6.3.1/CHANGELOG.md) - [Commits](npm/node-semver@v6.3.0...v6.3.1) --- updated-dependencies: - dependency-name: semver dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

) Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.47.0 to 1.53.0. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.47.0...v1.53.0) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

# [0.32.0](feast-dev/feast@v0.31.0...v0.32.0) (2023-07-17) ### Bug Fixes * Added generic Feature store Creation for CLI ([feast-dev#3618](feast-dev#3618)) ([bf740d2](feast-dev@bf740d2)) * Broken non-root path with projects-list.json ([feast-dev#3665](feast-dev#3665)) ([4861af0](feast-dev@4861af0)) * Clean up snowflake to_spark_df() ([feast-dev#3607](feast-dev#3607)) ([e8e643e](feast-dev@e8e643e)) * Entityless fv breaks with `KeyError: __dummy` applying feature_store.plan() on python ([feast-dev#3640](feast-dev#3640)) ([ef4ef32](feast-dev@ef4ef32)) * Fix scan datasize to 0 for inference schema ([feast-dev#3628](feast-dev#3628)) ([c3dd74e](feast-dev@c3dd74e)) * Fix timestamp consistency in push api ([feast-dev#3614](feast-dev#3614)) ([9b227d7](feast-dev@9b227d7)) * For SQL registry, increase max data_source_name length to 255 ([feast-dev#3630](feast-dev#3630)) ([478caec](feast-dev@478caec)) * Implements connection pool for postgres online store ([feast-dev#3633](feast-dev#3633)) ([059509a](feast-dev@059509a)) * Manage redis pipe's context ([feast-dev#3655](feast-dev#3655)) ([48e0971](feast-dev@48e0971)) * Missing Catalog argument in athena connector ([feast-dev#3661](feast-dev#3661)) ([f6d3caf](feast-dev@f6d3caf)) * Optimize bytes processed when retrieving entity df schema to 0 ([feast-dev#3680](feast-dev#3680)) ([1c01035](feast-dev@1c01035)) ### Features * Add gunicorn for serve with multiprocess ([feast-dev#3636](feast-dev#3636)) ([4de7faf](feast-dev@4de7faf)) * Use string as a substitute for unregistered types during schema inference ([feast-dev#3646](feast-dev#3646)) ([c474ccd](feast-dev@c474ccd))

* Add fully-qualified-table-name Redshift prop Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> * pre-commit Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> * Docstring Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> * Test fully_qualified_table_name Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> * Simplify logic Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> * pre-commit Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> * pre-commit Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> * Test offline_write_batch Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> * Bump to trigger CI Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> * another bump for ci Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com> --------- Signed-off-by: Robin Neufeld <metavee@users.noreply.github.com>

…SA role (feast-dev#3696) Add aws-sts dependency in java sdk Signed-off-by: harmeet-singh-discovery <harmeet_singh@discovery.com>

…onality

…e-functionality" This reverts commit 8487678, reversing changes made to 0578b9b.

michaelbackes · 2023-08-09T18:06:46Z

sdk/python/feast/expediagroup/vectordb/milvus_online_store.py

+                collection_available = utility.has_collection(table_to_keep.name)
+                try:
+                    if collection_available:
+                        logger.error(f"Collection {table_to_keep.name} already exists.")


please make this log statement on "info" level. If a collection exists it is not an error and is expected.

michaelbackes · 2023-08-09T18:10:15Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+
+
+@dataclass
+class MockFeatureView:


is it really necessary to use a mock? Can you use a VectorFeatureView instead?

michaelbackes · 2023-08-09T18:25:14Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+
+        # Assert that logger.info was called with expected messages
+        expected_log_calls = [
+            mocker.call("Closing the connection to Milvus"),


verification through logs is not a good practice in unit tests. Look into methods of "connections" like get_connection_addr() https://github.com/milvus-io/pymilvus/blob/master/pymilvus/orm/connections.py#L396 to use for the assert statements

michaelbackes · 2023-08-09T18:27:43Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+        assert len(utility.list_collections()) == 1
+        assert utility.has_collection(self.collection_to_write) is True
+        assert (
+            f"Collection {self.collection_to_write} has been created successfully."


no need to assert a specific log statement here

michaelbackes · 2023-08-09T18:29:33Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+            partial=None,
+        )
+
+        assert f"Collection {self.collection_to_write} already exists." in caplog.text


assert with list_collections() == 1 instead

michaelbackes · 2023-08-09T18:31:57Z

sdk/python/tests/expediagroup/test_milvus_online_store.py


-    online_store_creator.teardown()
+        MilvusOnlineStore().update(


using the milvus online store here makes this test dependent on update() working as expected. Instead creating a collection via the pymilvus sdk directly would be better here to simulate that a collection already exists.

michaelbackes · 2023-08-09T18:38:05Z

sdk/python/feast/expediagroup/vectordb/milvus_online_store.py

+                    if collection_available:
+                        logger.error(f"Collection {table_to_keep.name} already exists.")
+                    else:
+                        Collection(name=table_to_keep.name, schema=table_to_keep.schema)


just catching this. FeatureView schemas are not the same as the schema that Milvus expects. We will need to write logic to translate that.

michaelbackes · 2023-08-09T18:40:36Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+            ]
+        )
+
+        MilvusOnlineStore().update(


here I would also rather create the collection first through the pymilvus sdk so that we do not introduce tests that are interdependent.

michaelbackes · 2023-08-09T18:41:34Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+        assert utility.has_collection(self.collection_to_delete) is False
+        assert len(utility.list_collections()) == 0
+        assert (
+            f"Collection {self.collection_to_delete} has been deleted successfully."


do not assert log-statements. If the wording changes one has to go and update the tests.

michaelbackes · 2023-08-09T18:43:23Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+        )
+
+        assert (
+            f"Collection {self.collection_to_delete} does not exist or is already deleted."


check with assert connections.has_connection(self.collection_to_delete) instead

…vus readable schema

michaelbackes · 2023-08-10T22:35:00Z

sdk/python/feast/expediagroup/vectordb/milvus_online_store.py

+                            f"Collection {table_to_delete.name} has been deleted successfully."
+                        )
+                    else:
+                        return logger.error(


remove return statement

michaelbackes · 2023-08-10T22:47:31Z

sdk/python/feast/expediagroup/vectordb/milvus_online_store.py

+            field_name = field.name
+            description = field.tags.get("description")
+            is_primary = boolean_mapping_from_string.get(field.tags.get("is_primary"))
+            dimension = field.tags.get("dimension")


dimensions have to be retrieved from VectorFeatureView. Also, the name of the column holding the vector is defined separately in VectorFeatureView ("vector_field")

michaelbackes · 2023-08-10T22:49:09Z

sdk/python/feast/expediagroup/vectordb/milvus_online_store.py

+            is_primary = boolean_mapping_from_string.get(field.tags.get("is_primary"))
+            dimension = field.tags.get("dimension")
+
+            if dimension is not None:


why is this needed?

michaelbackes · 2023-08-10T22:51:08Z

sdk/python/feast/expediagroup/vectordb/vector_feature_view.py

@@ -50,6 +50,7 @@ class VectorFeatureView(BaseFeatureView):

    # inheriting from FeatureView wouldn't work due to issue with conflicting proto classes
    # therefore using composition instead
+    name: str


feature_view already has an attribute name

It did not reflect and threw an error when I tried to add name

michaelbackes · 2023-08-10T22:54:01Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+
+    def setup_method(self, milvus_online_setup):
+        # Ensuring that the collections created are dropped before the tests are run
+        with PymilvusConnectionContext():


it's ok to re-use MilvusConnectionManager in order to not duplicate code

I want to keep the test not dependent on any functionality in Milvus online store. This context manager is a very simple open and close connection, purely for testing purposes. Will not add in username, password, etc.

michaelbackes · 2023-08-10T22:57:03Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+                tags={
+                    "is_primary": "False",
+                    "description": "float32",
+                    "dimension": "128",


thinking about it this is actually a great suggestion to use a tag to pass along the number of dimensions. I think we can do likewise with index algorithm and eliminate the need for a VectorFeatureView altogether. What do you think?

Yes, thats a good idea

michaelbackes · 2023-08-10T23:09:57Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+        )
+
+        # Here we want to open and add a collection using pymilvus directly and close the connection, we need to check if the collection count remains 1 and exists.
+        with PymilvusConnectionContext():


does the embedded Milvus actually persist data after it is stopped?

Yes it does

michaelbackes · 2023-08-10T23:13:29Z

sdk/python/tests/expediagroup/test_milvus_online_store.py

+            partial=None,
+        )
+
+        assert "Collection abc does not exist or is already deleted." in caplog.text


minor: I'd rather assert against Milvus not having any collection. The update() method should really have a return value, but that is an issue in Feast itself

michaelbackes

great work!

adchia and others added 26 commits July 3, 2023 12:29

ci: Add bigtable cleanup script

d372a09

Signed-off-by: Danny C <d.chiao@gmail.com>

fix: Missing Catalog argument in athena connector (feast-dev#3661)

f6d3caf

update Catalog argument in athena connector Signed-off-by: Gyumin Lee <t1100394@T1100394PM01.local> Co-authored-by: Gyumin Lee <t1100394@T1100394PM01.local>

ci: Disable flaky lambda materialization test

d4f9158

Signed-off-by: Danny C <d.chiao@gmail.com>

fix: Broken non-root path with projects-list.json (feast-dev#3665)

4861af0

ensure correct precedence with the two operators Signed-off-by: Ben Fletcher <ben.fletcher@ft.com>

fix: Manage redis pipe's context (feast-dev#3655)

48e0971

Signed-off-by: Jiwon Park <bakjeeone@hotmail.com>

fix: For SQL registry, increase max data_source_name length to 255 (f…

478caec

…east-dev#3630) * sql.py data_sources.data_source_name String(255) Extend the limit of the data_source_name field from 50 to 255. Signed-off-by: Ross Donnachie <code@radonn.co.za>

fix: Optimize bytes processed when retrieving entity df schema to 0 (f…

1c01035

…east-dev#3680) feat: Optimize bytes processed when retrieving entity df schema to 0 Signed-off-by: Hai Nguyen <quanghai.ng1512@gmail.com>

fix: Add aws-sts dependency in java sdk so that S3 client acquires IR…

c75a01f

…SA role (feast-dev#3696) Add aws-sts dependency in java sdk Signed-off-by: harmeet-singh-discovery <harmeet_singh@discovery.com>

Adding initial update changes

0578b9b

Merge branch 'feast-dev:master' into msudhir/add-vector-update-functi…

8487678

…onality

Added formatting changes

5828891

Revert "Merge branch 'feast-dev:master' into msudhir/add-vector-updat…

4a29d33

…e-functionality" This reverts commit 8487678, reversing changes made to 0578b9b.

Added more tests and functionality

e209770

updating tests

ebe1e32

updated functionality and added more tests

62692e0

correcting a test case

0680c94

michaelbackes reviewed Aug 9, 2023

View reviewed changes

Making formatting corrections and changeing log

5c5490d

michaelbackes reviewed Aug 9, 2023

View reviewed changes

Improved tests and added functionality to convert feast schema to mil…

cdadb87

…vus readable schema

michaelbackes reviewed Aug 10, 2023

View reviewed changes

Manisha Sudhir added 2 commits August 11, 2023 13:53

Added PR Review comments

e1fd230

Fixed failing test

d0c4269

michaelbackes approved these changes Aug 11, 2023

View reviewed changes

Manisha4 merged commit 2c7b7b1 into master Aug 11, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Msudhir/add vector update functionality #14

Msudhir/add vector update functionality #14

Manisha4 commented Aug 7, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 9, 2023

michaelbackes Aug 10, 2023

michaelbackes Aug 10, 2023

michaelbackes Aug 10, 2023

michaelbackes Aug 10, 2023

Manisha4 Aug 10, 2023

michaelbackes Aug 10, 2023

Manisha4 Aug 11, 2023

michaelbackes Aug 10, 2023

Manisha4 Aug 11, 2023

michaelbackes Aug 10, 2023

Manisha4 Aug 10, 2023

michaelbackes Aug 10, 2023

michaelbackes left a comment

Msudhir/add vector update functionality #14

Msudhir/add vector update functionality #14

Conversation

Manisha4 commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelbackes left a comment

Choose a reason for hiding this comment