diff --git a/tests/unit/VectorDB/test_thread_safety.py b/tests/unit/VectorDB/test_thread_safety.py index a4e436c..e34e2b3 100644 --- a/tests/unit/VectorDB/test_thread_safety.py +++ b/tests/unit/VectorDB/test_thread_safety.py @@ -93,6 +93,9 @@ def _test_vector_db_thread_safety(self, vector_db): f"Expected {expected_total} unique IDs, got {len(all_ids)}" ) + # REVIEW COMMENT: This test assumes sequential ID generation (0 to n-1) + # This assumption may not hold for all vector DB implementations + # Consider testing uniqueness instead of specific ID values # Verify IDs are sequential (0 to expected_total - 1) expected_ids = set(range(expected_total)) self.assertEqual( diff --git a/vcache/vcache_core/cache/cache.py b/vcache/vcache_core/cache/cache.py index fa7f931..7264ef3 100644 --- a/vcache/vcache_core/cache/cache.py +++ b/vcache/vcache_core/cache/cache.py @@ -25,13 +25,19 @@ def add(self, prompt: str, response: str) -> int: Initializes the metadata object using the response and stores it in the embedding metadata storage. Args: - prompt: str - The prompt to add to the cache - response: str - The response to add to the cache + prompt (str): The prompt to add to the cache + response (str): The response to add to the cache Returns: - int - The id of the embedding + int: The id of the embedding + + # REVIEW COMMENT: Docstring format should follow Google style guide: + # - Use (type) instead of : type - format + # - Use Returns: type instead of returns: type """ embedding = self.embedding_engine.get_embedding(prompt) + # REVIEW COMMENT: Missing return statement - method should return the embedding ID + # as documented in the docstring self.embedding_store.add_embedding(embedding, response) def remove(self, embedding_id: int) -> int: diff --git a/vcache/vcache_core/cache/embedding_store/vector_db/strategies/chroma.py b/vcache/vcache_core/cache/embedding_store/vector_db/strategies/chroma.py index ca80532..7f91c1b 100644 --- a/vcache/vcache_core/cache/embedding_store/vector_db/strategies/chroma.py +++ b/vcache/vcache_core/cache/embedding_store/vector_db/strategies/chroma.py @@ -17,6 +17,8 @@ def __init__( self.collection = None self.client = None self.similarity_metric_type = similarity_metric_type + # REVIEW COMMENT: Consider using threading.Lock() instead of RLock() for better performance + # RLock allows recursive locking which may mask potential issues and is slower self._operation_lock = threading.RLock() def add(self, embedding: List[float]) -> int: diff --git a/vcache/vcache_core/cache/embedding_store/vector_db/strategies/faiss.py b/vcache/vcache_core/cache/embedding_store/vector_db/strategies/faiss.py index 53104fe..d073ea9 100644 --- a/vcache/vcache_core/cache/embedding_store/vector_db/strategies/faiss.py +++ b/vcache/vcache_core/cache/embedding_store/vector_db/strategies/faiss.py @@ -17,6 +17,8 @@ def __init__( self.similarity_metric_type = similarity_metric_type self.__next_embedding_id = 0 self.index = None + # REVIEW COMMENT: Consider using threading.Lock() instead of RLock() for better performance + # RLock allows recursive locking which may mask potential issues and is slower self._operation_lock = threading.RLock() def transform_similarity_score( @@ -100,6 +102,8 @@ def get_knn(self, embedding: List[float], k: int) -> List[tuple[float, int]]: if metric_type == "cosine": faiss.normalize_L2(embedding_array) similarities, ids = self.index.search(embedding_array, k_) + # REVIEW COMMENT: Potential bug - filtering IDs after transforming similarities + # can cause index mismatch. Filter both arrays together or use enumerate. similarity_scores = [ self.transform_similarity_score(sim, metric_type) for sim in similarities[0] ] @@ -111,6 +115,8 @@ def reset(self) -> None: Thread-safe reset of the vector database. """ with self._operation_lock: + # REVIEW COMMENT: Setting index to None loses dimension info. + # Consider storing dim separately or reinitializing with previous dim self.index = None self.__next_embedding_id = 0 diff --git a/vcache/vcache_core/cache/embedding_store/vector_db/strategies/hnsw_lib.py b/vcache/vcache_core/cache/embedding_store/vector_db/strategies/hnsw_lib.py index 23d82a8..6c9f263 100644 --- a/vcache/vcache_core/cache/embedding_store/vector_db/strategies/hnsw_lib.py +++ b/vcache/vcache_core/cache/embedding_store/vector_db/strategies/hnsw_lib.py @@ -29,6 +29,8 @@ def __init__( self.M = None self.ef = None self.index = None + # REVIEW COMMENT: Consider using threading.Lock() instead of RLock() for better performance + # RLock allows recursive locking which may mask potential issues and is slower self._operation_lock = threading.RLock() def add(self, embedding: List[float]) -> int: @@ -140,5 +142,7 @@ def is_empty(self) -> bool: Returns: bool - True if the database is empty, False otherwise """ + # REVIEW COMMENT: Consider if locking is necessary for simple read operations + # This might impact performance unnecessarily with self._operation_lock: return self.embedding_count == 0 diff --git a/vcache/vcache_policy/strategies/dynamic_local_threshold.py b/vcache/vcache_policy/strategies/dynamic_local_threshold.py index 17b3b24..105c716 100644 --- a/vcache/vcache_policy/strategies/dynamic_local_threshold.py +++ b/vcache/vcache_policy/strategies/dynamic_local_threshold.py @@ -1,5 +1,7 @@ import logging import random +# REVIEW COMMENT: ThreadPoolExecutor is imported but not used (commented out below) +# Consider removing unused imports from concurrent.futures import ThreadPoolExecutor from enum import Enum from typing import Dict, List, Optional, Tuple @@ -39,6 +41,8 @@ def __init__(self, delta: float = 0.01, max_background_workers: int = 4): self.similarity_evaluator: SimilarityEvaluator = None self.inference_engine: InferenceEngine = None self.cache: Cache = None + # REVIEW COMMENT: Commented out code should be removed or properly implemented + # If async processing is intended, implement it properly or remove the code # self._executor = ThreadPoolExecutor( # max_workers=max_background_workers, thread_name_prefix="vcache-bg" # ) @@ -92,6 +96,8 @@ def process_request( response = self.inference_engine.create( prompt=prompt, system_prompt=system_prompt ) + # REVIEW COMMENT: Commented out async processing - this defeats the purpose + # of the background processing optimization. Either implement properly or remove. # self._executor.submit( # self._generate_label, # response=response, @@ -148,6 +154,7 @@ def _generate_label( f"Error in background label generation: {e}", exc_info=True ) + # REVIEW COMMENT: Commented out cleanup code should be removed if not used # def __del__(self): # """Cleanup the ThreadPoolExecutor when the policy is destroyed.""" # if hasattr(self, "_executor") and self._executor: