feat(py): close querychat-created chatlas clients in cleanup() - #286
Draft
cpsievert wants to merge 3 commits into
Draft
feat(py): close querychat-created chatlas clients in cleanup()#286cpsievert wants to merge 3 commits into
cpsievert wants to merge 3 commits into
Conversation
ellmer 0.5.0 moves model details from Provider into a new Model class: Provider() no longer accepts model as its second positional argument, and Chat$new() requires a separate model argument. Gate the mock chat client construction on whether ellmer::Model exists so tests pass on both old and new ellmer. Fixes #283
DuckDB 1.3.0+ and SQLite reject a CTE whose name matches a table it references (e.g., WITH t AS (SELECT ... FROM t) SELECT * FROM t), raising a 'circular reference' error. This broke 4 TblSqlSource tests that use transformed tbls (CTE mode). The fix schema-qualifies table references in the CTE body (e.g., FROM main.test_table instead of FROM test_table), which disambiguates the CTE name from the physical table. The schema name is obtained via current_schema() with a fallback to 'main' for databases that don't support it (e.g., SQLite).
QueryChatBase.cleanup() now closes the chatlas client, but only when querychat created it (client was None or a string spec resolved via ChatAuto). User-supplied Chat instances are never closed -- their lifecycle stays with the caller. Session/console/greeter clients are deepcopy clones sharing the base provider, so closing the owned base client releases their resources too. Ownership is tracked via an _owns_client flag captured at construction, since the original client argument is discarded after resolution. Temporarily pins chatlas to the Chat.close() PR (posit-dev/chatlas#427); must be replaced with a released version before merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
chatlas PR #427 adds
Chat.close()/close_async()to release provider resources (HTTP connection pools, Snowflake sessions, MCP sessions). This PR adopts that API in querychat under a clear ownership rule:Changes
QueryChatBase.__init__records an_owns_clientflag at construction (the originalclientargument is discarded after resolution, so ownership must be captured there)QueryChatBase.cleanup()now closes the base client when owned, in addition to the existing query-executor and data-source cleanuppyproject.toml— must be replaced with a releasedchatlas>=X.Y.Zbefore this merges (TODO comment in place)Design notes
session.on_endedhandler. Investigation showed every closable resource (data sources, query executor, DuckDB connection, base chat client) is app-scoped onQueryChatBaseand shared across sessions; session-scoped state is pure reactive values reclaimed by GC. Cleanup is an app-shutdown concern: callqc.cleanup()(e.g., viaatexit).copy.deepcopyclones that share the base client's provider by reference (chatlasChat.__deepcopy__keepsprovidershared), so closing the owned base client releases their underlying resources too. Closing a per-session clone would be actively harmful — it would close the shared HTTP clients out from under other live sessions.cleanup()is sufficient for now.Testing
New
pkg-py/tests/test_cleanup.py(7 tests): ownership flag for all threeclientargument forms, owned string/deferred clients closed oncleanup(), user-supplied client left open, clones covered via the shared provider, existing data-source cleanup preserved, and idempotency. Full pkg-py suite passes (588 passed; the 5 Gradio failures and playwright collection errors pre-exist onmain).