Caching opened tantivy.Indexes in the package
#627
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dosubot
bot
added
size:L
whitead
approved these changes
Oct 22, 2024
mskarlin
reviewed
Oct 22, 2024
| index_path = await self.index_filename | ||
| if await (index_path / "meta.json").exists(): | ||
| self._index = Index.open(path=str(index_path)) # type: ignore[assignment] | ||
| index_meta_directory = await self.index_filename |
There was a problem hiding this comment.
I think we can hit a race condition here (because the exists check is awaitable). We should prob. acquire a lock and then do the check or do the check synchronously.
Otherwise I think a "gather" call could result in several False responses to this exists call.
There was a problem hiding this comment.
Ok we talked about this, because we are only instantiating and Index and not opening one, this likely isn't an issue. @jamesbraza is going to add a comment for future reference.
There was a problem hiding this comment.
Yep thanks for the discussion! I added a comment to the code documenting some of our talking points
…directory_index per trajectory
…all get_directory_index per trajectory" This reverts commit e7f4519.
with added test assertions
…ementation is used
jamesbraza
force-pushed
the
fixing-index-reopening
branch
from
7b39d7a to
fe6ed72
Compare
mskarlin
approved these changes
Oct 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
quickwit-oss/tantivy-py#359 (comment) reveals that one of our servers can only handle three opened indexes at once. The reason why remains unclear, but this PR at least reacts to the issue by caching opened indexes in the
search.pymodule's scope. Now we can invokeawait SearchIndex(index_name="normal-index").indexas many times as desired within one Python process.Implementation Details
Why a global scope? We want to accommodate caching the
Indexacross:deepcopyof anPaperQAEnvironment, whosetoolswhich contains aPaperSearchtool instance.IndexinPaperSearch, or else we'd be makingIndexcopies (and the side effects of this are unknown)Trajectory, where we (1) build the index and (2) use the index in 0+ paper searchesTaskDataset, where we (1) build the index and (2) run 0+ envs for one trajectory eachTaskDataset, for larger experimentsThis can only be accomplished using global scope, whose lifetime matches the entire Python process. This unfortunately requires callers to invoke the newly added
reap_opened_index_cacheat runtime if intermediary cleaning of the cache is desired.The caching added here can be disabled by setting
1ortrue(case insensitive) to the newly-added environment variablePQA_INDEX_DONT_CACHE_INDEXES.Risks
reap_opened_index_cachewhile also usingPaperSearchreap_opened_index_cacheto avoid this, but the tradeoff is our testing slowly accrues indexes across cases