diff --git a/chrome/content/zotero/collectionViewItemTree.jsx b/chrome/content/zotero/collectionViewItemTree.jsx index 37e64fb00a..b762c83381 100644 --- a/chrome/content/zotero/collectionViewItemTree.jsx +++ b/chrome/content/zotero/collectionViewItemTree.jsx @@ -184,17 +184,12 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { } /** - * Compute the current index coverage across the selected rows' libraries - * (see Zotero.BestMatch.getIndexState()) + * Compute the current index coverage (see Zotero.BestMatch.getIndexState()) * * @return {Promise} */ async _getBestMatchIndexState() { - return Zotero.BestMatch.getIndexState( - this.collectionTreeRows - .map(row => row.ref?.libraryID) - .filter(id => id !== undefined) - ); + return Zotero.BestMatch.getIndexState(); } /** @@ -576,12 +571,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { this.itemTree._refreshPromise = deferred.promise; try { - // A best-match rerank after an embeddings change reuses the cached - // search results -- only the ranking depends on the embeddings, so - // there's no need to re-run the underlying search - if (!options.reuseSearchResults) { - this.collectionTreeRows.forEach(row => row.clearCache()); - } + this.collectionTreeRows.forEach(row => row.clearCache()); this._bestMatchIndexState = null; // Get the full set of items we want to show, merged across all selected rows let newSearchItemSet = new Set(); @@ -942,7 +932,6 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { var madeChanges = false; var refresh = false; - var reuseSearchResults = false; var sort = false; // Selection strategy @@ -993,23 +982,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { if (items.length == 0) return; } - if (action == 'refresh' && type == 'item' && extraData && extraData.embeddingsUpdate - && collectionTreeRows.some(rowIsBestMatchSearch)) { - // The background indexer committed new or changed embeddings, so - // rerun the active best-match search to update the scores and ranks. - // Only the ranking depends on the embeddings, so unless a selected - // row trims membership by score (a top-K cutoff), the underlying - // search results are unchanged and can be reused while just the - // ranking is recomputed. - reuseSearchResults = collectionTreeRows.every((row) => { - return typeof row.hasBestMatchCutoff == 'function' - && !row.hasBestMatchCutoff(); - }); - this.itemTree.invalidateRowCache(ids); - refresh = true; - madeChanges = true; - } - else if (action == 'refresh' && type == 'item' && this._bestMatchSession + if (action == 'refresh' && type == 'item' && this._bestMatchSession && ids.some(id => this._bestMatchSession.getPreviews(parseInt(id)))) { // The invalidation above reset these items' previews, and only a // scoring pass derives previews, so re-run the search @@ -1292,7 +1265,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider { } if (refresh) { - await this._refresh({ reuseSearchResults }); + await this._refresh(); } if (sort) { await this.itemTree._ensureSortContextReady(); diff --git a/chrome/content/zotero/xpcom/bestMatch.js b/chrome/content/zotero/xpcom/bestMatch.js index d3b54ae618..cf362a0349 100644 --- a/chrome/content/zotero/xpcom/bestMatch.js +++ b/chrome/content/zotero/xpcom/bestMatch.js @@ -132,37 +132,33 @@ Zotero.BestMatch = new function () { }; /** - * Embedding-index coverage over the given libraries, for banners - * explaining incomplete best-match results. Null when the semantic - * engine is disabled or every eligible item is indexed. Never throws -- - * the state is informational and shouldn't break a search. + * Embedding-index coverage, for banners explaining incomplete best-match + * results. Null when the semantic engine is disabled or everything + * eligible is indexed. Never throws -- the state is informational and + * shouldn't break a search. * - * @param {Number[]} [libraryIDs] - Limit coverage to these libraries; - * all libraries when empty * @return {Promise} - { type: 'indexing'|'paused', indexed, - * total } + * total }, where the counts add items to attachment chunks */ - this.getIndexState = async function (libraryIDs = []) { + this.getIndexState = async function () { try { let status = Zotero.Embeddings.Indexing.getStatus(); if (!status.enabled) { return null; } // Counts aren't populated until the indexer runs in this session - if (!status.libraries.length) { + if (!status.items.total && !status.chunks.total) { status = await Zotero.Embeddings.Indexing.refreshStatus(); } - let ids = new Set(libraryIDs); - let libraries = status.libraries - .filter(lib => !ids.size || ids.has(lib.libraryID)); // Coverage is coverage: attachment fulltext is reported separately // in the preferences, but an incomplete index is incomplete - // whichever part of it is still filling in - let indexed = libraries.reduce( - (sum, lib) => sum + lib.indexed + lib.indexedAttachments, 0); - let total = libraries.reduce( - (sum, lib) => sum + lib.eligible + lib.eligibleAttachments, 0); - if (indexed >= total) { + // whichever part of it is still filling in. Queued work and + // documents still being extracted aren't in the totals yet. + let indexed = status.items.done + status.chunks.done; + let total = status.items.total + status.chunks.total; + let pending = status.queued.items || status.queued.attachments + || status.extractionProgress; + if (indexed >= total && !pending) { return null; } // Only an explicit pause reports as paused. Anything else -- diff --git a/chrome/content/zotero/xpcom/embeddings.js b/chrome/content/zotero/xpcom/embeddings.js index 5f85af75ee..51dc611a69 100644 --- a/chrome/content/zotero/xpcom/embeddings.js +++ b/chrome/content/zotero/xpcom/embeddings.js @@ -1741,12 +1741,6 @@ Zotero.Embeddings.Indexing = new function () { // clear/prune/re-index steps concurrently. let _switchChain = Promise.resolve(); - // Items whose embeddings were written but not yet announced to views, and - // the coalescing timer for the announcement (see _notifyIndexed()) - let _indexedNotifyIDs = new Set(); - let _indexedNotifyTimer = null; - const INDEXED_NOTIFY_DELAY = 2000; - /** * Wire up the background indexer. Guarded so multiple windows don't * double-initialize. @@ -2208,17 +2202,8 @@ Zotero.Embeddings.Indexing = new function () { // the computed vectors, not the downloaded model files. async function _clearEmbeddings() { await Zotero.Embeddings.initDB(); - // Announce the removals, so active semantic views refresh after the - // notification's coalescing delay (e.g. after disabling or a model - // switch) - let cleared = await Zotero.DB.columnQueryAsync( - "SELECT DISTINCT itemID FROM embeddings.itemEmbeddings" - ); await Zotero.DB.queryAsync("DELETE FROM embeddings.itemEmbeddings"); await Zotero.DB.queryAsync("DELETE FROM embeddings.itemChunkCounts"); - if (cleared.length) { - _notifyIndexed(cleared); - } } // Indexed items, notes and annotations -- the numerator for their @@ -2906,10 +2891,7 @@ Zotero.Embeddings.Indexing = new function () { } } }); - if (completed.length) { - _notifyIndexed(completed.map(entry => entry.item.id)); - done += completed.length; - } + done += completed.length; if (onProgress) { onProgress({ done, total: toEmbed.length }); } @@ -2924,28 +2906,6 @@ Zotero.Embeddings.Indexing = new function () { .filter(library => ['user', 'group'].includes(library.libraryType)); } - // Announce written or removed embeddings with a 'refresh' item event, so - // an active best-match search reranks as vectors change (e.g. during - // initial indexing, or after a clear). The embeddingsUpdate flag lets the - // item tree rerank only for these events, not for every refresh. Coalesced, - // so a long indexing run produces an update every couple of seconds rather - // than one per committed batch. - function _notifyIndexed(itemIDs) { - for (let id of itemIDs) { - _indexedNotifyIDs.add(id); - } - if (_indexedNotifyTimer) { - return; - } - _indexedNotifyTimer = setTimeout(() => { - _indexedNotifyTimer = null; - let ids = [..._indexedNotifyIDs]; - _indexedNotifyIDs.clear(); - Zotero.Notifier.trigger('refresh', 'item', ids, { embeddingsUpdate: true }) - .catch(e => Zotero.logError(e)); - }, INDEXED_NOTIFY_DELAY); - } - /** * Current runner state, for the preferences UI. * diff --git a/chrome/content/zotero/xpcom/notifier.js b/chrome/content/zotero/xpcom/notifier.js index 75776fe3ff..2d7af1969d 100644 --- a/chrome/content/zotero/xpcom/notifier.js +++ b/chrome/content/zotero/xpcom/notifier.js @@ -27,7 +27,7 @@ Zotero.Notifier = new function () { // Options that apply to an entire event, not a specific object - this.EVENT_LEVEL_OPTIONS = ['autoSyncDelay', 'skipAutoSync', 'embeddingsUpdate']; + this.EVENT_LEVEL_OPTIONS = ['autoSyncDelay', 'skipAutoSync']; var _observers = {}; var _types = [ diff --git a/test/tests/collectionViewItemTreeTest.js b/test/tests/collectionViewItemTreeTest.js index 234576b4fa..91c88f5cc8 100644 --- a/test/tests/collectionViewItemTreeTest.js +++ b/test/tests/collectionViewItemTreeTest.js @@ -277,13 +277,10 @@ describe("CollectionViewItemTree", function () { enabled: true, indexing: false, paused: false, - libraries: [{ - libraryID: Zotero.Libraries.userLibraryID, - indexed: 0, - eligible: 0, - indexedAttachments: 0, - eligibleAttachments: 0 - }] + items: { done: 1, total: 1 }, + chunks: { done: 0, total: 0 }, + queued: { items: 0, attachments: 0 }, + extractionProgress: null })); Zotero.Prefs.set('search.quicksearch-mode', 'bestMatch'); }); @@ -891,13 +888,10 @@ describe("CollectionViewItemTree", function () { enabled: true, indexing: true, paused: false, - libraries: [{ - libraryID: Zotero.Libraries.userLibraryID, - indexed: 700, - eligible: 9000, - indexedAttachments: 52, - eligibleAttachments: 553 - }] + items: { done: 700, total: 9000 }, + chunks: { done: 52, total: 553 }, + queued: { items: 0, attachments: 0 }, + extractionProgress: null }); await select(win, col); @@ -919,13 +913,10 @@ describe("CollectionViewItemTree", function () { enabled: true, indexing: false, paused: false, - libraries: [{ - libraryID: Zotero.Libraries.userLibraryID, - indexed: 700, - eligible: 9000, - indexedAttachments: 52, - eligibleAttachments: 553 - }] + items: { done: 700, total: 9000 }, + chunks: { done: 52, total: 553 }, + queued: { items: 0, attachments: 0 }, + extractionProgress: null }); await itemsView.setFilter('search', 'between runs query'); banner = win.document.querySelector('.best-match-index-banner'); @@ -934,13 +925,10 @@ describe("CollectionViewItemTree", function () { enabled: true, indexing: false, paused: true, - libraries: [{ - libraryID: Zotero.Libraries.userLibraryID, - indexed: 700, - eligible: 9000, - indexedAttachments: 52, - eligibleAttachments: 553 - }] + items: { done: 700, total: 9000 }, + chunks: { done: 52, total: 553 }, + queued: { items: 0, attachments: 0 }, + extractionProgress: null }); await itemsView.setFilter('search', 'paused query'); banner = win.document.querySelector('.best-match-index-banner'); @@ -951,13 +939,10 @@ describe("CollectionViewItemTree", function () { enabled: true, indexing: false, paused: false, - libraries: [{ - libraryID: Zotero.Libraries.userLibraryID, - indexed: 9000, - eligible: 9000, - indexedAttachments: 553, - eligibleAttachments: 553 - }] + items: { done: 9000, total: 9000 }, + chunks: { done: 553, total: 553 }, + queued: { items: 0, attachments: 0 }, + extractionProgress: null }); await itemsView.setFilter('search', 'another query'); assert.notOk(win.document.querySelector('.best-match-index-banner')); @@ -1083,28 +1068,7 @@ describe("CollectionViewItemTree", function () { await itemsView.setFilter('advanced-search', null); }); - it("should rerank when the indexer announces changed embeddings", async function () { - let col = await createDataObject('collection'); - let itemA = await createDataObject('item', { title: "rerank A", collections: [col.id] }); - let itemB = await createDataObject('item', { title: "rerank B", collections: [col.id] }); - let best = itemA.id; - stubs.push(sinon.stub(Zotero.Embeddings, 'scoreItemIDs').callsFake(scoreEnvelope( - async (query, itemIDs) => new Map(itemIDs.map(id => [id, id == best ? 0.9 : 0.5])) - ))); - - await select(win, col); - itemsView = zp.itemsView; - await itemsView.setFilter('search', 'some query'); - assert.deepEqual(itemsView._rows.map(row => row.id), [itemA.id, itemB.id]); - - // The indexer's coalesced notification after new/removed vectors - best = itemB.id; - await Zotero.Notifier.trigger('refresh', 'item', [itemA.id, itemB.id], { embeddingsUpdate: true }); - await itemsView._refreshPromise; - assert.deepEqual(itemsView._rows.map(row => row.id), [itemB.id, itemA.id]); - }); - - it("shouldn't rerank on a refresh that isn't an embeddings update", async function () { + it("shouldn't rerank on a refresh", async function () { let col = await createDataObject('collection'); let itemA = await createDataObject('item', { title: "norerank A", collections: [col.id] }); let itemB = await createDataObject('item', { title: "norerank B", collections: [col.id] }); @@ -1118,38 +1082,13 @@ describe("CollectionViewItemTree", function () { await itemsView.setFilter('search', 'some query'); assert.deepEqual(itemsView._rows.map(row => row.id), [itemA.id, itemB.id]); - // An unrelated refresh (e.g. a field change) leaves the ranking alone + // A refresh (e.g. a field change) leaves the ranking alone best = itemB.id; await Zotero.Notifier.trigger('refresh', 'item', [itemA.id, itemB.id]); await itemsView._refreshPromise; assert.deepEqual(itemsView._rows.map(row => row.id), [itemA.id, itemB.id]); }); - it("should rerank without re-running the search when embeddings change", async function () { - let col = await createDataObject('collection'); - let itemA = await createDataObject('item', { title: "reuse A", collections: [col.id] }); - let itemB = await createDataObject('item', { title: "reuse B", collections: [col.id] }); - let best = itemA.id; - stubs.push(sinon.stub(Zotero.Embeddings, 'scoreItemIDs').callsFake(scoreEnvelope( - async (query, itemIDs) => new Map(itemIDs.map(id => [id, id == best ? 0.9 : 0.5])) - ))); - - await select(win, col); - itemsView = zp.itemsView; - await itemsView.setFilter('search', 'some query'); - assert.deepEqual(itemsView._rows.map(row => row.id), [itemA.id, itemB.id]); - - // The underlying search is dropped and re-run by clearing the row cache - let clearCacheSpy = sinon.spy(Zotero.CollectionTreeRow.prototype, 'clearCache'); - stubs.push(clearCacheSpy); - - best = itemB.id; - await Zotero.Notifier.trigger('refresh', 'item', [itemA.id, itemB.id], { embeddingsUpdate: true }); - await itemsView._refreshPromise; - - assert.deepEqual(itemsView._rows.map(row => row.id), [itemB.id, itemA.id]); - assert.isFalse(clearCacheSpy.called); - }); }); it("should expand parent item and attachment for an annotation match", async function () { diff --git a/test/tests/embeddingsTest.js b/test/tests/embeddingsTest.js index 4120e31ef6..46af3d6c83 100644 --- a/test/tests/embeddingsTest.js +++ b/test/tests/embeddingsTest.js @@ -1285,7 +1285,7 @@ describe("Zotero.Embeddings", function () { }); describe("Indexing", function () { - it("should announce cleared embeddings when the model changes", async function () { + it("should clear embeddings when the model changes", async function () { let stubs = [ sinon.stub(Zotero.Embeddings.Indexing, 'startIndexing').resolves(), sinon.stub(Zotero.Embeddings, 'pruneModels').resolves() @@ -1303,13 +1303,9 @@ describe("Zotero.Embeddings", function () { + "VALUES (?, ?, 1)", [item.id, 'hash'] ); - // The model switch clears the old vectors and announces the - // removals (after the coalescing delay), so active semantic - // views refresh - let promise = waitForNotifierEvent('refresh', 'item'); + // The model switch clears the old vectors Zotero.Prefs.set('embeddings.model', 'bge-small-en-v1.5'); - let event = await promise; - assert.include(event.ids, item.id); + await Zotero.Embeddings.Indexing.waitForPendingModelSwitch(); assert.equal( await Zotero.DB.valueQueryAsync( "SELECT COUNT(*) FROM embeddings.itemEmbeddings"