Repository navigation
fix: stop lru_cache on TokenizerSPM.__call__ from leaking instances - #299
Conversation
|
The failing job (`check-build (ubuntu-latest, 3.11)`) is failing on Ruff lint, not on the actual change or the test suite. Looked into it: the workflow uses `chartboost/ruff-action@v1` with no pinned version, so it pulls whatever the latest Ruff release is at run time. All 331 flagged errors are on pre-existing code across the repo (e.g. `test_significance.py`, `test_ter.py`) — including 3 in the file I touched (`tokenizer_spm.py`), but all 3 are on lines I didn't change (the file's top-of-file coding comment, its existing import block, and a pre-existing `.warn()` call at line 42 — my fix is at lines 57-78). This looks like Ruff-version drift against unpinned CI rather than anything introduced by this PR — happy to rebase/re-push if useful once resolved on the repo side, just didn't want it to look like this diff broke the build. |
0700b8c to
2552af9
Compare
sentence_bleu()/corpus_bleu() construct a fresh BLEU instance (and therefore a fresh tokenizer instance) on every call. lru_cache applied as a decorator on the instance method __call__ keys its cache on (self, line), so the shared, class-level cache accumulates a strong reference to every tokenizer instance -- and its loaded SentencePieceProcessor -- ever created, for the life of the process. Since each instance is only ever called with the same handful of lines, the cache never gets a hit across calls either, so it provides no benefit while still leaking. Move the cache to be created per instance instead, bound to self._tokenize. The cache then dies with the instance, and reuse within a single instance's lifetime (e.g. many sentences scored by one corpus-level BLEU instance) is still memoized exactly as before.
2552af9 to
2d3be9a
Compare
2 participants
Closes #264
Root cause
sentence_bleu()/corpus_bleu()construct a freshBLEUinstance — and therefore a fresh tokenizer instance — on every top-level call (sacrebleu/compat.py→BLEU.__init__→sacrebleu/metrics/bleu.py:203,self.tokenizer = _get_tokenizer(best_tokenizer)()).TokenizerSPM.__call__was decorated with@lru_cache(maxsize=2**16)directly on the instance method.lru_cacheon a method keys its cache on(self, line)— so with a fresh tokenizer instance every call, the shared class-level cache accumulates a strong reference to every instance (and its loadedSentencePieceProcessor, which holds the full SPM model in memory) ever created, for the life of the process. Worse, since each instance is only ever called with the same one or two lines, the cache never actually gets a hit across calls — it provides zero benefit while still leaking.Fix
Move the
lru_cacheto be created per-instance in__init__, bound toself._tokenize(a bound method, so the cache key becomes justline, not(self, line)). The cache then dies naturally with the instance via normal garbage collection, while memoization within a single instance's lifetime (e.g. many sentences scored by one corpus-levelBLEUobject) works exactly as before.Validation
sentence_bleu()→BLEU.__init__→_get_tokenizer(...)()creates a new tokenizer instance per call (traced the exact call chain, not assumed).sentence_bleu(..., tokenize="flores101")in a loop) for 300 iterations with the fix applied: memory usage plateaus after initial model-load overhead rather than growing with iteration count, and the per-instance cache (cache_info()) correctly showscurrsize=0between calls, confirming each instance's cache is being collected rather than accumulating. I kept the iteration count modest and didn't run it to the original crash point — happy to run a longer soak test if useful for review, just didn't want to load down my own machine further while iterating on this.