Cache packaged stop vocabulary in the hot path - #845
Conversation
📝 WalkthroughWalkthroughStopService now uses a cached vocabulary loader. The cache separates entries by vocabulary name and language, limits storage to 32 entries, and clears during shutdown. Unit tests verify reuse, isolation, returned values, and loader calls. ChangesStop resource caching
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ovos_core/intent_services/stop_service.py (1)
30-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse an instance-scoped vocabulary cache.
@lru_cacheon_CachedStopResources.load_vocabulary()makesselfpart of the cache key and shares themaxsize=32limit across(resource, base_name, lang)entries. Localize the cache to the created_CachedStopResourcesinstance and clear it when the service is restarted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ovos_core/intent_services/stop_service.py` around lines 30 - 33, Replace the class-level `@lru_cache` on _CachedStopResources.load_vocabulary with an instance-scoped cache so entries are isolated per resource instance and keyed only by vocabulary inputs. Initialize the cache when _CachedStopResources is created, route load_vocabulary through it, and clear that cache during the service restart/reset flow.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ovos_core/intent_services/stop_service.py`:
- Around line 30-33: Replace the class-level `@lru_cache` on
_CachedStopResources.load_vocabulary with an instance-scoped cache so entries
are isolated per resource instance and keyed only by vocabulary inputs.
Initialize the cache when _CachedStopResources is created, route load_vocabulary
through it, and clear that cache during the service restart/reset flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 56136421-fe51-49b5-b582-4b915876c8c5
📒 Files selected for processing (2)
ovos_core/intent_services/stop_service.pytest/unittests/test_stop_resource_cache.py
| from ovos_utils.parse import match_one | ||
|
|
||
|
|
||
| class _CachedStopResources(LocaleResources): |
There was a problem hiding this comment.
why not add a caching option to LocaleResources class directly instead?
There was a problem hiding this comment.
Agreed. I moved the cache into the resource owner in OpenVoiceOS/ovos-spec-tools#95. It is opt-in and instance-local, defaults off so live overrides remain live, returns defensive copies, and has explicit invalidation. Once that is reviewed and released, this PR should replace _CachedStopResources with LocaleResources(..., expanded_cache_size=32) and bump the explicit ovos-spec-tools minimum. I am leaving this thread open until that consumer rebase is complete; no fallback or getattr compatibility path will be added.
Maintainer-requested architecture update
Jarbas correctly requested that resource lifecycle and caching belong to
LocaleResources, not to a Core-local subclass.OpenVoiceOS/ovos-spec-tools#95 now implements that ownership model directly:
user_localeremains live;getattr, or compatibility fallback exists.The exact-head benchmark on the Core stop path measured 720.327 µs → 112.769 µs per request across
en-usandfr-fr: 6.4× faster and 607.558 µs CPU saved per request. Full details and multi-skill measurements are in OpenVoiceOS/ovos-spec-tools#95.Current state
This branch still contains the superseded
_CachedStopResourcesimplementation and must not merge as-is.After #95 is merged and released, this PR should be reduced to:
dev;_CachedStopResourcesand its duplicate cache tests;LocaleResources(...)construction;ovos-spec-toolsminimum to the released version;No consumer API switch is otherwise required because
LocaleResourcesnow owns the behavior by default. No CI workflow was changed and no upstream PR was merged.