From 41f42109389ca2231bbd4e6fdfee2aea58448878 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Mon, 17 Aug 2026 23:32:55 -0700 Subject: [PATCH] chore: comment cleanup Comment-Gate: 44 quarantined --- .../0007_add_asset_semantics_version.py | 5 -- app/assets/database/models.py | 9 +-- app/assets/database/queries/semantics.py | 24 +------- app/assets/seeder.py | 4 +- app/assets/semantics/__init__.py | 27 ++------- app/assets/semantics/reproject_derived.py | 58 +++++-------------- app/assets/semantics/step.py | 8 +-- app/assets/services/path_utils.py | 8 +-- .../assets_test/test_semantics_reset.py | 30 +--------- tests-unit/seeder_test/test_seeder.py | 1 - 10 files changed, 29 insertions(+), 145 deletions(-) diff --git a/alembic_db/versions/0007_add_asset_semantics_version.py b/alembic_db/versions/0007_add_asset_semantics_version.py index 4aa446b03..b548352a8 100644 --- a/alembic_db/versions/0007_add_asset_semantics_version.py +++ b/alembic_db/versions/0007_add_asset_semantics_version.py @@ -1,11 +1,6 @@ """ Add asset_semantics_version table. -Alembic records the *shape* of the assets tables. This table records the -*meaning* of their contents: which generation of the derivation logic produced -the values currently stored in them. The two move independently, so they are -tracked independently. - Revision ID: 0007_add_asset_semantics_version Revises: 0006_add_loader_path Create Date: 2026-08-18 diff --git a/app/assets/database/models.py b/app/assets/database/models.py index df698a125..9320ccc78 100644 --- a/app/assets/database/models.py +++ b/app/assets/database/models.py @@ -24,14 +24,7 @@ from app.database.models import Base class AssetSemanticsVersion(Base): - """Which generation of the derivation logic produced this database's rows. - - Alembic tracks the *shape* of the assets tables; this tracks the *meaning* - of what is stored in them. A row can be structurally current and still hold - values a superseded rule computed, so the two versions move independently. - Reset steps in ``app.assets.semantics`` bring such rows forward and then - advance this stamp. Always a single row, keyed on id 1. - """ + """Which generation of the derivation logic wrote these rows -- deliberately not the Alembic version, which tracks shape rather than meaning.""" __tablename__ = "asset_semantics_version" diff --git a/app/assets/database/queries/semantics.py b/app/assets/database/queries/semantics.py index ff035114f..8e591cf19 100644 --- a/app/assets/database/queries/semantics.py +++ b/app/assets/database/queries/semantics.py @@ -1,4 +1,3 @@ -"""Queries backing the asset semantics reset (see ``app.assets.semantics``).""" from dataclasses import dataclass @@ -20,21 +19,17 @@ from app.assets.database.queries.common import ( from app.assets.database.queries.tags import ensure_tags_exist from app.assets.helpers import get_utc_now -# The version table holds exactly one row; the id is a constant, not a sequence. _VERSION_ROW_ID = 1 -# Tags this subsystem derives from the filesystem rather than being told. AUTOMATIC_TAG_ORIGIN = "automatic" def get_semantics_version(session: Session) -> int: - """Return the stored semantics version, or 0 if none has been stamped.""" row = session.get(AssetSemanticsVersion, _VERSION_ROW_ID) return int(row.version) if row is not None else 0 def set_semantics_version(session: Session, version: int) -> None: - """Stamp the semantics version. Caller commits.""" row = session.get(AssetSemanticsVersion, _VERSION_ROW_ID) if row is None: session.add( @@ -50,7 +45,6 @@ def set_semantics_version(session: Session, version: int) -> None: @dataclass(frozen=True) class DerivedStateRow: - """A file-backed reference and the state a reprojection may rewrite.""" reference_id: str file_path: str @@ -66,13 +60,7 @@ def get_file_backed_references_page( after_id: str | None, limit: int, ) -> list[DerivedStateRow]: - """Return up to ``limit`` file-backed references ordered by id, after ``after_id``. - - Keyset pagination on the primary key so a reprojection can walk the whole - table in bounded transactions without holding one open across the walk. - Soft-deleted references are included: their derived columns are as stale as - anyone else's, and an undelete would serve them. - """ + """Soft-deleted references are deliberately included: their derived columns are as stale as anyone else's.""" query = ( sa.select( AssetReference.id, @@ -108,7 +96,6 @@ def get_file_backed_references_page( def get_tags_by_reference( session: Session, reference_ids: list[str] ) -> dict[str, dict[str, str]]: - """Return {reference_id: {tag_name: origin}} for the given references.""" if not reference_ids: return {} @@ -129,7 +116,6 @@ def get_tags_by_reference( def bulk_set_loader_paths( session: Session, loader_paths: dict[str, str | None] ) -> None: - """Set loader_path on the given references. Caller commits.""" if not loader_paths: return session.execute( @@ -142,7 +128,6 @@ def bulk_set_loader_paths( def bulk_add_automatic_tags(session: Session, links: list[tuple[str, str]]) -> None: - """Attach (reference_id, tag_name) links with automatic origin. Caller commits.""" if not links: return @@ -171,12 +156,7 @@ def bulk_add_automatic_tags(session: Session, links: list[tuple[str, str]]) -> N def bulk_remove_automatic_tags(session: Session, links: list[tuple[str, str]]) -> None: - """Detach (reference_id, tag_name) links, but only automatic ones. - - The origin is re-asserted in the statement rather than trusted from the - caller's snapshot, so a manual tag can never be removed by this path. - Caller commits. - """ + """The origin is re-asserted below rather than trusted from the caller's snapshot.""" if not links: return diff --git a/app/assets/seeder.py b/app/assets/seeder.py index 977204a26..d45fb954f 100644 --- a/app/assets/seeder.py +++ b/app/assets/seeder.py @@ -549,9 +549,7 @@ class _AssetSeeder: ) return - # Stale rows are brought forward before anything reads or extends - # them. A database already at the current semantics version costs a - # single row read here. + # Must precede the prune and the scan: both read and extend these rows. run_pending_semantics_steps(interrupt_check=self._is_cancelled) if self._prune_first: diff --git a/app/assets/semantics/__init__.py b/app/assets/semantics/__init__.py index 7b2c850f1..a25ff819e 100644 --- a/app/assets/semantics/__init__.py +++ b/app/assets/semantics/__init__.py @@ -1,18 +1,10 @@ """Reset steps that bring stale asset rows forward to the current semantics. -Alembic migrates the *shape* of the assets tables. Nothing migrates their -*meaning*: a row whose columns are structurally current can still hold values -that a superseded rule computed, and the steady-state scan will not repair one, -because it only ever looks at paths it has not seen before. - -A reset step closes that gap. Each step is numbered, applied in order from the -version stamped in the database up to ``CURRENT_SEMANTICS_VERSION``, and stamped -only once it finishes, so an interrupted run resumes rather than half-applying. -Steps must be idempotent: re-running one on rows it has already fixed changes -nothing. - -The registry, not any individual step, is the durable part. A step is free to be -as broad or as surgical as its particular drift calls for. +Alembic migrates the shape of the assets tables; these steps migrate the meaning +of what is in them. A step must be idempotent, is applied in order from the +version stamped in the database, and is stamped only once it finishes, so an +interrupted run resumes rather than half-applying. How broad or surgical a step +is depends on the drift it repairs; the registry is what stays. """ import logging @@ -51,14 +43,7 @@ CURRENT_SEMANTICS_VERSION = SEMANTICS_STEPS[-1].version def run_pending_semantics_steps(interrupt_check: InterruptCheck | None = None) -> int: - """Apply every reset step this database has not been stamped for. - - A database already at ``CURRENT_SEMANTICS_VERSION`` costs one indexed row - read and nothing else -- no filesystem is touched to discover there is - nothing to do, which is the overwhelmingly common case. - - Returns the number of steps applied. - """ + """Returns steps applied. A database already at the current version costs one indexed row read.""" if not can_create_session(): return 0 diff --git a/app/assets/semantics/reproject_derived.py b/app/assets/semantics/reproject_derived.py index 177309118..b1a4bb7a3 100644 --- a/app/assets/semantics/reproject_derived.py +++ b/app/assets/semantics/reproject_derived.py @@ -1,30 +1,17 @@ """Semantics step 1: re-derive the reference state that comes from the path. -Three columns are pure functions of a file's location and whether it is there: -``loader_path``, the backend tags a path implies, and ``is_missing``. Every one -of them was computed once, when the reference was first written, by whatever -rules were current that day. ``loader_path`` in particular was added to existing -databases as a NULL column and no path has ever filled it in, so references -older than that column serve a null loader path forever. +Recomputes ``loader_path``, the backend tags a path implies, and ``is_missing``. -This step recomputes those three from the filesystem as it stands now. It reads -no file contents: ``verify_file_unchanged`` answers whether a file still matches -what the database recorded, and the only thing that answer is used for is -deciding *not* to touch content-derived state. Nothing here re-hashes, so a -library of untouched 500GB models costs one ``stat`` each. +It must keep reading no file contents, so an untouched model library costs one +``stat`` per file. That rules out repairing anything derived from content -- +``hash``, ``size_bytes``, mime -- so a file that has changed underneath its row +goes to the existing ``needs_verify`` path instead. -What the step will not touch: - -- ``hash`` and ``size_bytes`` -- content facts, unrecoverable without reading - the file. A file that has changed underneath its row is handed to the existing - ``needs_verify`` path instead, exactly as the scanner would flag it. -- Anything a person chose: manual and upload-origin tags, ``user_metadata``, - ``preview_id``, ``deleted_at``, ``job_id``, ``name``. -- References whose file is gone, or whose path is not under any root this - install currently knows about. Retiring the former is the scanner's job under - its own rules; the latter cannot be classified at all, and a misconfigured - ``extra_model_paths.yaml`` must not be able to strip tags off assets that are - merely out of view. +It must also leave alone anything a person chose (manual and upload-origin tags, +``user_metadata``, ``preview_id``, ``deleted_at``, ``job_id``, ``name``), +references whose file is gone, and references whose path is under no root this +install currently knows about -- a misconfigured ``extra_model_paths.yaml`` must +not be able to strip tags off assets that are merely out of view. """ import logging @@ -54,14 +41,11 @@ from app.assets.services.path_utils import ( ) from app.database.db import create_session -# Bounds each transaction. Small enough that an interrupt loses little work, -# large enough that the walk is not dominated by session setup. _BATCH_SIZE = 500 @dataclass class ReprojectionSummary: - """What a reprojection pass did, for the log line.""" scanned: int = 0 unchanged_files: int = 0 @@ -89,12 +73,7 @@ class ReprojectionSummary: def reproject_derived_state( interrupt_check: InterruptCheck | None = None, ) -> ReprojectionSummary: - """Re-derive path-derived state for every file-backed reference. - - Walks the reference table in keyset-paginated batches, committing each one, - so a kill mid-walk leaves committed batches reprojected and the rest - untouched -- both states the step handles on its next run. - """ + """Each batch commits on its own, so a kill mid-walk is safe to resume.""" summary = ReprojectionSummary() vocabulary = get_path_derived_tag_vocabulary() after_id: str | None = None @@ -142,9 +121,7 @@ def _reproject_batch( if unchanged: summary.unchanged_files += 1 else: - # The file moved on from what the row records, so its hash, size and - # mime no longer describe it. Re-deriving those means reading the - # file, which this step does not do; flag it for the path that does. + # Hand it to the verify path rather than re-read the file for hash and size. summary.changed_files += 1 if not row.needs_verify: set_needs_verify.append(row.reference_id) @@ -174,8 +151,8 @@ def _reproject_batch( tags_to_remove.append((row.reference_id, tag_name)) if row.is_missing: - # The file is right there. Nothing else un-flags this: the scanner - # excludes already-missing references from its temp reconciliation. + # Nothing else un-flags this: the scanner excludes already-missing + # references from its temp reconciliation. clear_missing.append(row.reference_id) bulk_set_loader_paths(session, loader_paths) @@ -191,12 +168,7 @@ def _reproject_batch( def _classify_file(row: DerivedStateRow) -> tuple[bool, bool]: - """Return (file is present, file still matches what the row recorded). - - Mirrors the scanner's stat handling so the two agree about what counts as a - present file: a permission error means the file is there but unreadable, any - other OS error means treat it as gone. - """ + """Mirrors the scanner's stat handling: permission denied means present-but-unreadable, any other OS error means gone.""" try: stat_result = os.stat(row.file_path, follow_symlinks=True) except FileNotFoundError: diff --git a/app/assets/semantics/step.py b/app/assets/semantics/step.py index a3bf0fe03..61d6a3208 100644 --- a/app/assets/semantics/step.py +++ b/app/assets/semantics/step.py @@ -1,4 +1,3 @@ -"""What a semantics reset step is. See ``app.assets.semantics`` for the why.""" from dataclasses import dataclass from typing import Callable @@ -16,12 +15,7 @@ class SemanticsStepInterrupted(Exception): @dataclass(frozen=True) class SemanticsStep: - """One numbered reset step. - - ``apply`` takes an optional interrupt check and returns a summary of what it - did, which is logged. It must be idempotent and must raise - ``SemanticsStepInterrupted`` rather than return if it stops early. - """ + """``apply`` must be idempotent, and must raise SemanticsStepInterrupted rather than return early.""" version: int description: str diff --git a/app/assets/services/path_utils.py b/app/assets/services/path_utils.py index 7e46e6a03..d47fccd02 100644 --- a/app/assets/services/path_utils.py +++ b/app/assets/services/path_utils.py @@ -326,13 +326,7 @@ def get_path_derived_tags_from_path(path: str) -> list[str]: def get_path_derived_tag_vocabulary() -> set[str]: - """Every tag get_path_derived_tags_from_path can emit under the current config. - - Bounds which tags a re-derivation may take away: a stored automatic tag - inside this vocabulary that the current rules no longer emit was produced - by a superseded rule, while one outside it came from somewhere else and is - none of the derivation's business. - """ + """Bounds which stored tags a re-derivation may take away; one outside this set came from elsewhere.""" vocabulary = {"input", "output", "temp", "models"} vocabulary.update(_KNOWN_SUBFOLDER_TAGS) for folder_name, _bases, _extensions in get_comfy_models_folders(): diff --git a/tests-unit/assets_test/test_semantics_reset.py b/tests-unit/assets_test/test_semantics_reset.py index 0eb6f3d1c..bf08476c4 100644 --- a/tests-unit/assets_test/test_semantics_reset.py +++ b/tests-unit/assets_test/test_semantics_reset.py @@ -1,9 +1,4 @@ -"""Tests for the asset semantics reset (app/assets/semantics). - -Runs standalone against in-memory SQLite: - - pytest tests-unit/assets_test/test_semantics_reset.py --noconftest -""" +"""Tests for the asset semantics reset (app/assets/semantics).""" import os import tempfile @@ -34,17 +29,12 @@ from app.assets.services.file_utils import get_mtime_ns @pytest.fixture(autouse=True) def autoclean_unit_test_assets(): - """Override the package autouse fixture; these tests need no server.""" + """Override parent autouse fixture - these tests don't need server cleanup.""" yield @pytest.fixture def session_factory(): - """A session factory over one shared in-memory database. - - StaticPool keeps every session on the same connection, so the batching walk - sees its own committed writes across sessions. - """ engine = create_engine( "sqlite:///:memory:", poolclass=StaticPool, @@ -62,14 +52,12 @@ def session_factory(): @pytest.fixture def session(session_factory) -> Session: - """A session for the test's own reads and writes.""" with session_factory() as sess: yield sess @pytest.fixture def comfy_dirs(): - """Point every asset root at a throwaway tree with one model category.""" with tempfile.TemporaryDirectory() as base: dirs = { name: Path(base) / name @@ -118,10 +106,6 @@ def _register( job_id: str | None = None, tags: dict[str, str] | None = None, ) -> AssetReference: - """Insert an Asset + AssetReference (+ tags as {name: origin}) and commit. - - ``asset_hash=""`` means "any unique hash"; assets.hash is unique-indexed. - """ if asset_hash == "": asset_hash = f"blake3:{ref_id}" if mtime_ns is None: @@ -168,7 +152,6 @@ def _tags(session: Session, ref_id: str) -> dict[str, str]: def _snapshot(session: Session) -> list[tuple]: - """Every field the reset could plausibly touch, for equality across runs.""" session.expire_all() refs = session.execute(select(AssetReference).order_by(AssetReference.id)).scalars() rows = [ @@ -195,7 +178,6 @@ def _snapshot(session: Session) -> list[tuple]: class TestLoaderPathReprojection: def test_null_loader_path_is_backfilled(self, session, comfy_dirs): - """The drift 0006 left behind: a column added with no backfill.""" path = _write(comfy_dirs["checkpoints"], "flux/model.safetensors") _register(session, path, "ref-1", loader_path=None) @@ -224,7 +206,6 @@ class TestLoaderPathReprojection: assert summary.loader_paths_rewritten == 0 def test_unloadable_extension_loses_its_loader_path(self, session, comfy_dirs): - """The current rule gives no loader path to a file its category cannot load.""" path = _write(comfy_dirs["checkpoints"], "notes.txt") _register(session, path, "ref-1", loader_path="notes.txt") @@ -269,7 +250,6 @@ class TestHashPreservation: def test_changed_file_keeps_its_hash_and_is_flagged_for_verify( self, session, comfy_dirs ): - """A file that moved on is handed to the existing verify path, not re-read.""" path = _write(comfy_dirs["checkpoints"], "big.safetensors") _register( session, @@ -335,13 +315,11 @@ class TestIntentIsPreserved: tags = _tags(session, "ref-1") assert tags["favourite"] == "manual" assert tags["uploaded"] == "upload" - # ...while the derived state around them was still brought forward. assert ref.loader_path == "curated.safetensors" def test_manual_tag_inside_the_derived_vocabulary_is_not_removed( self, session, comfy_dirs ): - """A person may tag an input file 'models'; that is their business.""" path = _write(comfy_dirs["input"], "photo.png") _register(session, path, "ref-1", tags={"models": "manual"}) @@ -363,7 +341,6 @@ class TestTagReprojection: } def test_superseded_automatic_tag_is_removed(self, session, comfy_dirs): - """An older rule tagged by directory alone; extensions now gate the tag.""" path = _write(comfy_dirs["checkpoints"], "model.safetensors") _register( session, @@ -384,7 +361,6 @@ class TestTagReprojection: def test_automatic_tag_outside_the_vocabulary_is_left_alone( self, session, comfy_dirs ): - """'missing' is automatic but not path-derived; the scanner owns it.""" path = _write(comfy_dirs["checkpoints"], "model.safetensors") _register(session, path, "ref-1", tags={"missing": "automatic"}) @@ -424,7 +400,6 @@ class TestFileState: assert summary.absent_files == 1 def test_path_outside_every_known_root_is_left_alone(self, session, comfy_dirs): - """A root missing from the config must not strip tags off its assets.""" path = _write(comfy_dirs["elsewhere"], "model.safetensors") _register( session, @@ -488,7 +463,6 @@ class TestIdempotence: def test_statements_chunked_by_bind_param_limit_stay_correct( self, session, comfy_dirs ): - """The bind-param chunking must not drop or duplicate a tag.""" for index in range(4): _register( session, diff --git a/tests-unit/seeder_test/test_seeder.py b/tests-unit/seeder_test/test_seeder.py index 566069d41..f6e49bfa4 100644 --- a/tests-unit/seeder_test/test_seeder.py +++ b/tests-unit/seeder_test/test_seeder.py @@ -498,7 +498,6 @@ class TestSeederMarkMissing: class TestSeederSemanticsReset: - """The scan brings stale rows forward before it reads or extends them.""" def test_semantics_reset_runs_before_any_scan_work( self, fresh_seeder: _AssetSeeder