From 1b3334b5afbf7a18ad8017be09be943fadcb90f1 Mon Sep 17 00:00:00 2001 From: Yiorgis Gozadinos Date: Wed, 26 Aug 2026 13:29:00 +0300 Subject: [PATCH] Trim the comments to what a reader needs One sentence for the contract, one or two more only where an invariant is not obvious. The reasons that stay are about correctness and ownership: which database a result belongs to, who closes what, why assembly order is the tiebreak. The ones that go narrated how the code got here. --- haiku_rag_slim/haiku/rag/app.py | 7 +--- .../rag/chat/widgets/document_filter_modal.py | 5 ++- haiku_rag_slim/haiku/rag/client/__init__.py | 7 +--- haiku_rag_slim/haiku/rag/client/scope.py | 36 +++++++------------ haiku_rag_slim/haiku/rag/client/session.py | 19 ++++------ haiku_rag_slim/haiku/rag/sandbox/sandbox.py | 11 +++--- .../haiku/rag/store/models/chunk.py | 14 +++----- 7 files changed, 31 insertions(+), 68 deletions(-) diff --git a/haiku_rag_slim/haiku/rag/app.py b/haiku_rag_slim/haiku/rag/app.py index 7e7a2cc7..d9712a83 100644 --- a/haiku_rag_slim/haiku/rag/app.py +++ b/haiku_rag_slim/haiku/rag/app.py @@ -39,12 +39,7 @@ class HaikuRAGApp: config: AppConfig | None = None, read_only: bool = False, ): - """The databases this command works on, resolved by whoever built it. - - One selector, not three: a command that took a path and a name and a - scope would have to decide between them, which is the deciding this - layer exists to have already done. - """ + """The databases this command works on, resolved by whoever built it.""" self.scope = scope self.config = config if config is not None else get_config() self.read_only = read_only diff --git a/haiku_rag_slim/haiku/rag/chat/widgets/document_filter_modal.py b/haiku_rag_slim/haiku/rag/chat/widgets/document_filter_modal.py index ef556a54..70e61147 100644 --- a/haiku_rag_slim/haiku/rag/chat/widgets/document_filter_modal.py +++ b/haiku_rag_slim/haiku/rag/chat/widgets/document_filter_modal.py @@ -8,9 +8,8 @@ from textual.widgets import Button, Checkbox, Input, Static from haiku.rag.client import HaikuRAG from haiku.rag.utils import escape_sql_string -# One page of documents. A corpus is not a list to scroll: mounting a checkbox -# per document wedges the modal at tens of thousands, so the list is a page and -# the search box asks the database for the rest. +# Documents listed at once. Mounting a checkbox per document wedges the modal on +# a large corpus, so the rest is reached through the search box. DOCUMENT_PAGE = 200 diff --git a/haiku_rag_slim/haiku/rag/client/__init__.py b/haiku_rag_slim/haiku/rag/client/__init__.py index 90a114af..baf13a1b 100644 --- a/haiku_rag_slim/haiku/rag/client/__init__.py +++ b/haiku_rag_slim/haiku/rag/client/__init__.py @@ -360,12 +360,7 @@ class HaikuRAG: ] def _facade_for(self, name: str, session: SingleDatabaseSession) -> "HaikuRAG": - """The client for one covered database, made once and kept. - - The facade is public and the session is not, so the wrapper lives here - while the federated session keeps the database it wraps. A borrowed facade - never closes what it did not open. - """ + """The cached client borrowing this session, made once and kept.""" facade = self._clients.get(name) if facade is None: facade = HaikuRAG._from_session(session) diff --git a/haiku_rag_slim/haiku/rag/client/scope.py b/haiku_rag_slim/haiku/rag/client/scope.py index 4d2dbe2b..0aa15068 100644 --- a/haiku_rag_slim/haiku/rag/client/scope.py +++ b/haiku_rag_slim/haiku/rag/client/scope.py @@ -8,20 +8,12 @@ from haiku.rag.utils import locate_database @dataclass(frozen=True) class DatabaseRef: - """One database, and the name it answers to. + """A resolved database location, and the configured name it answers to. - ``name`` is the key from ``lancedb.databases``, and the only identity that - leaves the configuration: it travels in results, citations and the errors an - operator or a model sees, where a location must not. The invariant below is - the exception, and deliberately so — a malformed ref is a programming error - raised in the caller's own process, where naming what it was given is what - makes it fixable. None where nothing names the database — a path given - directly, or the legacy single ``uri``. - - Location is resolved once, on construction, into exactly one of ``uri`` and - ``db_path``. Keeping the configured string and re-reading it later would let a - path the caller gave be reinterpreted as a URI because it happens to carry a - scheme. + Exactly one of ``uri`` and ``db_path`` is set. ``name`` is the key from + ``lancedb.databases``, and the only identity that leaves the configuration: + it travels in results, citations and errors, where a location must not. + None where nothing names the database. """ name: str | None @@ -42,18 +34,16 @@ class DatabaseRef: @classmethod def configured(cls, name: str | None, location: str) -> "DatabaseRef": - """A database the configuration placed, by ``lancedb.uri`` or by an entry - in ``lancedb.databases``. A value carrying a scheme is a URI and anything - else is a local path, so the two settings place a database alike.""" + """A database the configuration placed, by ``lancedb.uri`` or an entry in + ``lancedb.databases``. A location carrying a scheme is a URI, anything + else a local path.""" uri, db_path = locate_database(location) return cls(name=name, uri=uri, db_path=db_path) def connection(self, config: AppConfig) -> tuple[AppConfig, Path | None]: """The configuration and path to open this one database with. - A copy. The scope is resolved once from the caller's configuration, and - rewriting that configuration in place is what left downstream code unable - to tell that a set had been named. + A copy: the caller's configuration still names whatever set it named. """ one = config.model_copy(deep=True) one.lancedb.databases = {} @@ -66,12 +56,10 @@ class DatabaseScope: """The databases an operation covers. Resolved once, from configuration plus at most one selector, and passed down - rather than re-derived. Never empty: every resolution reaches a database, and - the sessions built from a scope have no meaning without one. + rather than re-derived. Never empty. - Nothing here reads the environment. ``HAIKU_RAG_DB`` is honoured by the - capability entry point alone, which passes it as ``database_path``, so - resolving a scope cannot quietly change what any other caller opens. + Nothing here reads the environment: ``HAIKU_RAG_DB`` is the capability entry + point's to honour, and reading it here would extend it to every caller. """ databases: tuple[DatabaseRef, ...] diff --git a/haiku_rag_slim/haiku/rag/client/session.py b/haiku_rag_slim/haiku/rag/client/session.py index 623e5b4b..ee69032d 100644 --- a/haiku_rag_slim/haiku/rag/client/session.py +++ b/haiku_rag_slim/haiku/rag/client/session.py @@ -58,9 +58,8 @@ class SingleDatabaseSession: it has one. ``source`` is the configured name this database answers to, or None where nothing names it. - ``db_path``, ``config``, ``read_only`` and ``source`` are readable because a - client facade is built over a session it does not own, and needs to report - the same things it would have reported had it opened the database itself. + ``db_path``, ``config``, ``read_only`` and ``source`` are readable so that a + client borrowing this session can report them as its own. """ def __init__( @@ -131,12 +130,9 @@ class SingleDatabaseSession: async def drain_vacuum(self) -> None: """Drain background vacuum work and run a final collapse before teardown. - Writes schedule a throttled background vacuum; many are debounced or skip - because another vacuum holds the lock. The final pass collapses the - versions those left behind. It runs whenever writes happened - (``_vacuum_dirty``) — not gated on in-flight tasks remaining, since a - debounced run may have scheduled none — but never when nothing was - written (so opening + closing a store still never writes). + The final pass runs whenever writes happened, not when tasks remain: a + debounced run may have scheduled none and still left versions behind. It + never runs without writes, so opening and closing a store writes nothing. """ if self._vacuum_tasks: await asyncio.gather(*self._vacuum_tasks, return_exceptions=True) @@ -251,15 +247,12 @@ class SingleDatabaseSession: class FederatedSession: - """Several databases, read as one. + """Several databases, read as one. Reads only. Composes single-database sessions and owns their teardown. They open on first use rather than at entry: which databases a query covers is a per-query choice, so a database nobody asked for must neither be opened for nothing nor be able to fail a query. - - Reads only. Writing names a database, and naming one is what - ``SingleDatabaseSession`` is. """ def __init__( diff --git a/haiku_rag_slim/haiku/rag/sandbox/sandbox.py b/haiku_rag_slim/haiku/rag/sandbox/sandbox.py index e84f0806..9cf2d093 100644 --- a/haiku_rag_slim/haiku/rag/sandbox/sandbox.py +++ b/haiku_rag_slim/haiku/rag/sandbox/sandbox.py @@ -272,14 +272,11 @@ class Sandbox: def _holders( owners: "list[HaikuRAG]", groups: "list[list[Any]]" ) -> "dict[str, HaikuRAG]": - """Map each document id to the database holding it. + """Map each document id to the database holding it, rejecting an id two + databases claim. - Document ids are UUID4, so the flat `/documents/{id}/` namespace is - unambiguous for databases that were filled independently — but not for one - copied from another, where the same id is in both. Duplicate results are - merely redundant in a search; here they would be two documents claiming one - path, and whichever arrived last would answer for both. Rejected rather - than resolved, since either answer would be wrong half the time. + The mount has one `/documents/{id}/` path per id, so a copied database + would put two documents on one path and the last would answer for both. """ holders: dict[str, HaikuRAG] = {} held_by: dict[str, str | None] = {} diff --git a/haiku_rag_slim/haiku/rag/store/models/chunk.py b/haiku_rag_slim/haiku/rag/store/models/chunk.py index c363d64f..9e28e393 100644 --- a/haiku_rag_slim/haiku/rag/store/models/chunk.py +++ b/haiku_rag_slim/haiku/rag/store/models/chunk.py @@ -117,16 +117,12 @@ SearchType = Literal["vector", "fts", "hybrid"] def qualified_id(source: str | None, id: str | None) -> tuple[str | None, str | None]: - """What tells one chunk from another. + """What tells one chunk from another: a chunk id is unique within a database + and says nothing across them. - A chunk id is unique within a database and says nothing across them: a - database copied from another holds the same ids, so the database is part of - the identity. `id` is optional because a result built by hand carries none, - and those cannot be told apart. - - Only for structures held in memory. Everything serialized — citations, - evidence refs, compaction — records the id alone, so ambiguity there is - rejected rather than qualified. + For in-memory structures only. Everything serialized records the id alone + and rejects ambiguity instead. `id` is optional, and results built by hand + carry none and cannot be told apart. """ return (source, id)