From a0de9509d735c3edc84133fcb367808b4a4c91be Mon Sep 17 00:00:00 2001 From: Jojii <15957865+JojiiOfficial@users.noreply.github.com> Date: Mon, 14 Oct 2024 10:27:00 +0200 Subject: [PATCH] Add strict mode config to update collection API + Populate strict mode in API (#5187) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * add strict mode to update collection API * update API specs * clippy * Update lib/storage/src/content_manager/toc/collection_meta_ops.rs * Strict mode integration tests (#5189) * Add integration tests for strict mode * add full update test * Apply suggestions from code review Co-authored-by: Tim Visée * Adjust error messages * improve checking of error response --------- Co-authored-by: Tim Visée * prefer explicit structu conversion --------- Co-authored-by: Tim Visée Co-authored-by: generall --- docs/grpc/docs.md | 1 + docs/redoc/master/openapi.json | 73 ++++ lib/api/src/grpc/proto/collections.proto | 1 + lib/api/src/grpc/qdrant.rs | 3 + .../src/collection/collection_ops.rs | 17 + .../content_manager/collection_meta_ops.rs | 4 +- .../src/content_manager/conversions.rs | 21 +- lib/storage/src/content_manager/mod.rs | 1 + .../toc/collection_meta_ops.rs | 4 + tests/openapi/test_strictmode.py | 373 ++++++++++++++++++ 10 files changed, 490 insertions(+), 8 deletions(-) create mode 100644 tests/openapi/test_strictmode.py diff --git a/docs/grpc/docs.md b/docs/grpc/docs.md index e9c4c22c879..d5909cee328 100644 --- a/docs/grpc/docs.md +++ b/docs/grpc/docs.md @@ -1469,6 +1469,7 @@ Note: 1kB = 1 vector of size 256. | | vectors_config | [VectorsConfigDiff](#qdrant-VectorsConfigDiff) | optional | New vector parameters | | quantization_config | [QuantizationConfigDiff](#qdrant-QuantizationConfigDiff) | optional | Quantization configuration of vector | | sparse_vectors_config | [SparseVectorConfig](#qdrant-SparseVectorConfig) | optional | New sparse vector parameters | +| strict_mode_config | [StrictModeConfig](#qdrant-StrictModeConfig) | optional | New strict mode configuration | diff --git a/docs/redoc/master/openapi.json b/docs/redoc/master/openapi.json index facde8e36f1..ab36f5c0034 100644 --- a/docs/redoc/master/openapi.json +++ b/docs/redoc/master/openapi.json @@ -8683,6 +8683,17 @@ "$ref": "#/components/schemas/SparseVectorParams" }, "nullable": true + }, + "strict_mode_config": { + "description": "Strict-mode config.", + "anyOf": [ + { + "$ref": "#/components/schemas/StrictModeConfig" + }, + { + "nullable": true + } + ] } } }, @@ -8777,6 +8788,58 @@ } } }, + "StrictModeConfig": { + "type": "object", + "properties": { + "enabled": { + "description": "Whether strict mode is enabled for a collection or not.", + "type": "boolean", + "nullable": true + }, + "max_query_limit": { + "description": "Max allowed `limit` parameter for all APIs that don't have their own max limit.", + "type": "integer", + "format": "uint", + "minimum": 1, + "nullable": true + }, + "max_timeout": { + "description": "Max allowed `timeout` parameter.", + "type": "integer", + "format": "uint", + "minimum": 1, + "nullable": true + }, + "unindexed_filtering_retrieve": { + "description": "Allow usage of unindexed fields in retrieval based (eg. search) filters.", + "type": "boolean", + "nullable": true + }, + "unindexed_filtering_update": { + "description": "Allow usage of unindexed fields in filtered updates (eg. delete by payload).", + "type": "boolean", + "nullable": true + }, + "search_max_hnsw_ef": { + "description": "Max HNSW value allowed in search parameters.", + "type": "integer", + "format": "uint", + "minimum": 0, + "nullable": true + }, + "search_allow_exact": { + "description": "Whether exact search is allowed or not.", + "type": "boolean", + "nullable": true + }, + "search_max_oversampling": { + "description": "Max oversampling value allowed in search.", + "type": "number", + "format": "double", + "nullable": true + } + } + }, "UpdateCollection": { "description": "Operation for updating parameters of the existing collection", "type": "object", @@ -8847,6 +8910,16 @@ "nullable": true } ] + }, + "strict_mode_config": { + "anyOf": [ + { + "$ref": "#/components/schemas/StrictModeConfig" + }, + { + "nullable": true + } + ] } } }, diff --git a/lib/api/src/grpc/proto/collections.proto b/lib/api/src/grpc/proto/collections.proto index 474735f62c6..589a96162b7 100644 --- a/lib/api/src/grpc/proto/collections.proto +++ b/lib/api/src/grpc/proto/collections.proto @@ -351,6 +351,7 @@ message UpdateCollection { optional VectorsConfigDiff vectors_config = 6; // New vector parameters optional QuantizationConfigDiff quantization_config = 7; // Quantization configuration of vector optional SparseVectorConfig sparse_vectors_config = 8; // New sparse vector parameters + optional StrictModeConfig strict_mode_config = 9; // New strict mode configuration } message DeleteCollection { diff --git a/lib/api/src/grpc/qdrant.rs b/lib/api/src/grpc/qdrant.rs index 9d8b0a9c923..2d30f531ba6 100644 --- a/lib/api/src/grpc/qdrant.rs +++ b/lib/api/src/grpc/qdrant.rs @@ -549,6 +549,9 @@ pub struct UpdateCollection { /// New sparse vector parameters #[prost(message, optional, tag = "8")] pub sparse_vectors_config: ::core::option::Option, + /// New strict mode configuration + #[prost(message, optional, tag = "9")] + pub strict_mode_config: ::core::option::Option, } #[derive(validator::Validate)] #[derive(serde::Serialize)] diff --git a/lib/collection/src/collection/collection_ops.rs b/lib/collection/src/collection/collection_ops.rs index f0d4dc998bf..853777020ed 100644 --- a/lib/collection/src/collection/collection_ops.rs +++ b/lib/collection/src/collection/collection_ops.rs @@ -162,6 +162,23 @@ impl Collection { Ok(()) } + /// Updates the strict mode configuration and saves it to disk. + pub async fn update_strict_mode_config( + &self, + strict_mode_diff: StrictModeConfig, + ) -> CollectionResult<()> { + { + let mut config = self.collection_config.write().await; + if let Some(current_config) = config.strict_mode_config.as_mut() { + *current_config = strict_mode_diff.update(current_config)?; + } else { + config.strict_mode_config = Some(strict_mode_diff); + } + } + self.collection_config.read().await.save(&self.path)?; + Ok(()) + } + /// Handle replica changes /// /// add and remove replicas from replica set diff --git a/lib/storage/src/content_manager/collection_meta_ops.rs b/lib/storage/src/content_manager/collection_meta_ops.rs index 58a73b86bd4..fa4e8db062a 100644 --- a/lib/storage/src/content_manager/collection_meta_ops.rs +++ b/lib/storage/src/content_manager/collection_meta_ops.rs @@ -170,7 +170,6 @@ pub struct CreateCollection { pub sparse_vectors: Option>, /// Strict-mode config. #[validate(nested)] - #[schemars(skip)] pub strict_mode_config: Option, } @@ -229,6 +228,8 @@ pub struct UpdateCollection { /// Map of sparse vector data parameters to update for each sparse vector. #[validate(nested)] pub sparse_vectors: Option, + #[validate(nested)] + pub strict_mode_config: Option, } /// Operation for updating parameters of the existing collection @@ -251,6 +252,7 @@ impl UpdateCollectionOperation { optimizers_config: None, quantization_config: None, sparse_vectors: None, + strict_mode_config: None, }, shard_replica_changes: None, } diff --git a/lib/storage/src/content_manager/conversions.rs b/lib/storage/src/content_manager/conversions.rs index 79f9917f265..b40040dda1b 100644 --- a/lib/storage/src/content_manager/conversions.rs +++ b/lib/storage/src/content_manager/conversions.rs @@ -1,5 +1,8 @@ +use collection::operations::config_diff::{ + CollectionParamsDiff, HnswConfigDiff, OptimizersConfigDiff, QuantizationConfigDiff, +}; use collection::operations::conversions::sharding_method_from_proto; -use collection::operations::types::SparseVectorsConfig; +use collection::operations::types::{SparseVectorsConfig, VectorsConfigDiff}; use segment::types::StrictModeConfig; use tonic::Status; @@ -92,19 +95,23 @@ impl TryFrom for CollectionMetaOperations { vectors: value .vectors_config .and_then(|config| config.config) - .map(TryInto::try_into) + .map(VectorsConfigDiff::try_from) + .transpose()?, + hnsw_config: value.hnsw_config.map(HnswConfigDiff::from), + params: value + .params + .map(CollectionParamsDiff::try_from) .transpose()?, - hnsw_config: value.hnsw_config.map(Into::into), - params: value.params.map(TryInto::try_into).transpose()?, - optimizers_config: value.optimizers_config.map(Into::into), + optimizers_config: value.optimizers_config.map(OptimizersConfigDiff::from), quantization_config: value .quantization_config - .map(TryInto::try_into) + .map(QuantizationConfigDiff::try_from) .transpose()?, sparse_vectors: value .sparse_vectors_config - .map(TryInto::try_into) + .map(SparseVectorsConfig::try_from) .transpose()?, + strict_mode_config: value.strict_mode_config.map(StrictModeConfig::from), }, ))) } diff --git a/lib/storage/src/content_manager/mod.rs b/lib/storage/src/content_manager/mod.rs index 0ed999c68c3..b20af768338 100644 --- a/lib/storage/src/content_manager/mod.rs +++ b/lib/storage/src/content_manager/mod.rs @@ -135,6 +135,7 @@ pub mod consensus_ops { hnsw_config: None, quantization_config: None, sparse_vectors: None, + strict_mode_config: None, }, ); operation diff --git a/lib/storage/src/content_manager/toc/collection_meta_ops.rs b/lib/storage/src/content_manager/toc/collection_meta_ops.rs index 4edea5a7797..4304eee5526 100644 --- a/lib/storage/src/content_manager/toc/collection_meta_ops.rs +++ b/lib/storage/src/content_manager/toc/collection_meta_ops.rs @@ -126,6 +126,7 @@ impl TableOfContent { optimizers_config, quantization_config, sparse_vectors, + strict_mode_config: strict_mode, } = operation.update_collection; let collection = self .get_collection_unchecked(&operation.collection_name) @@ -161,6 +162,9 @@ impl TableOfContent { if let Some(changes) = replica_changes { collection.handle_replica_changes(changes).await?; } + if let Some(strict_mode) = strict_mode { + collection.update_strict_mode_config(strict_mode).await?; + } // Recreate optimizers if recreate_optimizers { diff --git a/tests/openapi/test_strictmode.py b/tests/openapi/test_strictmode.py new file mode 100644 index 00000000000..ad35440366a --- /dev/null +++ b/tests/openapi/test_strictmode.py @@ -0,0 +1,373 @@ +import pytest + +from .helpers.collection_setup import basic_collection_setup, drop_collection +from .helpers.helpers import request_with_validation + +collection_name = "test_strict_mode" + + +@pytest.fixture(autouse=True) +def setup(): + basic_collection_setup(collection_name=collection_name) + yield + drop_collection(collection_name=collection_name) + + +def set_strict_mode(strict_mode_config): + request_with_validation( + api="/collections/{collection_name}", + method="PATCH", + path_params={"collection_name": collection_name}, + body={ + "strict_mode_config": strict_mode_config, + }, + ).raise_for_status() + + +def get_strict_mode(): + response = request_with_validation( + api='/collections/{collection_name}', + method="GET", + path_params={'collection_name': collection_name}, + ) + assert response.ok + + config = response.json()['result']['config'] + if "strict_mode_config" not in config: + return None + else: + return config['strict_mode_config'] + + +def strict_mode_enabled() -> bool: + strict_mode = get_strict_mode() + return strict_mode is not None and strict_mode['enabled'] + + +def test_patch_collection_full(): + assert not strict_mode_enabled() + + set_strict_mode({ + "enabled": True, + "max_query_limit": 10, + "max_timeout": 2, + "unindexed_filtering_retrieve": False, + "unindexed_filtering_update": False, + "search_max_hnsw_ef": 3, + "search_allow_exact": False, + "search_max_oversampling": 1.5, + }) + + response = request_with_validation( + api='/collections/{collection_name}', + method="GET", + path_params={'collection_name': collection_name}, + ) + + assert response.ok + new_strict_mode_config = response.json()['result']['config']['strict_mode_config'] + assert new_strict_mode_config['enabled'] + assert new_strict_mode_config['max_query_limit'] == 10 + assert new_strict_mode_config['max_timeout'] == 2 + assert not new_strict_mode_config['unindexed_filtering_retrieve'] + assert not new_strict_mode_config['unindexed_filtering_update'] + assert new_strict_mode_config['search_max_hnsw_ef'] == 3 + assert not new_strict_mode_config['search_allow_exact'] + assert new_strict_mode_config['search_max_oversampling'] == 1.5 + + +def test_patch_collection_partially(): + assert not strict_mode_enabled() + + set_strict_mode({ + "enabled": True, + "max_query_limit": 10, + "search_max_oversampling": 1.5, + }) + + response = request_with_validation( + api='/collections/{collection_name}', + method="GET", + path_params={'collection_name': collection_name}, + ) + + assert response.ok + new_strict_mode_config = response.json()['result']['config']['strict_mode_config'] + assert new_strict_mode_config['enabled'] + assert new_strict_mode_config['max_query_limit'] == 10 + assert new_strict_mode_config['search_max_oversampling'] == 1.5 + + +def test_strict_mode_query_limit_validation(): + def search_request(): + return request_with_validation( + api='/collections/{collection_name}/points/search', + method="POST", + path_params={'collection_name': collection_name}, + body={ + "vector": [0.2, 0.1, 0.9, 0.7], + "limit": 4 + } + ) + + search_request().raise_for_status() + + set_strict_mode({ + "enabled": True, + "max_query_limit": 4, + }) + + search_request().raise_for_status() + + set_strict_mode({ + "max_query_limit": 3, + }) + + search_fail = search_request() + + assert "limit" in search_fail.json()['status']['error'] + assert not search_fail.ok + + +def test_strict_mode_timeout_validation(): + def search_request_with_timeout(timeout): + return request_with_validation( + api='/collections/{collection_name}/points/search', + method="POST", + path_params={'collection_name': collection_name}, + query_params={'timeout': timeout}, + body={ + "vector": [0.2, 0.1, 0.9, 0.7], + "limit": 3 + } + ) + + search_request_with_timeout(3).raise_for_status() + + set_strict_mode({ + "enabled": True, + "max_timeout": 2, + }) + + search_request_with_timeout(2).raise_for_status() + + search_fail = search_request_with_timeout(3) + + assert "timeout" in search_fail.json()['status']['error'] + assert not search_fail.ok + + +def test_strict_mode_unindexed_filter_read_validation(): + def search_request_with_filter(): + return request_with_validation( + api='/collections/{collection_name}/points/search', + method="POST", + path_params={'collection_name': collection_name}, + body={ + "vector": [0.2, 0.1, 0.9, 0.7], + "limit": 3, + "filter": { + "must": [ + { + "key": "city", + "match": { + "value": "Berlin" + } + } + ] + }, + } + ) + + search_request_with_filter().raise_for_status() + + set_strict_mode({ + "enabled": True, + "unindexed_filtering_retrieve": True, + }) + + search_request_with_filter().raise_for_status() + + set_strict_mode({ + "unindexed_filtering_retrieve": False, + }) + + search_fail = search_request_with_filter() + + assert "city" in search_fail.json()['status']['error'] + assert not search_fail.ok + + request_with_validation( + api='/collections/{collection_name}/index', + method="PUT", + path_params={'collection_name': collection_name}, + query_params={'wait': 'true'}, + body={ + "field_name": "city", + "field_schema": "keyword" + } + ).raise_for_status() + + # We created an index on this field so it should work now + search_request_with_filter().raise_for_status() + + +def test_strict_mode_unindexed_filter_write_validation(): + def update_request_with_filter(): + return request_with_validation( + api='/collections/{collection_name}/points/delete', + method="POST", + path_params={'collection_name': collection_name}, + query_params={'wait': 'true'}, + body={ + "filter": { + "must": [ + { + "key": "city", + "match": { + "value": "Berlin" + } + } + ] + } + }) + + update_request_with_filter().raise_for_status() + + # Reset any changes + basic_collection_setup(collection_name=collection_name) + + set_strict_mode({ + "enabled": True, + "unindexed_filtering_update": True, + }) + + update_request_with_filter().raise_for_status() + + set_strict_mode({ + "unindexed_filtering_update": False, + }) + + search_fail = update_request_with_filter() + + assert "city" in search_fail.json()['status']['error'] + assert not search_fail.ok + + request_with_validation( + api='/collections/{collection_name}/index', + method="PUT", + path_params={'collection_name': collection_name}, + query_params={'wait': 'true'}, + body={ + "field_name": "city", + "field_schema": "keyword" + } + ).raise_for_status() + + # We created an index on this field so it should work now + update_request_with_filter().raise_for_status() + + +def test_strict_mode_max_ef_hnsw_validation(): + def search_request(): + return request_with_validation( + api='/collections/{collection_name}/points/search', + method="POST", + path_params={'collection_name': collection_name}, + body={ + "vector": [0.2, 0.1, 0.9, 0.7], + "limit": 4, + "params": { + "hnsw_ef": 5, + } + } + ) + + search_request().raise_for_status() + + set_strict_mode({ + "enabled": True, + "search_max_hnsw_ef": 5, + }) + + search_request().raise_for_status() + + set_strict_mode({ + "search_max_hnsw_ef": 4, + }) + + search_fail = search_request() + + assert "hnsw_ef" in search_fail.json()['status']['error'] + assert not search_fail.ok + + +def test_strict_mode_allow_exact_validation(): + def search_request(): + return request_with_validation( + api='/collections/{collection_name}/points/search', + method="POST", + path_params={'collection_name': collection_name}, + body={ + "vector": [0.2, 0.1, 0.9, 0.7], + "limit": 4, + "params": { + "exact": True, + } + } + ) + + search_request().raise_for_status() + + set_strict_mode({ + "enabled": True, + "search_allow_exact": True, + }) + + search_request().raise_for_status() + + set_strict_mode({ + "search_allow_exact": False, + }) + + search_fail = search_request() + + assert "exact" in search_fail.json()['status']['error'].lower() + assert not search_fail.ok + + +def test_strict_mode_search_max_oversampling_validation(): + def search_request(): + return request_with_validation( + api='/collections/{collection_name}/points/search', + method="POST", + path_params={'collection_name': collection_name}, + body={ + "vector": [0.2, 0.1, 0.9, 0.7], + "limit": 4, + "params": { + "quantization": { + "oversampling": 2.0, + } + } + } + ) + + search_request().raise_for_status() + + set_strict_mode({ + "enabled": True, + "search_max_oversampling": 2.0, + }) + + search_request().raise_for_status() + + set_strict_mode({ + "enabled": True, + "search_max_oversampling": 1.9, + }) + + search_fail = search_request() + + assert "oversampling" in search_fail.json()['status']['error'] + assert not search_fail.ok