What we want
get_settings() should read the environment when it is called, not when app.config is first imported.
Why it matters
app/config.py:
@dataclass(frozen=True)
class Settings:
database_url: str = os.getenv("DATABASE_URL", "postgresql://vaultrag:vaultrag@localhost:5433/vaultrag")
embedder: str = os.getenv("EMBEDDER", "local")
...
def get_settings() -> Settings:
return Settings()
Those os.getenv calls are dataclass field defaults, so they are evaluated once, when the class body runs at import time. get_settings() then just re-uses the frozen defaults. Setting DATABASE_URL, EMBEDDER, or LLM after app.config has been imported has no effect at all, which is not what a function named get_settings looks like it does.
This is not hypothetical. tests/test_api.py has to work around it, and says so:
os.environ.setdefault("EMBEDDER", "fake")
os.environ.setdefault("LLM", "fake")
os.environ["DATABASE_URL"] = os.getenv("TEST_DATABASE_URL", "...vaultrag_test")
from app.db import reset_schema # noqa: E402
from app.main import app # noqa: E402
Two # noqa: E402 markers exist purely because the environment has to be mutated before the import. tests/conftest.py does the same thing at line 161. That fragility matters here more than usual: the README (the "Two bugs this found in its own repo" section) records that an earlier version of exactly this setup pointed the test suite at the working database and deleted the demo corpus, after which the eval reported 0% leaks against an empty corpus. Import-order-dependent configuration is the mechanism that made that possible.
It also means a caller cannot construct Settings for a different target at runtime, for example to run the eval against a second database in one process.
Suggested approach
- Change the fields in
app/config.py so the environment is read per call. Either give Settings plain annotated fields with no defaults and build it inside get_settings(), or use dataclasses.field(default_factory=...) per field. Keep the same env var names and the same fallback values so nothing else changes.
- Keep
Settings frozen. The goal is late binding, not mutability.
- If you want it cached again afterwards, make that explicit and opt in (for example
functools.lru_cache on get_settings), rather than an accident of class-body evaluation. Say which you chose in the PR.
- Add a small test,
tests/test_config.py, using monkeypatch.setenv to show that get_settings() picks up a changed EMBEDDER value. That test is the whole proof, and it fails on main today.
- Optional follow up, only if it stays simple: with late binding in place, the
# noqa: E402 import dance in tests/test_api.py and tests/conftest.py may no longer be needed. Removing it is welcome but not required.
Running it
docker compose up -d db
pytest -q
Comment here if you would like to take this one and I will assign it. I usually reply within a day.
What we want
get_settings()should read the environment when it is called, not whenapp.configis first imported.Why it matters
app/config.py:Those
os.getenvcalls are dataclass field defaults, so they are evaluated once, when the class body runs at import time.get_settings()then just re-uses the frozen defaults. SettingDATABASE_URL,EMBEDDER, orLLMafterapp.confighas been imported has no effect at all, which is not what a function namedget_settingslooks like it does.This is not hypothetical.
tests/test_api.pyhas to work around it, and says so:Two
# noqa: E402markers exist purely because the environment has to be mutated before the import.tests/conftest.pydoes the same thing at line 161. That fragility matters here more than usual: the README (the "Two bugs this found in its own repo" section) records that an earlier version of exactly this setup pointed the test suite at the working database and deleted the demo corpus, after which the eval reported 0% leaks against an empty corpus. Import-order-dependent configuration is the mechanism that made that possible.It also means a caller cannot construct
Settingsfor a different target at runtime, for example to run the eval against a second database in one process.Suggested approach
app/config.pyso the environment is read per call. Either giveSettingsplain annotated fields with no defaults and build it insideget_settings(), or usedataclasses.field(default_factory=...)per field. Keep the same env var names and the same fallback values so nothing else changes.Settingsfrozen. The goal is late binding, not mutability.functools.lru_cacheonget_settings), rather than an accident of class-body evaluation. Say which you chose in the PR.tests/test_config.py, usingmonkeypatch.setenvto show thatget_settings()picks up a changedEMBEDDERvalue. That test is the whole proof, and it fails onmaintoday.# noqa: E402import dance intests/test_api.pyandtests/conftest.pymay no longer be needed. Removing it is welcome but not required.Running it
Comment here if you would like to take this one and I will assign it. I usually reply within a day.