diff --git a/TESTING.md b/TESTING.md index 37b998f6..603f53e7 100644 --- a/TESTING.md +++ b/TESTING.md @@ -1208,13 +1208,18 @@ The Images selection bar shows the concept's **Add tags** after the count, then **Auto-tag selected** (D85), before Set category. Both take the checked files when pressed, in listing order, as their reveal identity (`{root_index, root_scope, path}`); more than 500 toasts "Select at most 500 images." and opens nothing. The selection -stays after every start, refusal and review. +stays after every start, refusal and review. The job's start body is bounded so 500 +targets of the longest legal path (1024 code points, even escaped as `\uXXXX`) fit: +`500 × (1024 × 6 + 256) + 64 KiB`; a larger body answers 413 +(`tests/test_image_tag_jobs.py` "test_start_body_bounds"). - **Add tags** opens the concept's `simpleInput` (app.js a1378–1383) as Set category does: "Tag selected images", "Tags, separated by commas" (empty: the concept's "cinematic, concept" is sample text), Cancel / Apply. The names are parsed as the Tag image dialog's manual field parses them (trimmed, case-insensitive repeats dropped, - first spelling wins); none toasts "Enter at least one tag.". Apply starts the + first spelling wins); a field that parses to no names keeps the dialog open with the + typed text and reports "Enter at least one tag." on the field (its native validity, + cleared on the next edit), and nothing is sent. Apply starts the `image-tags` job (`POST /prompt_manager/jobs/image-tags`, `{mode: "add", targets, tags}`) and toasts "Tagging started."; Activity shows its progress. - A refused start toasts why and changes nothing: another record-changing job (a scan) @@ -1249,16 +1254,20 @@ stays after every start, refusal and review. running; its actions are Stop reviewing, Skip and Approve and next (Approve and finish on the last). Approve saves the manual field plus the remaining chips as Save does, without its per-image toast, and opens the next image; Skip writes nothing and - advances; Stop reviewing, Escape and × end the session. Each image is one gated + advances; Stop reviewing, Escape and × end the session. While Approve's save is in + flight, Skip and Stop reviewing are `aria-disabled` and Escape and × do nothing, so the + closing count includes that save. When no suggestions are on screen (a model change + cleared them, or Generate was refused) the primary saves the manual field only and + reads "Save and next" ("Save and finish" on the last image). Each image is one gated `suggest` call, so the session holds no model between images; a `model-busy` answer stays on that image with its sentence in `#tag-generation-status` and D116's Retry until the user retries or skips. Ending toasts " of images tagged." and returns focus to Auto-tag selected. - **D91** extends to the bulk dialog: its Suggestions field is the job's and the review's `suggestion_limit`. -- **D117** (copy the concept has no line for; the concept's `bulk-apply` only toasts a - demo sentence): an Add tags field that parses to no names toasts "Enter at least one - tag."; a start refused for no coded reason toasts "Tagging could not start. Try +- **D118** (copy the concept has no line for; the concept's `bulk-apply` only toasts a + demo sentence): an Add tags field that parses to no names reports "Enter at least one + tag." on the field; a start refused for no coded reason toasts "Tagging could not start. Try again." or "Auto-tagging could not start. Try again." (as D55's "The scan could not start. Try again."); and the bulk dialog's model panel is headed "Auto-tag image(s)" where the Tag image dialog's reads "Auto-tag this image". Everything else @@ -1286,12 +1295,18 @@ stays after every start, refusal and review. - The log is "● Tagging log — raw detail, click to show". - The row and log wording is the job's (D1), since neither the concept nor the mockup has a line for it: "Added N tag(s)", "No new tags", and the skip reasons - "Video: manual tags only", "Too large to verify", "File changed", "File not found" - and "Not a supported still image". Log prefixes are `+` tagged, `=` unchanged, `-` - skipped and `!` error. + "Video: manual tags only", "Too large to verify", "File changed", "File not found", + "Not a supported still image" and "The file could not be read now. Run the job again + for it." (the capture store stayed at capacity, for example with Tag dialogs open in + other tabs, after a short backoff of retries). Log prefixes are `+` tagged, `=` + unchanged, `-` skipped and `!` error. + - The progress line is the scan's: "Processing file N of M · about X min left", the + estimate (D56's `_time_left`) shown once two files are done, each file one batch. - Download report saves `prompt-manager-image-tags-.json`. - - The toasts stay the generic ones: "Tagging started." / "Auto-tagging started." - (D85) and " finished.", "<title> was cancelled." or "<title> failed.". + - The start toasts are "Tagging started." / "Auto-tagging started." (D85); the end + toasts follow the design's pattern with the job's counts, as the scan's: "<title> + finished · N tagged · M errors", "<title> cancelled · N tagged" or "<title> failed · + N tagged · M errors". - Tests: `tests/js/workspace-activity-store.test.js` ("image-tags jobs get their own tiles outcomes and pill verb", "unknown kinds keep the generic view"); `tests/browser/workspace-activity.spec.mjs` ("an image tagging job shows Tagged @@ -1308,7 +1323,9 @@ selected with Apply automatically runs a job with the chosen model, thresholds a suggestion count", "a model that is not downloaded keeps Start disabled and says why", "Suggestions field prefills from the setting and caps chips at the entered count", "Review each image walks the selection one dialog at a time and only approved -images gain tags" and "a running scan makes the start report busy". Units: +images gain tags", "Review holds Stop reviewing, Escape and × while a save is in +flight, and names a manual-only save" and "a running scan makes the start report +busy". Units: `tests/js/workspace-images-tagging-bulk.test.js`, `tests/js/workspace-images-tagging-review.test.js`. @@ -1318,7 +1335,7 @@ images gain tags" and "a running scan makes the start report busy". Units: | Auto-tag selected and its dialog | D85 (ruling L1) | | How to apply and the Review each image session | D90 (ruling T3-bulk) | | Suggestions count in the bulk dialog | D91 | -| Copy without a concept line: no tags, start fallbacks, the panel heading | D117 | +| Copy without a concept line: no tags, start fallbacks, the panel heading | D118 | | Activity item and job view for image tagging jobs | D89 (plan D66) | Native image tagging (#249 PR B routes, PR C UI): `POST diff --git a/py/api/admin.py b/py/api/admin.py index bc900ee4..f4a6af90 100644 --- a/py/api/admin.py +++ b/py/api/admin.py @@ -198,13 +198,22 @@ def _plural(count, word): return f"{count} {word}" if count == 1 else f"{count} {word}s" +def _time_left(done, total, batches, elapsed): + """ " · about X min left" once enough batches ran, else "" (D56). + + Shared by every Activity job that reports "Processing file N of M": the + scan counts its batches, the image tagging job each file. + """ + if batches < MIN_BATCHES_FOR_ESTIMATE or done <= 0 or done >= total: + return "" + minutes = max(1, math.ceil(elapsed / done * (total - done) / 60)) + return f" · about {minutes} min left" + + def _scan_status_line(done, total, batches, elapsed): """ "Processing file N of M", with an estimate once enough batches ran.""" line = f"Processing file {done:,} of {total:,}" - if batches < MIN_BATCHES_FOR_ESTIMATE or done <= 0 or done >= total: - return line - minutes = max(1, math.ceil(elapsed / done * (total - done) / 60)) - return f"{line} · about {minutes} min left" + return line + _time_left(done, total, batches, elapsed) def _file_size(path): diff --git a/py/api/image_tag_jobs.py b/py/api/image_tag_jobs.py index 24951165..38fa9b6e 100644 --- a/py/api/image_tag_jobs.py +++ b/py/api/image_tag_jobs.py @@ -20,14 +20,16 @@ keeping what it already wrote. Rows and log lines carry root-relative paths and public root labels only. """ +import asyncio import functools import os -from dataclasses import dataclass +import time +from dataclasses import dataclass, field from aiohttp import web from .. import model_gate -from .admin import _plural +from .admin import _plural, _time_left from .annotation_suggest import ( OptionError, unavailable_message, @@ -35,6 +37,8 @@ from .annotation_suggest import ( ) from .annotations import ( _MESSAGES, + MAX_TARGET_PATH, + _coded, _error, _pinned_suggestions, _settled, @@ -71,9 +75,14 @@ except ImportError: from utils.video_metadata import is_video_path MAX_JOB_TARGETS = 500 -# 500 targets of up to ~1.1 KiB each plus the options: the annotation routes' -# 64 KiB bound is for one target, too small for a full selection. -MAX_JOB_BODY_BYTES = 1024 * 1024 +# 500 maximal targets plus the options: a path of MAX_TARGET_PATH code points +# takes at most 6 bytes each on the wire (a \uXXXX escape; raw UTF-8 is at most +# 4), and the root index and scope fit in 256. The annotation routes' 64 KiB +# bound is for one target, too small for a full selection. +MAX_JOB_BODY_BYTES = MAX_JOB_TARGETS * (MAX_TARGET_PATH * 6 + 256) + 64 * 1024 +# A busy capture worker (a Tag dialog in another tab) is waited for this long, +# try by try, before the file is left for a rerun. +CAPACITY_BACKOFF = (0.1, 0.2, 0.4, 0.8) ADD_TITLE = "Add tags to images" AUTO_TITLE = "Auto-tag images" @@ -97,12 +106,32 @@ SAVE_FAILED = "The tags could not be saved. Check server logs for details." READ_FAILED = "The file could not be read." SOURCE_CHANGED = "The source changed. Refresh the image collection." STORAGE_RESTORED = "Prompt storage was restored; start the job again." +BUSY_RETRY = "The file could not be read now. Run the job again for it." + +_monotonic = time.monotonic # the job's clock (a test can fake it) class _JobStopped(Exception): """No later file can be written (prompt storage was restored).""" +@dataclass +class _Run: + """One running job: its request, the gate owner and the resolved roots. + + ``roots`` maps each target's ``(root_index, root_scope)`` to ``(root, + label)``, read once per job off the event loop; ``root`` is None when the + scope no longer names a current root. + """ + + job: object + spec: object + epoch: str + owner: object + service: object = None + roots: dict = field(default_factory=dict) + + @dataclass(frozen=True) class _Spec: """A validated start request.""" @@ -197,10 +226,26 @@ def _capture_outcome(exc: CaptureError, root, relative: str): path = os.path.join(root, *relative.split("/")) return "skipped", FILE_CHANGED if os.path.lexists(path) else FILE_NOT_FOUND if isinstance(exc, CaptureCapacity): - return "errors", _MESSAGES["review-capacity"] + return "skipped", BUSY_RETRY return "errors", READ_FAILED +async def _capacity_retried(job, attempt): + """``await attempt()``, tried again while the capture store is at capacity. + + Raises the last ``CaptureCapacity`` once ``CAPACITY_BACKOFF`` is spent or the + job was cancelled meanwhile. + """ + for delay in CAPACITY_BACKOFF: + try: + return await attempt() + except CaptureCapacity: + if job.cancelled: + raise + await asyncio.sleep(delay) + return await attempt() + + def _load_model(service, model_type: str, use_gpu: bool) -> None: """Worker thread: load (or swap to) *model_type*; never downloads.""" if service.is_model_loaded() and service.get_loaded_model_type() == model_type: @@ -225,10 +270,9 @@ class ImageTagJobsMixin: spec, error = parse_image_tag_request(data) if error is not None: return error - for index, scope in dict.fromkeys((t[0], t[1]) for t in spec.targets): - _root, error = self._scoped_root(index, scope) - if error is not None: - return error + roots = await self._run_in_executor(self._job_roots, spec.targets) + if any(root is None for root, _label in roots.values()): + return _coded("source-changed") epoch = await self._run_in_executor(self.db.get_execution_epoch) # A running job in the way answers first: it says which job to wait for. blocker = self._jobs.blocker(IMAGE_TAG_JOB_KIND, record_changing=True) @@ -259,6 +303,18 @@ class ImageTagJobsMixin: # -- runner ---------------------------------------------------------------- + def _job_roots(self, targets) -> dict: + """Worker thread: ``{(index, scope): (root or None, label)}`` (see _Run).""" + roots = list(self._get_all_output_dirs()) + resolved = {} + for index, scope in dict.fromkeys((t[0], t[1]) for t in targets): + label = _public_path(roots[index]) if index < len(roots) else "" + error = self._gallery_scope_error( + {"root_scope": scope, "root": str(index)}, roots + ) + resolved[(index, scope)] = (roots[index] if error is None else None, label) + return resolved + async def _run_image_tag_job(self, spec: _Spec, epoch: str, owner, job) -> None: verb = _VERBS[spec.mode] total = len(spec.targets) @@ -267,14 +323,14 @@ class ImageTagJobsMixin: if spec.mode == "auto": started += f" · {spec.model_type}" job.log_line(started) + run = _Run(job, spec, epoch, owner) try: - service = None if spec.mode == "auto": - service = await self._prepare_tagging_model(job, spec, owner) - if service is None: + run.service = await self._prepare_tagging_model(job, spec, owner) + if run.service is None: job.finish("failed") return - if not await self._tag_each_file(job, spec, epoch, owner, service): + if not await self._tag_each_file(run): job.log_line(f"{verb} stopped · {STORAGE_RESTORED}") job.finish("failed") return @@ -286,25 +342,27 @@ class ImageTagJobsMixin: if owner is not None: model_gate.release(owner) - async def _tag_each_file(self, job, spec, epoch, owner, service) -> bool: + async def _tag_each_file(self, run: _Run) -> bool: """Tag the targets in order; False when the job had to stop.""" - total = len(spec.targets) - for number, target in enumerate(spec.targets, 1): + job, total = run.job, len(run.spec.targets) + run.roots = await self._run_in_executor(self._job_roots, run.spec.targets) + started = _monotonic() + for number, target in enumerate(run.spec.targets, 1): if job.cancelled: break - job.update(status_line=f"Processing file {number:,} of {total:,}") + # The scan's estimate (D56), with each finished file as one batch. + left = _time_left(number - 1, total, number - 1, _monotonic() - started) + job.update(status_line=f"Processing file {number:,} of {total:,}{left}") job.publish({"type": "progress", "progress": job.progress}) stopped = False try: - group, detail = await self._tag_one_file( - spec, epoch, owner, service, target - ) + group, detail = await self._tag_one_file(run, target) except _JobStopped: stopped, group, detail = True, "errors", STORAGE_RESTORED except Exception: self.logger.exception("Image tagging failed for one file") group, detail = "errors", SAVE_FAILED - self._record_tag_outcome(job, target, group, detail) + self._record_tag_outcome(run, target, group, detail) job.update(processed=number, progress=number * 100 // total) if stopped: return False @@ -337,57 +395,63 @@ class ImageTagJobsMixin: return None return service - async def _tag_one_file(self, spec, epoch, owner, service, target): + async def _verified(self, run: _Run, store, root, relative): + """The current file's facts, waiting out a busy capture store.""" + return await _capacity_retried( + run.job, lambda: self._run_in_executor(store.verify, root, relative) + ) + + async def _tag_one_file(self, run: _Run, target): """The (group, detail) of one target; raises _JobStopped on a restore.""" index, scope, relative = target - root, error = self._scoped_root(index, scope) - if error is not None: + root, _label = run.roots[(index, scope)] + if root is None: return "errors", SOURCE_CHANGED store = self._capture_store() - if spec.mode == "add": + if run.spec.mode == "add": try: - facts = await self._run_in_executor(store.verify, root, relative) + facts = await self._verified(run, store, root, relative) except CaptureError as exc: return _capture_outcome(exc, root, relative) - return await self._union_image_tags(facts, list(spec.tags), epoch) + return await self._union_image_tags(facts, list(run.spec.tags), run.epoch) if is_video_path(relative): return "skipped", VIDEO_SKIPPED try: - capture = await _settled( - _submit( - store.capture, - root, - relative, - epoch=epoch, - scope=scope, - root_index=index, + capture = await _capacity_retried( + run.job, + lambda: _settled( + _submit( + store.capture, + root, + relative, + epoch=run.epoch, + scope=scope, + root_index=index, + ), + lambda made: made is not None and store.release(made.token), ), - lambda made: made is not None and store.release(made.token), ) except CaptureError as exc: return _capture_outcome(exc, root, relative) try: - return await self._auto_tag_capture( - store, capture, spec, epoch, owner, service, root, relative - ) + return await self._auto_tag_capture(run, store, capture, root, relative) finally: store.release(capture.token) - async def _auto_tag_capture( - self, store, capture, spec, epoch, owner, service, root, relative - ): + async def _auto_tag_capture(self, run: _Run, store, capture, root, relative): """Suggest on the capture, then union only onto the same current bytes.""" if not capture.inferable: return "skipped", NOT_A_STILL if store.acquire(capture.token) is None: raise _JobStopped # every capture was invalidated (restore) + spec = run.spec try: pairs = await model_gate.run( - owner, + run.owner, _pinned_suggestions, store, capture.token, - service, + run.service, capture, spec.model_type, spec.options, @@ -397,7 +461,7 @@ class ImageTagJobsMixin: self.logger.exception("Auto-tagging failed for one file") return "errors", INFERENCE_FAILED try: - facts = await self._run_in_executor(store.verify, root, relative) + facts = await self._verified(run, store, root, relative) except CaptureError as exc: return _capture_outcome(exc, root, relative) if facts.locator != capture.locator or facts.sha256 != capture.sha256: @@ -405,7 +469,7 @@ class ImageTagJobsMixin: names = [str(name) for name, _score in pairs] if not names: return "unchanged", NO_NEW_TAGS - return await self._union_image_tags(facts, names, epoch) + return await self._union_image_tags(facts, names, run.epoch) async def _union_image_tags(self, facts, tags: list, epoch: str): """``add_image_tags`` on the verified bytes, as a (group, detail).""" @@ -434,12 +498,11 @@ class ImageTagJobsMixin: return "tagged", f"Added {_plural(added, 'tag')}" return "unchanged", NO_NEW_TAGS - def _record_tag_outcome(self, job, target, group: str, detail: str) -> None: - index, _scope, relative = target - roots = list(self._get_all_output_dirs()) - label = _public_path(roots[index]) if index < len(roots) else "" + def _record_tag_outcome(self, run: _Run, target, group: str, detail: str) -> None: + index, scope, relative = target + _root, label = run.roots[(index, scope)] where = f"{label}/{relative}" if label else relative - job.record( + run.job.record( group, {"path": relative, "root": label, "root_index": index, "detail": detail}, f"{_LOG_PREFIXES[group]} {where} · {detail}", diff --git a/tests/browser/workspace-activity.spec.mjs b/tests/browser/workspace-activity.spec.mjs index b45b3c7f..80f9fcd3 100644 --- a/tests/browser/workspace-activity.spec.mjs +++ b/tests/browser/workspace-activity.spec.mjs @@ -861,11 +861,11 @@ test('an image tagging job shows Tagged Unchanged Skipped Errors and opens tagge // An error, then the end: the header and the Activity item read "N tagged · M errors". await step(request, job.id, { processed: 4, records: [ - { group: 'errors', path: 'broken/truncated.png', root: 'output', detail: 'The file could not be tagged.' }, + { group: 'errors', path: 'broken/truncated.png', root: 'output', detail: 'Auto-tagging failed. Check server logs for details.' }, ] }); await expect(tiles(page).last().locator('dd')).toHaveText('1'); await finish(request, job.id); - await expect(toast(page)).toHaveText('Auto-tag images finished.'); + await expect(toast(page)).toHaveText('Auto-tag images finished · 1 tagged · 1 error'); await expect(heading(page).locator('p.muted')).toHaveText(/^Finished \d\d:\d\d · 1 tagged · 1 error$/); await expect(heading(page).locator('.heading-actions > button')).toHaveText(['Download report']); await expect(progressSection(page).locator('.panel-heading > span.small')).toHaveText('4 of 4 files processed'); @@ -883,7 +883,7 @@ test('cancel tagging stops at a file boundary', async ({ page, request }) => { await openJob(page, job.id); await heading(page).getByRole('button', { name: 'Cancel tagging', exact: true }).focus(); await page.keyboard.press('Enter'); - await expect(toast(page)).toHaveText('Add tags to images was cancelled.'); + await expect(toast(page)).toHaveText('Add tags to images cancelled · 2 tagged'); await expect(heading(page).locator('h1')).toBeFocused(); await expect(heading(page).locator('p.muted')).toHaveText(/^Cancelled \d\d:\d\d at a file boundary · 2 tagged · completed work is kept$/); await expect(heading(page).getByRole('button', { name: 'Cancel tagging', exact: true })).toHaveCount(0); diff --git a/tests/browser/workspace-tagging-bulk.spec.mjs b/tests/browser/workspace-tagging-bulk.spec.mjs index 3cb0fb2e..d3561059 100644 --- a/tests/browser/workspace-tagging-bulk.spec.mjs +++ b/tests/browser/workspace-tagging-bulk.spec.mjs @@ -84,7 +84,16 @@ test('Add tags runs a job and the cards show the tags after it finishes', async const field = modal(page).getByRole('textbox', { name: 'Tags, separated by commas', exact: true }); await expect(field).toHaveValue(''); await expect(actionButtons(page)).toHaveText(['Cancel', 'Apply']); + let posted = 0; + page.on('request', item => { if (new URL(item.url()).pathname === '/prompt_manager/jobs/image-tags') posted += 1; }); + // A field that parses to no names keeps the dialog and its text, and reports why on the field (D118). + await field.fill(' , ,'); + await actionButtons(page).last().click(); + await expect.poll(() => field.evaluate(node => node.validationMessage)).toBe('Enter at least one tag.'); + await expect(title(page)).toHaveText('Tag selected images'); + await expect(field).toHaveValue(' , ,'); await field.fill('sunset, Sunset , blue_hour,,'); + expect(await field.evaluate(node => node.validity.valid)).toBe(true); const started = jobStart(page); const answer = page.waitForResponse(item => new URL(item.url()).pathname === '/prompt_manager/jobs/image-tags'); await actionButtons(page).last().click(); @@ -93,7 +102,10 @@ test('Add tags runs a job and the cards show the tags after it finishes', async expect((await started).postDataJSON()).toEqual({ mode: 'add', tags: ['sunset', 'blue_hour'], targets: [ { root_index: 0, root_scope: scope, path: FIRST }, { root_index: 0, root_scope: scope, path: SECOND }] }); const { job } = await (await answer).json(); + // The refused Apply sent nothing: only the valid one was posted. + expect(posted).toBe(1); await expect.poll(() => toasts(page)).toContain('Tagging started.'); + expect(await toasts(page)).not.toContain('Enter at least one tag.'); await expect(modal(page)).toHaveCount(0); await expect(bar(page).locator('strong')).toHaveText('2 selected'); expect((await ended(request, job.id)).counters).toEqual({ tagged: 2, unchanged: 0, skipped: 0, errors: 0 }); @@ -129,7 +141,6 @@ test('Auto-tag selected with Apply automatically runs a job with the chosen mode await actionButtons(page).last().click(); expect(await limit.evaluate(node => node.validity.valid)).toBe(false); await expect(title(page)).toHaveText('Auto-tag selected images'); - expect(posted).toBe(0); await limit.fill('2'); const started = jobStart(page); const answer = page.waitForResponse(item => new URL(item.url()).pathname === '/prompt_manager/jobs/image-tags'); @@ -139,6 +150,8 @@ test('Auto-tag selected with Apply automatically runs a job with the chosen mode options: { general_threshold: 0.5, character_threshold: 0.9, suggestion_limit: 2 }, targets: [{ root_index: 0, root_scope: scope, path: FIRST }, { root_index: 0, root_scope: scope, path: UNLINKED }] }); const { job } = await (await answer).json(); + // The invalid press sent nothing: only this Start was posted. + expect(posted).toBe(1); await expect.poll(() => toasts(page)).toContain('Auto-tagging started.'); await expect(modal(page)).toHaveCount(0); await expect(bar(page).locator('strong')).toHaveText('2 selected'); @@ -226,7 +239,8 @@ test('Review each image walks the selection one dialog at a time and only approv await actionButtons(page).last().click(); await expect(title(page)).toHaveText('Tag image · Image 2 of 3'); await expect(modal(page).locator('#tag-generation-status')).toHaveText('The tagging model is busy. Try again when it finishes.'); - await expect(actionButtons(page)).toHaveText(['Retry', 'Stop reviewing', 'Skip', 'Approve and next']); + // No suggestions on screen: the primary would save the manual field only (D90). + await expect(actionButtons(page)).toHaveText(['Retry', 'Stop reviewing', 'Skip', 'Save and next']); await actionButtons(page).nth(2).click(); await expect(title(page)).toHaveText('Tag image · Image 3 of 3'); await expect(chips(page)).toHaveCount(2); @@ -245,6 +259,47 @@ test('Review each image walks the selection one dialog at a time and only approv await expect.poll(async () => (await (await request.get('/__test__/tagging/captures')).json()).captures).toBe(0); }); +test('Review holds Stop reviewing, Escape and × while a save is in flight, and names a manual-only save', async ({ page, request }) => { + await select(page, FIRST, SECOND); + let release; + const held = new Promise(resolve => { release = resolve; }); + await page.route('**/prompt_manager/image-annotations/tags', async route => { + if (route.request().method() === 'PUT') await held; + await route.continue(); + }); + await barButton(page, 'Auto-tag selected').click(); + await modal(page).getByRole('radio', { name: 'Review each image', exact: true }).check(); + await actionButtons(page).last().click(); + await expect(title(page)).toHaveText('Tag image · Image 1 of 2'); + await expect(chips(page)).not.toHaveCount(0); + await expect(actionButtons(page)).toHaveText(['Stop reviewing', 'Skip', 'Approve and next']); + // D90: a model change clears the suggestions, so the primary saves the manual field only and says so. + await modal(page).getByRole('combobox', { name: 'Tagging model', exact: true }).selectOption('wd14-vit'); + await expect(chips(page)).toHaveCount(0); + await expect(actionButtons(page)).toHaveText(['Stop reviewing', 'Skip', 'Save and next']); + await modal(page).getByRole('textbox', { name: 'Image tags · edit manually', exact: true }).fill('mine'); + const put = page.waitForRequest(item => item.method() === 'PUT' && item.url().endsWith('/image-annotations/tags')); + await actionButtons(page).last().click(); + await put; + const stop = actionButtons(page).first(); + await expect(stop).toHaveAttribute('aria-disabled', 'true'); + await expect(actionButtons(page).nth(1)).toHaveAttribute('aria-disabled', 'true'); + // None of these ends the session while the save is in flight; the count then includes it. + await page.keyboard.press('Escape'); + await modal(page).locator('#modal-close').click(); + await stop.dispatchEvent('click'); // aria-disabled: Playwright's click would wait for it + release(); + await expect(title(page)).toHaveText('Tag image · Image 2 of 2'); + await expect(chips(page)).not.toHaveCount(0); + await expect(actionButtons(page)).toHaveText(['Stop reviewing', 'Skip', 'Approve and finish']); + await expect(actionButtons(page).first()).not.toHaveAttribute('aria-disabled', 'true'); + await actionButtons(page).first().click(); + await expect(modal(page)).toHaveCount(0); + await expect(toast(page)).toHaveText('1 of 2 images tagged.'); + expect(await savedTags(request, FIRST)).toEqual(['mine']); + expect(await savedTags(request, SECOND)).toEqual([]); +}); + test('a running scan makes the start report busy', async ({ page, request }) => { const scan = await (await request.post('/__test__/jobs/start', { data: { kind: 'scan', title: 'Scan for metadata' } })).json(); try { diff --git a/tests/js/workspace-activity-store.test.js b/tests/js/workspace-activity-store.test.js index eea5846e..a562fec8 100644 --- a/tests/js/workspace-activity-store.test.js +++ b/tests/js/workspace-activity-store.test.js @@ -300,6 +300,13 @@ test('image-tags jobs get their own tiles outcomes and pill verb', async () => { assert.equal(outcomeLine(job), '1,420 tagged · 1 error'); assert.equal(outcomeLine({ ...job, counters: {} }), '0 tagged · 0 errors'); assert.equal(startToast(job), 'Auto-tagging started.'); + // The design's toast pattern, as the scan's: "<Job> finished · N tagged · M errors". + const { finishToast } = await source; + assert.equal(finishToast({ ...job, state: 'finished' }), 'Auto-tag images finished · 1,420 tagged · 1 error'); + assert.equal(finishToast({ ...job, state: 'cancelled' }), 'Auto-tag images cancelled · 1,420 tagged'); + assert.equal(finishToast({ ...job, state: 'failed' }), 'Auto-tag images failed · 1,420 tagged · 1 error'); + assert.equal(finishToast({ ...job, title: 'Add tags to images', counters: {}, state: 'finished' }), + 'Add tags to images finished · 0 tagged · 0 errors'); }); test('unknown kinds keep the generic view', async () => { diff --git a/tests/js/workspace-images-tagging-bulk.test.js b/tests/js/workspace-images-tagging-bulk.test.js index 55abf664..e5647dbf 100644 --- a/tests/js/workspace-images-tagging-bulk.test.js +++ b/tests/js/workspace-images-tagging-bulk.test.js @@ -49,6 +49,24 @@ test('add parses comma tags with the same dedupe as the dialog', () => { assert.equal(parseBulkTags, state.parseManualTags); }); +test('Add tags keeps its dialog and the typed text when the field parses to no names', async () => { + const { openAddTags, NO_TAGS } = module(); + const toasts = []; + const posted = []; + const store = { start: async (kind, body) => { posted.push(body); return { job: { id: JOB_ID, state: 'running' } }; }, + find: () => null, subscribe: () => () => {} }; + let dialog = null; + openAddTags({ scope: pageScope(), store, images: [image('a.png')], trigger: null, notify: text => toasts.push(text), + openInput: args => { dialog = args; } }); + assert.equal(typeof dialog?.validate, 'function'); + // The dialog reports the message on its field and stays open: nothing toasts, nothing is sent. + assert.equal(dialog.validate(' , ,'), NO_TAGS); + assert.equal(dialog.validate('sunset'), null); + assert.deepEqual(toasts, []); + await dialog.onApply('sunset, Sunset'); + assert.deepEqual(posted.map(body => body.tags), [['sunset']]); +}); + test('busy start keeps the selection and names the running job', async () => { const { startTagJob } = module(); const toasts = []; diff --git a/tests/test_image_tag_jobs.py b/tests/test_image_tag_jobs.py index 089cfab2..3332422d 100644 --- a/tests/test_image_tag_jobs.py +++ b/tests/test_image_tag_jobs.py @@ -32,6 +32,7 @@ from database.execution_history_schema import rotate_history_epoch from py import model_gate from py.api import jobs as jobs_module from py.config import AutotagConfig +from utils.media_capture import CaptureCapacity START = "/prompt_manager/jobs/image-tags" WAIT = 30.0 @@ -644,5 +645,143 @@ class TestStartAndExclusion(ImageTagJobTestCase): self.assertTrue(any("output/sub/a.png" in line for line in detail["log"])) +class TestFixRound(ImageTagJobTestCase): + """The final review's fixes: estimate, body bound, busy captures, roots.""" + + async def test_status_line_estimates_minutes_left_after_the_minimum(self): + for index in range(3): + self.png(f"f{index}.png", color=(index, 7, 7)) + clock = iter([0.0, 0.0, 120.0, 240.0]) + lines = [] + self.tagger.on_generate = lambda _path: lines.append( + self.api._jobs.list()[0].status_line + ) + + with patch( + "py.api.image_tag_jobs._monotonic", side_effect=lambda: next(clock, 240.0) + ): + await self.run_job(self.auto_body([f"f{i}.png" for i in range(3)])) + + # Two files make the minimum (the scan's MIN_BATCHES_FOR_ESTIMATE); then + # 120 s per file leaves about 2 min for the third. + self.assertEqual( + lines, + [ + "Processing file 1 of 3", + "Processing file 2 of 3", + "Processing file 3 of 3 · about 2 min left", + ], + ) + + def maximal_target(self, number): + """A distinct target whose path is 1024 non-ASCII code points long.""" + head = f"{number:03d}/" + ("界" * 200 + "/") * 5 + path = head + "界" * (1024 - len(head) - 4) + ".png" + self.assertEqual(len(path), 1024) + return self.target(path) + + async def test_start_body_bounds(self): + from py.api import image_tag_jobs + + body = { + "mode": "add", + "targets": [self.maximal_target(n) for n in range(500)], + "tags": ["x"], + } + # Escaped as \uXXXX (6 bytes a code point), far over the old 1 MiB. + payload = json.dumps(body).encode("utf-8") + self.assertGreater(len(payload), 3 * 1000 * 1000) + response = await self.client.post( + START, data=payload, headers={"Content-Type": "application/json"} + ) + data = await self.json_of(response) + self.assertEqual(response.status, 202, data) + self.assertEqual(data["job"]["scope"]["images"], 500) + job = self.api._jobs.get(data["job"]["id"]) + await self.client.post(f"/prompt_manager/jobs/{job.id}/cancel") + await asyncio.wait_for(asyncio.shield(job.task), WAIT) + + cap = image_tag_jobs.MAX_JOB_BODY_BYTES + self.assertGreaterEqual(cap, len(payload)) + over = b'{"mode":"add","targets":[],"tags":["x"]' + over += b" " * (cap + 1 - len(over) - 1) + b"}" + self.assertEqual(len(over), cap + 1) + response = await self.client.post( + START, data=over, headers={"Content-Type": "application/json"} + ) + self.assertEqual(response.status, 413) + + def saturate(self, store): + with store._lock: + store._workers += store.max_workers + self.addCleanup(self.unsaturate, store) + + def unsaturate(self, store): + with store._lock: + if store._workers >= store.max_workers: + store._workers -= store.max_workers + + async def test_busy_capture_workers_are_retried_then_left_for_a_rerun(self): + self.png("a.png") + self.png("b.png", color=(1, 2, 3)) + store = self.api._capture_store() + self.saturate(store) + + with patch("py.api.image_tag_jobs.CAPACITY_BACKOFF", (0.001, 0.001)): + added = await self.run_job(self.add_body(["a.png"], ["x"])) + auto = await self.run_job(self.auto_body(["b.png"])) + + retry = "The file could not be read now. Run the job again for it." + for job, path in ((added, "a.png"), (auto, "b.png")): + self.assertEqual(job.state, "finished") + self.assertEqual(self.rows(job, "skipped"), [(path, retry)]) + self.assertEqual(job.counters["errors"], 0) + self.assertNotIn("Tag dialog", json.dumps(job.detail())) + self.assertEqual(self.tagger.calls, []) + self.assertEqual(self.stored("a.png")["tags"], []) + + async def test_a_busy_capture_worker_that_frees_up_is_tagged(self): + self.png("a.png") + store = self.api._capture_store() + real = store.verify + tries = [] + + def verify(root, relative): + tries.append(relative) + if len(tries) < 3: + raise CaptureCapacity("Too many captures are in progress") + return real(root, relative) + + with ( + patch.object(store, "verify", side_effect=verify), + patch("py.api.image_tag_jobs.CAPACITY_BACKOFF", (0.001, 0.001, 0.001)), + ): + job = await self.run_job(self.add_body(["a.png"], ["x"])) + + self.assertEqual(len(tries), 3) + self.assertEqual(self.rows(job, "tagged"), [("a.png", "Added 1 tag")]) + self.assertEqual(self.stored("a.png")["tags"], ["x"]) + + async def test_output_roots_are_read_once_per_job_off_the_event_loop(self): + for index in range(3): + self.png(f"f{index}.png", color=(index, 1, 1)) + body = self.add_body([f"f{i}.png" for i in range(3)], ["x"]) + original = self.api._get_all_output_dirs + threads = [] + + def lookup(): + threads.append(threading.current_thread()) + return original() + + with patch.object(self.api, "_get_all_output_dirs", side_effect=lookup): + job = await self.run_job(body) + + self.assertEqual(job.counters["tagged"], 3) + # One read when the start checks the scopes, one for the whole run. + self.assertEqual(len(threads), 2) + self.assertNotIn(threading.main_thread(), threads) + self.assertEqual(job.outcomes["tagged"][0]["root"], "output") + + if __name__ == "__main__": unittest.main() diff --git a/tools/vendor/frontend-coverage-baseline.json b/tools/vendor/frontend-coverage-baseline.json index fc8a2d9e..31ab8b3e 100644 --- a/tools/vendor/frontend-coverage-baseline.json +++ b/tools/vendor/frontend-coverage-baseline.json @@ -169,7 +169,7 @@ }, "workspace/activity-store.mjs": { "lines": 98, - "branches": 95, + "branches": 96, "functions": 98, "statements": 98 }, @@ -230,7 +230,7 @@ "workspace/image-tagging.mjs": { "lines": 97, "branches": 90, - "functions": 87, + "functions": 88, "statements": 97 }, "workspace/images-category.mjs": { @@ -265,8 +265,8 @@ }, "workspace/images-tagging-bulk.mjs": { "lines": 98, - "branches": 91, - "functions": 85, + "branches": 93, + "functions": 86, "statements": 98 }, "workspace/images-tagging-review.mjs": { diff --git a/web/js/workspace/activity-store.mjs b/web/js/workspace/activity-store.mjs index 06be04dd..3df6405b 100644 --- a/web/js/workspace/activity-store.mjs +++ b/web/js/workspace/activity-store.mjs @@ -112,13 +112,20 @@ const errorsOf = job => `${number(job.counters.errors ?? 0)} ${noun(job.counters /** "1,420 added". */ export const addedOf = job => `${number(job.counters.added ?? 0)} added`; -/** The toast when a job ends: "Scan finished · N added · M errors", "Scan cancelled · N added". */ +/** + * The toast when a job ends: "Scan finished · N added · M errors", "Scan cancelled · N added"; an image tagging job + * the same way with its own title and count (D89): "Auto-tag images finished · N tagged · M errors". + */ export function finishToast(job) { if (job.kind === 'scan') { if (job.state === 'cancelled') return `Scan cancelled · ${addedOf(job)}`; if (job.state === 'failed') return `Scan failed · ${addedOf(job)} · ${errorsOf(job)}`; return `Scan finished · ${addedOf(job)} · ${errorsOf(job)}`; } + if (job.kind === 'image-tags') { + if (job.state === 'cancelled') return `${job.title} cancelled · ${taggedOf(job)}`; + return `${job.title} ${job.state === 'failed' ? 'failed' : 'finished'} · ${outcomeLine(job)}`; + } if (job.state === 'cancelled') return `${job.title} was cancelled.`; if (job.state === 'failed') return `${job.title} failed.`; return `${job.title} finished.`; diff --git a/web/js/workspace/image-tagging.mjs b/web/js/workspace/image-tagging.mjs index 534d139b..52d31c20 100644 --- a/web/js/workspace/image-tagging.mjs +++ b/web/js/workspace/image-tagging.mjs @@ -275,7 +275,9 @@ export function openImageTagging({api, scope, image, source = 'images', trigger const skip = createButton('Skip'); const primary = createButton('Save manual tags', 'ca-primary'); const last = review && position.index === position.total; - const approveLabel = last ? 'Approve and finish' : 'Approve and next'; + // D90: Approve with suggestions on screen; after a model change cleared them (M2) Save keeps only the manual field. + const reviewLabel = state => (state.generated ? (last ? 'Approve and finish' : 'Approve and next') + : (last ? 'Save and finish' : 'Save and next')); if (review) actions.append(cancel, skip, primary); else actions.append(cancel, primary); @@ -343,9 +345,10 @@ export function openImageTagging({api, scope, image, source = 'images', trigger if (state.suggestions !== shownRows || state.suggestionsText !== shownText) renderSuggestions(state); setText(count, state.countText); setText(status, state.statusText); - setText(primary, review ? approveLabel : state.primaryLabel); + setText(primary, review ? reviewLabel(state) : state.primaryLabel); setDisabled(primary, !state.canSave); - if (review) setDisabled(skip, state.phase === 'saving'); + // D90: while Approve's save is in flight the session cannot be left, so its count includes that save. + if (review) for (const button of [skip, cancel]) setDisabled(button, state.phase === 'saving'); if (state.canRetry !== retry.isConnected) { if (state.canRetry) actions.prepend(retry); // D116 else { @@ -356,7 +359,7 @@ export function openImageTagging({api, scope, image, source = 'images', trigger if (state.phase === 'saved') complete(state); } - cancel.addEventListener('click', () => closeWorkspaceDialog()); + cancel.addEventListener('click', () => { if (!disabled(cancel)) closeWorkspaceDialog(); }); skip.addEventListener('click', () => { if (!open || advancing || disabled(skip)) return; advancing = true; @@ -382,7 +385,8 @@ export function openImageTagging({api, scope, image, source = 'images', trigger if (review && !advancing) onStop?.(); } const dialogTitle = review ? `Tag image · Image ${position.index} of ${position.total}` : 'Tag image'; // D90 - openContentDialog({scope, title: dialogTitle, nodes, trigger, onClose: release}); + openContentDialog({scope, title: dialogTitle, nodes, trigger, onClose: release, + canClose: review ? () => controller.state.phase !== 'saving' : null}); document.getElementById('modal')?.classList.add('tagging-modal'); local.cleanup(controller.subscribe(render)); render(controller.state); diff --git a/web/js/workspace/images-tagging-bulk.mjs b/web/js/workspace/images-tagging-bulk.mjs index 4d197998..b4788bec 100644 --- a/web/js/workspace/images-tagging-bulk.mjs +++ b/web/js/workspace/images-tagging-bulk.mjs @@ -26,14 +26,14 @@ import { showWorkspaceToast } from './toast.mjs'; export const IMAGE_TAG_JOB = 'image-tags'; export const MAX_BULK_IMAGES = 500; export const TOO_MANY = 'Select at most 500 images.'; -export const NO_TAGS = 'Enter at least one tag.'; // D117 +export const NO_TAGS = 'Enter at least one tag.'; // D118 export const ADD_DIALOG = Object.freeze({title: 'Tag selected images', label: 'Tags, separated by commas', action: 'Apply'}); export const AUTO_TITLE = 'Auto-tag selected images'; export const APPLY_CHOICES = Object.freeze([ Object.freeze({value: 'auto', label: 'Apply automatically', action: 'Start auto-tagging'}), Object.freeze({value: 'review', label: 'Review each image', action: 'Start review'}), ]); -// D117: the concept has no copy for these; they follow D55's "The scan could not start. Try again.". +// D118: the concept has no copy for these; they follow D55's "The scan could not start. Try again.". const START_FAILED = Object.freeze({add: 'Tagging could not start. Try again.', auto: 'Auto-tagging could not start. Try again.'}); const FINAL = new Set(['finished', 'cancelled', 'failed']); @@ -102,13 +102,17 @@ function followTagged({store, scope, job, images, onTagged}) { followJobEnd({store, scope, id: job.id, onEnd: ended => { if ((ended.counters.tagged ?? 0) > 0) onTagged(keys); }}); } -/** Add tags: the concept's simpleInput, then the job over `images` (the checked files, taken now). */ -export function openAddTags({scope, store, images, trigger, onTagged = () => {}, notify = showWorkspaceToast}) { +/** + * Add tags: the concept's simpleInput, then the job over `images` (the checked files, taken now). A field that parses + * to no names keeps the dialog and its text, and reports NO_TAGS on the field (D118). `openInput` is the dialog seam. + */ +export function openAddTags({scope, store, images, trigger, onTagged = () => {}, notify = showWorkspaceToast, + openInput = openInputDialog}) { const targets = bulkTargets(images); if (!targets) { notify(TOO_MANY); return; } - openInputDialog({scope, trigger, ...ADD_DIALOG, onApply: async text => { + openInput({scope, trigger, ...ADD_DIALOG, validate: text => (parseBulkTags(text).length ? null : NO_TAGS), + onApply: async text => { const tags = parseBulkTags(text); - if (!tags.length) { notify(NO_TAGS); return; } const job = await startTagJob({store, scope, body: {mode: 'add', targets, tags}, notify}); if (job) followTagged({store, scope, job, images, onTagged}); }}); @@ -145,7 +149,7 @@ export function openAutoTag({api, scope, store, images, trigger, onTagged = () = const options = createTaggingOptionsController({api}); // D85: the Tag image dialog's model panel without Generate; its Suggestions field is the job's count (D91). const {element: panel, validate} = renderModelPanel({scope: local, controller: options, - heading: `Auto-tag ${countOf(listed.length, 'image')}`, generate: false}); // D117 + heading: `Auto-tag ${countOf(listed.length, 'image')}`, generate: false}); // D118 let choice = APPLY_CHOICES[0]; const actions = el('div', undefined, 'dialog-actions'); const cancel = createButton('Cancel'); diff --git a/web/js/workspace/workspace-dialogs.mjs b/web/js/workspace/workspace-dialogs.mjs index 72b3e860..bdfa84f9 100644 --- a/web/js/workspace/workspace-dialogs.mjs +++ b/web/js/workspace/workspace-dialogs.mjs @@ -59,10 +59,10 @@ function createShell() { body.id = 'modal-body'; header.append(title, close); dialog.append(header, body); - close.addEventListener('click', () => finish()); + close.addEventListener('click', () => dismiss()); dialog.addEventListener('cancel', event => { event.preventDefault(); - finish(); + dismiss(); }); // A browser may still close a modal on its own; that is a dismissal too. `close` arrives as a later task, so it // is ignored once this shell was reopened or replaced. @@ -82,6 +82,12 @@ function finish() { if (trigger?.isConnected) trigger.focus(); } +/** × or Escape: close, unless the open dialog's `canClose()` holds it (a save in flight). */ +function dismiss() { + if (session?.canClose && !session.canClose()) return; + finish(); +} + const boundScopes = new WeakSet(); function requireScope(scope) { @@ -104,13 +110,13 @@ function bindScope(scope) { }); } -function show(heading, nodes, { scope, trigger, focusTarget, onClose = null }) { +function show(heading, nodes, { scope, trigger, focusTarget, onClose = null, canClose = null }) { shell ??= createShell(); if (!shell.dialog.isConnected) document.body.append(shell.dialog); bindScope(scope); // A swapped-out body is gone from the shell, so its owner releases it. const replaced = session?.onClose; - session = { scope, trigger: trigger ?? session?.trigger ?? null, onClose }; + session = { scope, trigger: trigger ?? session?.trigger ?? null, onClose, canClose }; replaced?.(); shell.title.textContent = heading; shell.body.replaceChildren(...nodes); @@ -298,10 +304,14 @@ export function openCopyFallback(text, { scope, trigger = null } = {}) { * The concept's `simpleInput`: one labelled `input#action-input` (starting at `value`), then Cancel and the primary * `action`. The action reports the trimmed value and closes; an empty value keeps the dialog open on its field, or * reports `fallback` when one is given (Save view's "Saved view", a1339). A positive `maxLength` limits the field. + * `validate(value)` may refuse a value with a message: the dialog then stays open with the typed text and the field + * reports the message (its native validity) until it is edited. */ -export function openInputDialog({ scope, title, label, action, value = '', maxLength, fallback, onApply, trigger = null }) { +export function openInputDialog({ scope, title, label, action, value = '', maxLength, fallback, onApply, trigger = null, + validate = null }) { requireScope(scope); requireCallbacks({ onApply }); + if (validate !== null) requireCallbacks({ validate }); for (const [name, text] of Object.entries({ title, label, action })) { if (typeof text !== 'string' || !text) throw new TypeError(`The input dialog needs a ${name}`); } @@ -314,9 +324,17 @@ export function openInputDialog({ scope, title, label, action, value = '', maxLe const cancel = createButton('Cancel'); const apply = createButton(action, 'ca-primary'); cancel.addEventListener('click', () => finish()); + input.addEventListener('input', () => input.setCustomValidity('')); apply.addEventListener('click', () => { const entered = input.value.trim() || fallback; if (!entered) { input.focus(); return; } + const problem = validate?.(entered); + if (problem) { + input.setCustomValidity(problem); + input.reportValidity(); + input.focus(); + return; + } finish(); onApply(entered); }); @@ -327,14 +345,16 @@ export function openInputDialog({ scope, title, label, action, value = '', maxLe /** * The concept's plain `dialog(title, body)` with page-built `nodes`. `onClose` runs once when the dialog closes or - * another dialog replaces its body, so the page can dispose what it mounted there. + * another dialog replaces its body, so the page can dispose what it mounted there. While `canClose()` answers false, + * × and Escape leave the dialog open (the page, a route change or a replacing dialog still close it). */ -export function openContentDialog({ scope, title, nodes, onClose = null, trigger = null }) { +export function openContentDialog({ scope, title, nodes, onClose = null, trigger = null, canClose = null }) { requireScope(scope); if (typeof title !== 'string' || !title) throw new TypeError('The dialog needs a title'); if (!Array.isArray(nodes) || nodes.some(node => !node || typeof node !== 'object')) throw new TypeError('The dialog needs its nodes'); if (onClose !== null && typeof onClose !== 'function') throw new TypeError('The dialog onClose must be a function'); - show(title, nodes, { scope, trigger, onClose }); + if (canClose !== null && typeof canClose !== 'function') throw new TypeError('The dialog canClose must be a function'); + show(title, nodes, { scope, trigger, onClose, canClose }); } export const SCAN_REVIEW_SENTENCE = 'Enrich positive / negative associations, reuse canonical text and report unchanged files. ' +