added CRUD for Chorma Vector store#7
added CRUD for Chorma Vector store#7RonitKiranMurmu wants to merge 5 commits intopixelThreaderOfficial:mainfrom
Conversation
pixelThreader
left a comment
pixelThreader
left a comment
There was a problem hiding this comment.
@RonitKiranMurmu Please Update these changes in the file as instructed and use the DBLogger log the data and please ensure you're not repeating yourself!
use from main.src.utils.versionManagement import getAppVersion for getting the version Dynamically. for instance you can read the DRSecrets.py for looking how I used the logger there!!
backend/main/src/store/DBVector.py
Outdated
| _std_logger = logging.getLogger(__name__) | ||
|
|
||
| try: | ||
| import chromadb |
There was a problem hiding this comment.
keep all import statements at top
There was a problem hiding this comment.
The try-except block around [import chromadb] is a pattern for optional dependencies.
Here is why it is used instead of a standard top-level import:
Prevents App Crashes: [chromadb] is a heavy library. If a user (or a CI/CD server) doesn't have it installed, a standard [import chromadb] would crash the entire application immediately upon startup. The try-except block allows the app to start even if is missing.
Graceful Handling: It sets the flag [_CHROMA_AVAILABLE] which allows the rest of the code to handle the missing library gracefully (e.g., by setting [db_vector_manager = None] or raising a clear error message like ["Run: uv add chromadb"] when someone actually tries to use it).
If you move it to a bare [import chromadb] at the top, the program will crash instantly with ModuleNotFoundError if the package isn't installed.
So do i make it essential and move it to the top anyway or leave it right there
There was a problem hiding this comment.
actually remove the try-catch block from the chromadb that's not important because the backend is setup by experienced developers.
backend/main/src/store/DBVector.py
Outdated
| origin="system", | ||
| module="DB", | ||
| urgency=urgency, # type: ignore | ||
| app_version="1.0", |
There was a problem hiding this comment.
this is wrong!!
NO HARDCODED VERSION ACCEPTED!!
use from main.src.utils.versionManagement import getAppVersion
then the use the getAppVersion like app_version=getAppVersion()
There was a problem hiding this comment.
Updated it to remove hardcoding
There was a problem hiding this comment.
no, there is still the version hardcoded
| try: | ||
| chroma_store_dir.mkdir(parents=True, exist_ok=True) | ||
| _std_logger.info(f"ChromaDB storage directory ensured: {chroma_store_dir}") | ||
| dr_logger.log( |
There was a problem hiding this comment.
use the _log function and don't hardcode the version
| dr_logger.log( | ||
| log_type="error", | ||
| message=f"Failed to initialize ChromaDB store directory: {e}", | ||
| origin="system", | ||
| module="DB", | ||
| urgency="critical", | ||
| app_version="1.0", | ||
| ) |
There was a problem hiding this comment.
use the _log function and don't hardcode the version
There was a problem hiding this comment.
Brother use the logging module and you dont have to write the tests in the logs database!
There was a problem hiding this comment.
When running automated tests, we should not pollute the real logs database (logs.db.sqlite3) with test data. Tests should be isolated and "silent" regarding side effects like database writes.
How I addressed it:
I ensured this in two ways:
Mocking the Logger in Tests:
In [test_db_vector.py], the [setUp] method now includes:
self.patcher = patch("main.src.store.DBVector.dr_logger")
self.mock_logger = self.patcher.start()
This intercepts all calls to [dr_logger.log] during tests and sends them to a fake (mock) object instead of the real database. This effectively stops tests from writing to the logs DB.
Preventing Import Side-Effects:
In [DBVector.py], I added this check:
if not any("unittest" in arg for arg in sys.argv) and not any("pytest" in arg for arg in sys.argv):
_initialize_chroma_store()
This ensures that simply importing the file during a test run doesn't trigger the initialization log entry.
So, your tests are now clean and they verify the logic without junking up your actual database logs.
There was a problem hiding this comment.
Ronit, I think this is complicated for a quick testing. we can use the logger to log directly in terminal to see the usage.. you don't have to mock into the DB, because the tests don't have to do anything with the logger it will not run at the prod....
So from future you can skip this Mock DB Loggers in test, because those are just complications. just use terminal loggers only!!
pixelThreader
left a comment
pixelThreader
left a comment
There was a problem hiding this comment.
Reviewed and there are some minor needs just make them then, we're good to merge to main branch.
backend/main/src/store/DBVector.py
Outdated
| _std_logger = logging.getLogger(__name__) | ||
|
|
||
| try: | ||
| import chromadb |
There was a problem hiding this comment.
actually remove the try-catch block from the chromadb that's not important because the backend is setup by experienced developers.
There was a problem hiding this comment.
Ronit, I think this is complicated for a quick testing. we can use the logger to log directly in terminal to see the usage.. you don't have to mock into the DB, because the tests don't have to do anything with the logger it will not run at the prod....
So from future you can skip this Mock DB Loggers in test, because those are just complications. just use terminal loggers only!!
backend/main/src/store/DBVector.py
Outdated
| origin="system", | ||
| module="DB", | ||
| urgency=urgency, # type: ignore | ||
| app_version="1.0", |
There was a problem hiding this comment.
no, there is still the version hardcoded
2 participants
No description provided.