fix: Use singleton PostgresDBClient (Sqlalchemy engine) #321
Conversation
Promptfoo Evaluation Results
|
| promptfoo eval --max-concurrency 1 --config "/tmp/promptfooconfig.processed.yaml" --share --output "${OUTPUT_JSON_FILE}" --no-cache | tee "${EVAL_OUTPUT_FILE}" | ||
| promptfoo eval --config "/tmp/promptfooconfig.processed.yaml" --share --output "${OUTPUT_JSON_FILE}" --no-cache | tee "${EVAL_OUTPUT_FILE}" | ||
| else | ||
| promptfoo eval --max-concurrency 1 --config "/tmp/promptfooconfig.processed.yaml" --output "${OUTPUT_JSON_FILE}" --no-cache | tee "${EVAL_OUTPUT_FILE}" | ||
| promptfoo eval --config "/tmp/promptfooconfig.processed.yaml" --output "${OUTPUT_JSON_FILE}" --no-cache | tee "${EVAL_OUTPUT_FILE}" |
There was a problem hiding this comment.
Reverts #317, so it should default back to 4 threads
| @cached_property | ||
| def db_client(self) -> db.PostgresDBClient: | ||
| return db.PostgresDBClient() |
There was a problem hiding this comment.
This basically creates a singleton PostgresDBClient instance, which holds the Sqlalchemy engine.
“the engine is thread safe yes. individual Connection objects are not. we try to describe this at Working with Engines and Connections — SQLAlchemy 2.0 Documentation”
| return db.PostgresDBClient().get_session() | ||
| return self.db_client.get_session() |
There was a problem hiding this comment.
A new session uses an available connection (from the connection pool), so threads (e.g., those created from API calls) will not share a connection.
Our pool size is 20 – what happens when max sqlalchemy pool size is reached?
Google’s AI states:
Requests are queued: Any new requests for a database connection are placed in a queue, waiting for a connection to become available.
Timeout begins: SQLAlchemy starts a timeout period (defaulting to 30 seconds, but configurable) to see if a connection is released back into the pool.
Connection timeout error: If a connection doesn't become available within the timeout period, an exception (e.g., TimeoutError) is thrown, indicating a connection timeout.
SQLAlchemy connection pooling, what are checked out connections? : “If all the connections are simultaneously checked out then you can expect an error (there will be a timeout period during which SQLAlchemy waits to see if a connection gets freed up; this is also configurable).”
Promptfoo Evaluation Results
|
Promptfoo Evaluation Results
|
Promptfoo Evaluation Results
|
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Promptfoo Evaluation Results
|
Promptfoo Evaluation Results
|
|
Promptfoo run with Gemini 2.5 pro: https://github.com/navapbc/labs-decision-support-tool/actions/runs/15737577700 |
|
DB connections spiked but went down: |
Promptfoo Evaluation Results
|
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes database session management by introducing a singleton PostgresDBClient, updates tests to use the app_config fixture for obtaining test sessions, and reverts the Promptfoo GH action to its default concurrency.
- Introduce a cached
db_clientinAppConfigand routedb_sessionthrough it - Update pytest fixtures and test signatures to include
app_config - Remove explicit
--max-concurrencyflags in Promptfoo workflow to restore default threading
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/src/app_config.py | Added db_client cached property, updated db_session to use it |
| app/tests/conftest.py | Changed app_config fixture to depend on db_client |
| app/tests/src/test_retrieve.py and other tests | Updated test signatures to include app_config and use chunk.id |
| .github/workflows/promptfoo-googlesheet-evaluation.yml | Removed --max-concurrency flags to revert to default concurrency |
Comments suppressed due to low confidence (2)
app/src/app_config.py:45
- The
cached_propertydecorator is used but not imported; addfrom functools import cached_propertyat the top of the file.
@cached_property
app/tests/conftest.py:111
- The
db_clientfixture is referenced here but not defined; consider adding adb_clientpytest fixture or switch back to using the existingdb_sessionfixture.
def app_config(monkeypatch, db_client: db.DBClient):
Promptfoo Evaluation Results
|
Promptfoo Evaluation Results
|
| assert results[0].chunk == short_chunk | ||
| assert results[0].chunk.id == short_chunk.id |
There was a problem hiding this comment.
What's the reason for these changes? Because the db_session from the test fixture (the second parameter to test_retrieve_with_scores) is different from the db_session generated by retrieve_with_scores?
There was a problem hiding this comment.
Yes. There's some identifier in the chunk instances that is specific to the db_session, so those identifiers are different and cause the assertion to fail. Otherwise the chunks are identical.
There was a problem hiding this comment.
Gotcha. I think the identifier here is the literal address in memory: under the hood, SQLAlchemy ensures that if you retrieve the same row back in the same session, it creates only a single instance of the object in memory so you can do things like (psuedocode):
with some_session:
# in this call SQLAlchemy creates a new instance of the Chunk class and returns that
chunk_1 = some_session.select(chunk_id="abc").first()
# in this call SQLAlchemy will recognize that it already has an instance of the Chunk class for this row, so it will return that Chunk
chunk_2 = some_session.select(chunk_id="abc").first()
assert chunk_1 == chunk_2 # these are literally the same object in memory
Ticket
https://navalabs.atlassian.net/browse/DST-1025
Test changes for https://navalabs.atlassian.net/browse/DST-1042
Changes
Create a single PostgresDBClient instance, and create sessions from that instance.
Reverts #317, so Promptfoo GH action should default back to 4 threads.
Fixed tests that indirectly create a DB session so that they use the
app_configtest fixture (which accesses the test DB schema) rather than the non-testapp_config(which accesses the real DB schema).Testing
Tested against Gemini LLM: #321 (comment)
and posted resulting DB connections: #321 (comment)
Preview environment for app
♻️ Environment destroyed ♻️