feat: auto-fallback to SQLite when embedded SeekDB is unavailable - #993
Conversation
b5d2f2a to
8328d4c
Compare
|
Thanks for the update. I rechecked this after the latest commits. The checks are green, but GitHub currently reports this branch as conflicting with Please rebase or merge the latest |
Centralize platform-aware storage default selection so zero-config startup uses embedded SeekDB only when available, and falls back to SQLite otherwise. Expose memory service readiness and SQLite capability limitations through server status and the dashboard. Closes oceanbase#992
8328d4c to
50a9b5c
Compare
|
Thanks @wayyoungboy. Rebased on the latest The only conflict was a non-semantic import ordering in Test plan still green:
Ready for re-review whenever you have a moment. |
wayyoungboy
left a comment
There was a problem hiding this comment.
Thanks for rebasing. I rechecked the latest head (50a9b5c). The previous merge conflict is resolved and the GitHub checks are green, but I still see a few items to fix before LGTM:
-
The new default-provider test is not hermetic, and it exposes an explicit-vs-default mismatch for env-file based config.
DatabaseSettingsreads the auto-detected settings env file throughsettings_config()/_DEFAULT_ENV_FILE, butdefault_database_provider()anddatabase_provider_explicitly_configured()only inspectos.environ. With a temporary HOME containing.powermem/.envandDATABASE_PROVIDER=oceanbase,DatabaseSettings().providerresolves tooceanbasewhiledefault_database_provider()resolves tosqlite, sotests/unit/test_seekdb_default_storage.py::test_database_settings_default_provider_matches_platform_helperfails. Please either make the test isolate settings env-file loading, or make the platform default/defaulted helpers use the same source of truth asDatabaseSettings. -
/api/v1/system/healthis public, but it now returns rawstartup_errortext from service initialization. Those exceptions can include deployment details such as dependency errors, local paths, or database connection information. Please keep the public health response coarse/sanitized, and expose detailed startup errors only via authenticated status endpoints or logs. -
git diff --check origin/main...HEADcurrently fails withsrc/server/utils/service_errors.py:19: new blank line at EOF.
Validation I ran:
- Activated
.venvand ranpython -m pytest tests/unit/test_storage_default_provider.py tests/unit/test_seekdb_default_storage.py -q: 1 failed, 9 passed. - Activated
.venvand ran the same tests with a temporary clean HOME: 10 passed. - Activated
.venvand ranpython -m pytest tests/unit/test_config_loader.py tests/unit/test_cli_config_show.py -qwith a temporary clean HOME: 9 passed. - Activated
.venvand ranpython -m pytest tests/unit/server -qwith a temporary clean HOME: 44 passed. - Activated
.venvand ranpython -m pip check: no broken requirements.
|
Thanks @wayyoungboy. I addressed the three review items in the latest push (
Validation:
Please re-review when you have a chance. |
|
Rechecked the current PR state after the latest author update. GitHub currently reports this branch as conflicting with The existing requested-changes review remains the active review state for now. After the conflict is resolved, please ping for a fresh review of the rebased diff and updated test results. |
Resolve the setup documentation conflict and keep the storage fallback behavior aligned with the latest server and documentation changes.
|
@wayyoungboy Thank you for the careful recheck and review. I merged the latest What changed in this update:
Validation:
Please take another fresh look when convenient. |
wayyoungboy
left a comment
There was a problem hiding this comment.
Rechecked the current head after the latest merge/rebase. The previously requested items appear addressed in the diff:
- The platform default helper now reads the same explicit provider sources as
DatabaseSettings(process env first, then configured env files), so env-file basedDATABASE_PROVIDER/OCEANBASE_HOSTno longer diverges from the default-provider decision. - Public
/api/v1/system/healthno longer exposes raw startup errors; the added test checks that sensitive-looking startup details are absent from the public health response. - The native dependency probe now handles modules with broken / missing specs as unavailable, which covers the CI stability issue around storage probes.
- The latest status checks are green and GitHub reports the branch as mergeable.
I did not find a new blocking issue in this static re-review. I am not turning this into an approval in this pass because I did not run the PR test plan locally.
|
LGTM |
…eanbase#993) * fix(storage): fall back to sqlite when seekdb is unavailable Centralize platform-aware storage default selection so zero-config startup uses embedded SeekDB only when available, and falls back to SQLite otherwise. Expose memory service readiness and SQLite capability limitations through server status and the dashboard. Closes oceanbase#992 * fix(ci): stabilize storage probes and extension build * fix(storage): align defaults with env-file config
Summary
powermem.platform_defaultswithchoose_default_database_provider()to centralize zero-config storage selection.DATABASE_PROVIDERandOCEANBASE_HOSTare unset:./data/powermem_dev.db).pyobvector,pyseekdb,pylibseekdb); uses OceanBase embedded mode when available, otherwise falls back to SQLite with a WARNING.storage_capabilitieson/api/v1/system/statusso clients can see SQLite limitations (no Graph Store, sub_stores, sparse vectors, SkillStore)..env.example/.env.example.fullcomments to document platform-aware defaults.Closes #992
Test plan
pytest tests/unit/test_storage_default_provider.pypytest tests/unit/test_seekdb_default_storage.pypowermem-serverstarts, memory APIs return 200 (not 503)powermem[seekdb]: zero-config still uses embedded SeekDB at./seekdb_dataDATABASE_PROVIDER=oceanbaseon non-Linux still surfaces a clear validation error (no silent override)