Fix for updating vector-specific config params (qdrant#2328)

generall · timvisee · generall · commit d328f961e688 · 2023-07-31T11:42:06.000+02:00
* unit-test for updating multivec config

* Remove single vector support from vector params update in REST API

With this change, a user is always required to specify vector names when
updating their parameters. Updating vector parameters in a collection
with a single vector is still possible by providing an empty name. This
is very reasonable as updating vector parameters on a single vector
collection is a bad pattern, since parameters can be set on the
collection itself.

* Update integration tests to reflect REST API change

* Update OpenAPI specification

* Remove obsolete VectorsConfigDiff functions, update docs

* Update OpenAPI specification

* Add name validation to update collection endpoint for vector params

---------

Co-authored-by: timvisee &lt;tim@visee.me&gt;
diff --git a/docs/redoc/master/openapi.json b/docs/redoc/master/openapi.json
@@ -5916,18 +5916,11 @@
         }
       },
       "VectorsConfigDiff": {
-        "description": "Vector update params separator for single and multiple vector modes\n\nSingle mode:\n\n{ \"hnsw_config\": { \"m\": 8 } }\n\nor multiple mode:\n\n{ \"default\": { \"hnsw_config\": { \"m\": 8 } } }",
-        "anyOf": [
-          {
-            "$ref": "#/components/schemas/VectorParamsDiff"
-          },
-          {
-            "type": "object",
-            "additionalProperties": {
-              "$ref": "#/components/schemas/VectorParamsDiff"
-            }
-          }
-        ]
+        "description": "Vector update params for multiple vectors\n\n{ \"vector_name\": { \"hnsw_config\": { \"m\": 8 } } }",
+        "type": "object",
+        "additionalProperties": {
+          "$ref": "#/components/schemas/VectorParamsDiff"
+        }
       },
       "VectorParamsDiff": {
         "type": "object",
diff --git a/lib/collection/src/collection.rs b/lib/collection/src/collection.rs
@@ -1140,6 +1140,7 @@ impl Collection {
         update_vectors_diff: &VectorsConfigDiff,
     ) -> CollectionResult<()> {
         let mut config = self.collection_config.write().await;
+        update_vectors_diff.check_vector_names(&config.params)?;
         config
             .params
             .update_vectors_from_diff(update_vectors_diff)?;
diff --git a/lib/collection/src/config.rs b/lib/collection/src/config.rs
@@ -183,7 +183,7 @@ impl CollectionParams {
         &mut self,
         update_vectors_diff: &VectorsConfigDiff,
     ) -> CollectionResult<()> {
-        for (vector_name, update_params) in update_vectors_diff.params_iter() {
+        for (vector_name, update_params) in update_vectors_diff.0.iter() {
             let vector_params = self.get_vector_params_mut(vector_name)?;
 
             // Update and replace vector params from vector specific diff
diff --git a/lib/collection/src/operations/conversions.rs b/lib/collection/src/operations/conversions.rs
@@ -362,14 +362,15 @@ impl TryFrom<api::grpc::qdrant::vectors_config_diff::Config> for VectorsConfigDi
     ) -> Result<Self, Self::Error> {
         Ok(match value {
             api::grpc::qdrant::vectors_config_diff::Config::Params(vector_params) => {
-                VectorsConfigDiff::Single(vector_params.try_into()?)
+                let diff: VectorParamsDiff = vector_params.try_into()?;
+                VectorsConfigDiff::from(diff)
             }
             api::grpc::qdrant::vectors_config_diff::Config::ParamsMap(vectors_params) => {
                 let mut params_map = BTreeMap::new();
                 for (name, params) in vectors_params.map {
                     params_map.insert(name, params.try_into()?);
                 }
-                VectorsConfigDiff::Multi(params_map)
+                VectorsConfigDiff(params_map)
             }
         })
     }
diff --git a/lib/collection/src/operations/types.rs b/lib/collection/src/operations/types.rs
@@ -32,7 +32,7 @@ use tonic::codegen::http::uri::InvalidUri;
 use validator::{Validate, ValidationErrors};
 
 use super::config_diff;
-use crate::config::CollectionConfig;
+use crate::config::{CollectionConfig, CollectionParams};
 use crate::lookup::types::WithLookupInterface;
 use crate::operations::config_diff::HnswConfigDiff;
 use crate::save_on_disk;
@@ -995,66 +995,51 @@ pub struct VectorParamsDiff {
     pub hnsw_config: Option<HnswConfigDiff>,
 }
 
-/// Vector update params separator for single and multiple vector modes
-///
-/// Single mode:
-///
-/// { "hnsw_config": { "m": 8 } }
-///
-/// or multiple mode:
+/// Vector update params for multiple vectors
 ///
 /// {
-///     "default": {
+///     "vector_name": {
 ///         "hnsw_config": { "m": 8 }
 ///     }
 /// }
 #[derive(Debug, Deserialize, Serialize, JsonSchema, Clone, PartialEq, Hash, Eq)]
-#[serde(rename_all = "snake_case", untagged)]
-pub enum VectorsConfigDiff {
-    Single(VectorParamsDiff),
-    Multi(BTreeMap<String, VectorParamsDiff>),
-}
+pub struct VectorsConfigDiff(pub BTreeMap<String, VectorParamsDiff>);
 
 impl VectorsConfigDiff {
-    pub fn get_params(&self, name: &str) -> Option<&VectorParamsDiff> {
-        match self {
-            VectorsConfigDiff::Single(params) => (name == DEFAULT_VECTOR_NAME).then_some(params),
-            VectorsConfigDiff::Multi(params) => params.get(name),
-        }
-    }
-
-    /// Iterate over the named vector parameters.
+    /// Check that the vector names in this config are part of the given collection.
     ///
-    /// If this is `Single` it iterates over a single parameter named [`DEFAULT_VECTOR_NAME`].
-    pub fn params_iter<'a>(&'a self) -> Box<dyn Iterator<Item = (&str, &VectorParamsDiff)> + 'a> {
-        match self {
-            VectorsConfigDiff::Single(p) => Box::new(std::iter::once((DEFAULT_VECTOR_NAME, p))),
-            VectorsConfigDiff::Multi(p) => Box::new(p.iter().map(|(n, p)| (n.as_str(), p))),
+    /// Returns an error if incompatible.
+    pub fn check_vector_names(&self, collection: &CollectionParams) -> CollectionResult<()> {
+        for vector_name in self.0.keys() {
+            collection
+                .vectors
+                .get_params(vector_name)
+                .map(|_| ())
+                .ok_or_else(|| OperationError::VectorNameNotExists {
+                    received_name: vector_name.into(),
+                })?;
         }
+        Ok(())
     }
 }
 
 impl Validate for VectorsConfigDiff {
     fn validate(&self) -> Result<(), ValidationErrors> {
-        match self {
-            VectorsConfigDiff::Single(single) => single.validate(),
-            VectorsConfigDiff::Multi(multi) => {
-                let errors = multi
-                    .values()
-                    .filter_map(|v| v.validate().err())
-                    .fold(Err(ValidationErrors::new()), |bag, err| {
-                        ValidationErrors::merge(bag, "?", Err(err))
-                    })
-                    .unwrap_err();
-                errors.errors().is_empty().then_some(()).ok_or(errors)
-            }
-        }
+        let errors = self
+            .0
+            .values()
+            .filter_map(|v| v.validate().err())
+            .fold(Err(ValidationErrors::new()), |bag, err| {
+                ValidationErrors::merge(bag, "?", Err(err))
+            })
+            .unwrap_err();
+        errors.errors().is_empty().then_some(()).ok_or(errors)
     }
 }
 
 impl From<VectorParamsDiff> for VectorsConfigDiff {
     fn from(params: VectorParamsDiff) -> Self {
-        VectorsConfigDiff::Single(params)
+        VectorsConfigDiff(BTreeMap::from([("".into(), params)]))
     }
 }
 
diff --git a/openapi/tests/openapi_integration/test_collection_update.py b/openapi/tests/openapi_integration/test_collection_update.py
@@ -3,6 +3,7 @@
 from .helpers.helpers import request_with_validation
 from .helpers.collection_setup import basic_collection_setup, drop_collection
 
+default_name = ""
 collection_name = 'test_collection_uuid'
 
 
@@ -20,9 +21,11 @@ def test_collection_update():
         path_params={'collection_name': collection_name},
         body={
             "vectors": {
-                "hnsw_config": {
-                    "m": 32,
-                    "ef_construct": 123,
+                default_name: {
+                    "hnsw_config": {
+                        "m": 32,
+                        "ef_construct": 123,
+                    },
                 },
             },
             "optimizers_config": {
@@ -77,8 +80,10 @@ def test_hnsw_update():
         path_params={'collection_name': collection_name},
         body={
             "vectors": {
-                "hnsw_config": {
-                    "m": 32,
+                default_name: {
+                    "hnsw_config": {
+                        "m": 32,
+                    },
                 },
             },
             "hnsw_config": {
@@ -106,9 +111,11 @@ def test_hnsw_update():
         path_params={'collection_name': collection_name},
         body={
             "vectors": {
-                "hnsw_config": {
-                    "m": 10,
-                    "ef_construct": 100,
+                default_name: {
+                    "hnsw_config": {
+                        "m": 10,
+                        "ef_construct": 100,
+                    },
                 },
             },
         }
diff --git a/openapi/tests/openapi_integration/test_collection_update_multivec.py b/openapi/tests/openapi_integration/test_collection_update_multivec.py
@@ -0,0 +1,156 @@
+import pytest
+
+from .helpers.helpers import request_with_validation
+from .helpers.collection_setup import drop_collection, multivec_collection_setup
+
+collection_name = 'test_collection_update_multivec'
+
+
+@pytest.fixture(autouse=True)
+def setup():
+    multivec_collection_setup(collection_name=collection_name)
+    yield
+    drop_collection(collection_name=collection_name)
+
+
+def test_collection_update_multivec():
+    response = request_with_validation(
+        api='/collections/{collection_name}',
+        method="PATCH",
+        path_params={'collection_name': collection_name},
+        body={
+            "vectors": {
+                "image": {
+                    "hnsw_config": {
+                        "m": 32,
+                        "ef_construct": 123,
+                    }
+                }
+            },
+            "optimizers_config": {
+                "default_segment_number": 6,
+                "indexing_threshold": 10000,
+            },
+        }
+    )
+    assert response.ok
+
+    response = request_with_validation(
+        api='/collections/{collection_name}/points',
+        method="PUT",
+        path_params={'collection_name': collection_name},
+        query_params={'wait': 'true'},
+        body={
+            "points": [
+                {
+                    "id": 7,
+                    "vector": {
+                        "image": [0.15, 0.31, 0.76, 0.74]
+                    },
+                    "payload": {"city": "Rome"}
+                }
+            ]
+        }
+    )
+    assert response.ok
+
+    response = request_with_validation(
+        api='/collections/{collection_name}/points/{id}',
+        method="GET",
+        path_params={'collection_name': collection_name, 'id': 7},
+    )
+    assert response.ok
+
+
+def test_hnsw_update():
+    response = request_with_validation(
+        api='/collections/{collection_name}',
+        method="GET",
+        path_params={'collection_name': collection_name},
+    )
+    assert response.ok
+    result = response.json()["result"]
+    config = result["config"]
+    assert "hnsw_config" not in config["params"]["vectors"]
+    assert config["hnsw_config"]["m"] == 16
+    assert config["hnsw_config"]["ef_construct"] == 100
+
+    response = request_with_validation(
+        api='/collections/{collection_name}',
+        method="PATCH",
+        path_params={'collection_name': collection_name},
+        body={
+            "vectors": {
+                "text": {
+                    "hnsw_config": {
+                        "m": 32,
+                    }
+                }
+            },
+            "hnsw_config": {
+                "ef_construct": 123,
+            },
+        }
+    )
+    assert response.ok
+
+    response = request_with_validation(
+        api='/collections/{collection_name}',
+        method="GET",
+        path_params={'collection_name': collection_name},
+    )
+    assert response.ok
+    result = response.json()["result"]
+    config = result["config"]
+    assert config["params"]["vectors"]["text"]["hnsw_config"]["m"] == 32
+    assert config["hnsw_config"]["m"] == 16
+    assert config["hnsw_config"]["ef_construct"] == 123
+
+    response = request_with_validation(
+        api='/collections/{collection_name}',
+        method="PATCH",
+        path_params={'collection_name': collection_name},
+        body={
+            "vectors": {
+                "text": {
+                    "hnsw_config": {
+                        "m": 10,
+                        "ef_construct": 100,
+                    }
+                }
+            },
+        }
+    )
+    assert response.ok
+
+    response = request_with_validation(
+        api='/collections/{collection_name}',
+        method="GET",
+        path_params={'collection_name': collection_name},
+    )
+    assert response.ok
+    result = response.json()["result"]
+    config = result["config"]
+    assert config["params"]["vectors"]["text"]["hnsw_config"]["m"] == 10
+    assert config["params"]["vectors"]["text"]["hnsw_config"]["ef_construct"] == 100
+
+
+def test_invalid_vector_name():
+    response = request_with_validation(
+        api='/collections/{collection_name}',
+        method="PATCH",
+        path_params={'collection_name': collection_name},
+        body={
+            "vectors": {
+                "i_do_no_exist": {
+                    "hnsw_config": {
+                        "m": 32,
+                    }
+                }
+            },
+        }
+    )
+    assert not response.ok
+    assert response.status_code == 400
+    error = response.json()["status"]["error"]
+    assert error == "Wrong input: Not existing vector name error: i_do_no_exist"

Original file line number	Diff line number	Diff line change
`@@ -362,14 +362,15 @@ impl TryFrom<api::grpc::qdrant::vectors_config_diff::Config> for VectorsConfigDi`
`362`	`362`	`) -> Result<Self, Self::Error> {`
`363`	`363`	`Ok(match value {`
`364`	`364`	`api::grpc::qdrant::vectors_config_diff::Config::Params(vector_params) => {`
`365`		`- VectorsConfigDiff::Single(vector_params.try_into()?)`
	`365`	`+ let diff: VectorParamsDiff = vector_params.try_into()?;`
	`366`	`+ VectorsConfigDiff::from(diff)`
`366`	`367`	`}`
`367`	`368`	`api::grpc::qdrant::vectors_config_diff::Config::ParamsMap(vectors_params) => {`
`368`	`369`	`let mut params_map = BTreeMap::new();`
`369`	`370`	`for (name, params) in vectors_params.map {`
`370`	`371`	`params_map.insert(name, params.try_into()?);`
`371`	`372`	`}`
`372`		`- VectorsConfigDiff::Multi(params_map)`
	`373`	`+ VectorsConfigDiff(params_map)`
`373`	`374`	`}`
`374`	`375`	`})`
`375`	`376`	`}`