Preload installed locale resources and keep user overrides live - #95
Open
goldyfruit wants to merge 4 commits into
Open
Preload installed locale resources and keep user overrides live#95goldyfruit wants to merge 4 commits into
goldyfruit wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesLocale resource lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant LocaleResources
participant UserResources
participant StaticResourceIndex
Caller->>LocaleResources: request resource or expansion
LocaleResources->>UserResources: read current user files
UserResources-->>LocaleResources: return override or no match
LocaleResources->>StaticResourceIndex: read indexed static resource
StaticResourceIndex-->>LocaleResources: return cached static value
LocaleResources-->>Caller: return fresh result
🚥 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 |
goldyfruit
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/test_resources.py (1)
130-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompare resolved paths in
counted_reader, and do not shadow the module name.
_snapshot_static_sourcesreads throughlang_target.resolve(), sopathis a resolved path. The test compares it to the unresolvedresource. If the temporary root is a symlink, the comparison fails,readsstays0, and the assertion fails for an unrelated reason. Also, rebindingresourcesto theLocaleResourcesinstance hides the module that was just patched.♻️ Proposed test hardening
- def counted_reader(path): - nonlocal reads - if path == resource: - reads += 1 - return original_reader(path) - - monkeypatch.setattr(resources, "read_resource_file", counted_reader) - resources = LocaleResources(str(locale)) - - first = resources.load_vocabulary("stop", "en-US") - first.append("mutated by caller") - assert resources.load_vocabulary("stop", "en-US") == ["stop"] - assert reads == 1 + target = resource.resolve() + + def counted_reader(path): + nonlocal reads + if Path(path).resolve() == target: + reads += 1 + return original_reader(path) + + monkeypatch.setattr(resources, "read_resource_file", counted_reader) + loader = LocaleResources(str(locale)) + + first = loader.load_vocabulary("stop", "en-US") + first.append("mutated by caller") + assert loader.load_vocabulary("stop", "en-US") == ["stop"] + assert reads == 1The diff assumes
Pathis imported in this module.🤖 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 `@test/test_resources.py` around lines 130 - 152, Update test_static_resources_are_read_once_and_return_defensive_copies so counted_reader compares path against resource.resolve(), and use a distinct variable name for the LocaleResources instance instead of rebinding the patched resources module. Keep the existing assertions and read-count behavior unchanged.
🤖 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.
Inline comments:
In `@ovos_spec_tools/resources.py`:
- Around line 427-432: The documentation must clarify that expanded results are
cached and pre-expanded only when no user_locale is configured, while the
resource index, file contents, dialogs, and prompts remain cached in both
configurations. Update ovos_spec_tools/resources.py lines 427-432 and
docs/locale-resources.md lines 86-96 to state this condition consistently.
- Around line 505-532: Update the static-language initialization around the
source-directory traversal so construction indexes resource paths and groups
them by language without calling read_resource_file or read_prompt_file for
every language. Defer those reads until the corresponding language is first
requested, then cache the loaded contents for reuse while preserving
language_index and _static_lines/_static_prompts behavior.
- Around line 461-491: The constructor currently lacks the documented opt-in
expanded cache API and always snapshots static resources, preventing live locale
reloads. Add the public expanded_cache_size option to the resource class
constructor, default it to disabled, use it to control _snapshot_static_sources
and expanded-resource caching, and provide an explicit refresh/invalidation
method that clears relevant caches and rebuilds snapshots; preserve existing
behavior when caching is enabled.
---
Nitpick comments:
In `@test/test_resources.py`:
- Around line 130-152: Update
test_static_resources_are_read_once_and_return_defensive_copies so
counted_reader compares path against resource.resolve(), and use a distinct
variable name for the LocaleResources instance instead of rebinding the patched
resources module. Keep the existing assertions and read-count behavior
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 466febac-edfd-43e8-a65f-cab2d828c921
📒 Files selected for processing (5)
docs/api-reference.mddocs/locale-resources.mdovos_spec_tools/resources.pytest/test_find_lang_dir.pytest/test_resources.py
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
LocaleResourcesnow follows the installed-resource lifecycle directly:user_localetree stays live, so create/update/delete is visible without restarting;The earlier opt-in bounded LRU and
expanded_cache_sizeAPI were removed. Installed resources have no runtime invalidation path because replacing an installed package or skill already requires recreating its owning service/resource loader. There is no arbitrary capacity, eviction policy, subclass, or compatibility fallback.The snapshot also gives standardized exact locale directories precedence over macro-language fallback. For example,
eu-ESremains distinct when botheu/andeu-ES/exist. Explicit custom resolvers retain control.Why
The runtime previously repeated installed-directory discovery, file reads, vocabulary-map assembly, and template expansion. Those inputs are static for the process lifetime. User overrides are the only source that needs runtime change detection, and they are deliberately excluded from the immutable snapshot.
Benchmark
Compared exact PR head
80cd7c8056858549197333b47320394967660f2cwithdevbaseline18272335d247e88e639477d7793753b2135a38b5on Python 3.14.6 and an AMD Ryzen AI 9 HX 370. The harness loaded both implementations in the same process, warmed filesystem pages, and reported medians. Expanded-resource trials used at least 1,600 lookups; the StopService-equivalent trial used 2,000 requests acrossen-usandfr-frand performed bothstopandglobal_stopvocabulary matches.Runtime hot path
devThe optional empty live-user layer costs 59–77 µs per lookup because it must check for runtime changes. It remains isolated from the installed static path. This is a resource-loader microbenchmark and does not claim an equivalent end-to-end assistant latency reduction.
Five existing Core stop resources fail strict expansion. The constructor snapshots their source data but deliberately defers their existing validation failures until those individual resources are accessed; unused locales therefore do not become startup failures.
Validation
uv run --extra test pytest -q— 582 passeduv run ruff check ovos_spec_tools/resources.py test/test_resources.py test/test_find_lang_dir.py— passeduv run --with build python -m build— sdist and wheel builtgit diff --check— passedTests cover immutable skill/core snapshots, defensive copies, live user create/update/delete, static-intent re-expansion from live user vocabularies, live dialogs/prompts, malformed-resource fault isolation, exact regional locale selection, and custom resolver behavior.
No CI workflow was changed.
Summary by CodeRabbit
New Features
Bug Fixes