Code Review Comments for Parallelism Implementation

[AlexCuadron]

Code Review Comments for PR #55 (lgs/parallelism)

This PR contains inline code review comments highlighting issues found in the parallelism implementation. The comments are embedded directly in the code for easy visibility in the GitHub UI.

Issues Identified:

🔒 Thread Safety Concerns

• RLock vs Lock: Using threading.RLock() everywhere may be overkill and impacts performance. Consider using regular Lock() where recursive locking isn't needed.
• Excessive Locking: Locking simple read operations like is_empty() may unnecessarily impact performance.

🐛 Potential Bugs

• FAISS Implementation: Index mismatch between similarity scores and IDs when filtering invalid results in get_knn().
• Missing Return Statement: Cache.add() method doesn't return the embedding ID as documented.
• Reset Logic: FAISS reset loses dimension information by setting index = None.

📚 Documentation Issues

• Google Style Guide: Docstrings don't follow Google style guide format consistently.
• Type Annotations: Inconsistent format for parameter types in docstrings.

🧹 Code Quality Issues

• Commented Code: Large blocks of commented-out ThreadPoolExecutor code should be removed or properly implemented.
• Unused Imports: ThreadPoolExecutor is imported but not used.
• Async Processing: The background processing optimization is commented out, defeating its purpose.

🧪 Test Issues

• ID Generation Assumptions: Tests assume sequential ID generation (0 to n-1) which may not hold for all implementations.

Recommendations:

1. Fix the FAISS filtering bug by properly handling the index alignment between similarities and IDs.
2. Decide on async processing: Either implement the ThreadPoolExecutor properly or remove the commented code.
3. Review locking strategy: Use regular locks where possible and avoid locking read-only operations.
4. Fix missing return statements and ensure all methods return what they document.
5. Update documentation to follow Google style guide consistently.
6. Improve tests to be more robust and not assume specific ID generation patterns.

Note:

These comments are meant to help improve the code quality and should be addressed before merging the parallelism changes.