refactor: migrate language matching & template expansion to ovos-spec-tools#411
refactor: migrate language matching & template expansion to ovos-spec-tools#411JarbasAl wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughMigrates dialog/vocabulary/intent rendering to ovos-spec-tools (spec-based expand/render), changes locale matching to use lang_distance, adds deprecation shims for legacy resource classes and dialog_renderer, and updates OVOSSkill APIs to use the new render path. ChangesSpec-based Resource Rendering and Deprecation
Sequence Diagram(s)sequenceDiagram
participant OVOSSkill
participant SkillResources
participant DialogFile
participant MustacheDialogRenderer
OVOSSkill->>SkillResources: render_dialog(key, data)
SkillResources->>DialogFile: resource_file.render()
DialogFile->>MustacheDialogRenderer: _spec_render(template, slots=data)
MustacheDialogRenderer-->>DialogFile: rendered utterance
DialogFile-->>SkillResources: final dialog text
SkillResources-->>OVOSSkill: returned utterance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…ec-tools Wave 2 of the OVOS migration (OpenVoiceOS/architecture#7). Replace reimplemented language-matching and template-expansion logic with the conformant reference implementation from ovos-spec-tools. - resource_files.py: locate_lang_directories now uses ovos_spec_tools.lang_distance instead of langcodes.tag_distance; vocabulary/intent loading uses ovos_spec_tools.expand instead of ovos_utils.bracket_expansion.expand_template. expand is single-spaced per OVOS-INTENT-1, fixing double spaces left when an optional segment is dropped. sorted() preserves the previous deterministic ordering. - converse.py: _get_closest_lang uses ovos_spec_tools.closest_lang, which standardizes both tags and applies the spec's distance-below-10 threshold internally. - Add ovos-spec-tools>=0.0.1a2 to dependencies. All migrated symbols were internal-only; no public API changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JarbasAl
force-pushed
the
feat/use-ovos-spec-tools
branch
from
60f53e2 to
9ddcac5
Compare
Synchronizing... Check results have been successfully retrieved. 📡I've aggregated the results of the automated checks for this PR below. 🔍 LintAnother check completed successfully! 🏁 ❌ ruff: issues found — see job log ⚖️ License CheckI've checked the license history of this repo. 📜 ✅ No license violations found (49 packages). License distribution: 12× MIT License, 8× Apache Software License, 7× Apache-2.0, 7× MIT, 2× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +7 more Full breakdown — 49 packages
Copyright (c) 2022 Phil Ewels Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔒 Security (pip-audit)Scanning the horizon for any zero-day threats. 🌅 ✅ No known vulnerabilities found (68 packages scanned). 🔨 Build TestsThe build report is now ready for your review. 📝
❌ 3.10: Install OK, tests failed 📋 Repo HealthEnsuring the repo is getting enough sleep (aka stable releases). 💤 ✅ All required files present. Latest Version: ✅ Generated with ❤️ by OVOS Automations |
Wave 2 of the OVOS migration (OpenVoiceOS/architecture#7). Replace the reimplemented MustacheDialogRenderer-based dialog rendering with the conformant reference renderer from ovos-spec-tools. - resource_files.py: DialogFile.render / IntentFile.render now load the raw phrase list and render it with ovos_spec_tools.render, which expands (a|b)/[x] variants and fills {name} slots per OVOS-DIALOG-1. SkillResources.render_dialog renders directly without routing through a MustacheDialogRenderer. SkillResources keeps handling the legacy resource types (qml, json, list, value, regex, word, template) that ovos-spec-tools does not cover. - ovos.py: OVOSSkill gains render_dialog, which renders via self.resources.render_dialog; speak_dialog and the get_response / ask on-fail flows call render_dialog instead of dialog_renderer.render. - The SkillResources.dialog_renderer and OVOSSkill.dialog_renderer properties are kept as deprecated compat shims (warnings.warn + @deprecated) that lazily build a MustacheDialogRenderer for legacy callers; removal version computed from VERSION_MAJOR. A missing .dialog file still falls back to the dialog name with dots replaced by spaces (eg "record.not.found" -> "record not found"), the documented contract of the previous renderer. Unfilled slots now raise ovos_spec_tools.UnfilledSlot instead of being silently dropped. BREAKING CHANGE: dialog rendering is now performed by ovos_spec_tools.render. The dialog_renderer properties on SkillResources and OVOSSkill are deprecated compat shims. A dialog phrase with a {name} slot that has no value now raises UnfilledSlot instead of producing a partially-rendered string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ovos_workshop/resource_files.py (1)
93-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a deterministic tiebreaker to language-directory sorting.
Line 101 sorts only by distance. When two folders score the same, the final order falls back to
iterdir()order, and downstream callers use the first result as the selected fallback language directory. That makes dialect selection nondeterministic across filesystems. Sort by(score, folder.name)here to keep the fallback stable.Suggested fix
- candidates = sorted(candidates, key=lambda k: k[1]) + candidates = sorted(candidates, key=lambda k: (k[1], k[0].name))🤖 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_workshop/resource_files.py` around lines 93 - 101, The sorting of candidate language directories only uses the distance score, causing nondeterministic order when scores tie; update the sort for candidates (after collecting tuples (folder, score) using lang_distance) to use a deterministic tiebreaker by sorting on (score, folder.name) instead of key=lambda k: k[1], so tied scores are resolved consistently by folder.name.
🤖 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_workshop/resource_files.py`:
- Around line 642-648: The current shim that populates self._dialog_renderer
only iterates immediate children of each directory, missing nested .dialog
files; update the loop over base_dirs (where renderer =
MustacheDialogRenderer()) to walk directories recursively (e.g., use
Path.rglob("*.dialog") or an os.walk) and call
renderer.load_template_file(path.stem, str(path)) for each matching file so all
nested .dialog templates are loaded into the dialog renderer.
---
Outside diff comments:
In `@ovos_workshop/resource_files.py`:
- Around line 93-101: The sorting of candidate language directories only uses
the distance score, causing nondeterministic order when scores tie; update the
sort for candidates (after collecting tuples (folder, score) using
lang_distance) to use a deterministic tiebreaker by sorting on (score,
folder.name) instead of key=lambda k: k[1], so tied scores are resolved
consistently by folder.name.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1244fcab-d6d1-45b3-9ddf-e237cef099ea
📒 Files selected for processing (4)
ovos_workshop/resource_files.pyovos_workshop/skills/converse.pyovos_workshop/skills/ovos.pypyproject.toml
DialogFile.render / IntentFile.render now raise FileNotFoundError when the resource file is missing or empty, instead of silently returning the resource name with dots replaced by spaces. OVOSSkill.render_dialog stays polymorphic: speak_dialog / get_response accept either a dialog key or a literal utterance, so a name with no matching .dialog file is caught and returned verbatim as the utterance. The silent dialog-name humanization is gone — a genuinely missing file is now an error at the layer that owns the file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mark the entire resource_files module as deprecated. It predates the OVOS formal specifications and is considered overengineered. Everything stays fully functional for downstream consumers; nothing is removed. All public names (ResourceType, ResourceFile and subclasses, SkillResources, CoreResources, UserResources, RegexExtractor, locate_base_directories, locate_lang_directories, find_resource) now carry the @deprecated decorator plus an inner warnings.warn and a .. deprecated:: docstring notice. Messages point downstream to the ovos-spec-tools replacements where they exist (LocaleResources, render/DialogRenderer, expand, closest_lang/ standardize_lang); legacy types with no spec replacement (.qml, .json, .list, .value, .rx, .word, .template) are flagged as not part of the OVOS formal specifications. To avoid the framework spamming deprecation warnings at itself (these classes are built per-skill, per-language on hot paths), a _caller_is_internal stack guard suppresses both the log notice and the DeprecationWarning when the caller is ovos_workshop's own code; the warnings remain visible to downstream callers. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ovos_workshop/skills/ovos.py (1)
1780-1782:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect the misleading comment about fallback behavior.
The comment states that a missing
.dialogfile "falls back to the name with dots replaced by spaces", but the actual implementation at lines 461-465 returnsnameverbatim without any transformation. Per the library context, literal utterances are spoken without dot-to-space fallback.📝 Proposed fix to correct the comment
def on_fail_default(utterance): fail_data = data.copy() fail_data['utterance'] = utterance - # render_dialog renders a phrase via ovos_spec_tools.render; a - # missing .dialog file falls back to the name with dots replaced - # by spaces. + # render_dialog renders a phrase via ovos_spec_tools.render; if + # no .dialog file is found, the name is returned verbatim as a + # literal utterance. if on_fail: return self.render_dialog(on_fail, fail_data)🤖 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_workshop/skills/ovos.py` around lines 1780 - 1782, Update the misleading comment for render_dialog to reflect actual behavior: instead of saying a missing .dialog "falls back to the name with dots replaced by spaces", change the comment to state that when a .dialog file is missing render_dialog returns/speaks the literal name verbatim (no dot->space transformation). Reference the render_dialog function and the code path that returns name so the comment matches implementation.ovos_workshop/resource_files.py (1)
831-846:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
SkillResources.dialog_rendererthe same way as other deprecated APIs inresource_files.py.
SkillResources.dialog_rendererbypasses_deprecated_public()/_caller_is_internal()and always emitsDeprecationWarningvia an unconditionalwarnings.warn(...). SinceOVOSSkill.dialog_rendererdelegates toself.resources.dialog_renderer, framework-owned access to the shim can still spam deprecation noise (thoughOVOSSkill.dialog_rendereritself is also unguarded).Proposed fix
`@property` - `@deprecated`("dialog_renderer is deprecated; dialog rendering is handled by " - "ovos_spec_tools.render via SkillResources.render_dialog. " - "This compat shim will be removed.", _REMOVAL_VERSION) + `@_deprecated_public`( + "dialog_renderer is deprecated; dialog rendering is handled by " + "ovos_spec_tools.render via SkillResources.render_dialog. " + "This compat shim will be removed.") def dialog_renderer(self) -> MustacheDialogRenderer: """ Get a dialog renderer object for these resources @@ - warnings.warn( - "dialog_renderer is deprecated; use SkillResources.render_dialog", - DeprecationWarning, stacklevel=3) if not self._dialog_renderer: self._load_dialog_renderer() return self._dialog_renderer🤖 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_workshop/resource_files.py` around lines 831 - 846, The dialog_renderer property currently always emits a DeprecationWarning; change it to follow the same guards used by other deprecated APIs by only issuing the warning when the public deprecation guard is active and the caller is not internal—i.e., call the same helpers used elsewhere (e.g., _deprecated_public() and _caller_is_internal()) inside the dialog_renderer getter and only run warnings.warn(...) when _deprecated_public() is true and _caller_is_internal() is false, leaving the rest of the lazy MustacheDialogRenderer construction intact.
🧹 Nitpick comments (1)
ovos_workshop/skills/ovos.py (1)
461-465: ⚡ Quick winConsider logging when dialog file is not found.
When a dialog key doesn't match a
.dialogfile, the method silently returnsnameverbatim, which means missing dialog files may go unnoticed. Ifnamewas intended as a dialog key (e.g.,"skill.error"), it will be spoken literally, which could be confusing.📝 Proposed enhancement to add debug logging
try: return self.resources.render_dialog(name, data) except FileNotFoundError: + self.log.debug(f"No .dialog file found for '{name}'; treating as literal utterance") # `name` is not a dialog key — treat it as a literal utterance return name🤖 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_workshop/skills/ovos.py` around lines 461 - 465, The FileNotFoundError in the call to self.resources.render_dialog is silently swallowed and returns name verbatim; update the except block to log the missing dialog (e.g., self.log.debug or self.log.warning) including the dialog key (name) and the caught exception before returning name so missing .dialog files are visible in logs; keep the catch for FileNotFoundError and only add the log call referencing self.resources.render_dialog and the local variable name.
🤖 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.
Outside diff comments:
In `@ovos_workshop/resource_files.py`:
- Around line 831-846: The dialog_renderer property currently always emits a
DeprecationWarning; change it to follow the same guards used by other deprecated
APIs by only issuing the warning when the public deprecation guard is active and
the caller is not internal—i.e., call the same helpers used elsewhere (e.g.,
_deprecated_public() and _caller_is_internal()) inside the dialog_renderer
getter and only run warnings.warn(...) when _deprecated_public() is true and
_caller_is_internal() is false, leaving the rest of the lazy
MustacheDialogRenderer construction intact.
In `@ovos_workshop/skills/ovos.py`:
- Around line 1780-1782: Update the misleading comment for render_dialog to
reflect actual behavior: instead of saying a missing .dialog "falls back to the
name with dots replaced by spaces", change the comment to state that when a
.dialog file is missing render_dialog returns/speaks the literal name verbatim
(no dot->space transformation). Reference the render_dialog function and the
code path that returns name so the comment matches implementation.
---
Nitpick comments:
In `@ovos_workshop/skills/ovos.py`:
- Around line 461-465: The FileNotFoundError in the call to
self.resources.render_dialog is silently swallowed and returns name verbatim;
update the except block to log the missing dialog (e.g., self.log.debug or
self.log.warning) including the dialog key (name) and the caught exception
before returning name so missing .dialog files are visible in logs; keep the
catch for FileNotFoundError and only add the log call referencing
self.resources.render_dialog and the local variable name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49a31374-6580-4ce1-bd4d-668bb88b7356
📒 Files selected for processing (2)
ovos_workshop/resource_files.pyovos_workshop/skills/ovos.py
1 participant
Wave 2 of the OVOS-wide migration onto ovos-spec-tools (OpenVoiceOS/architecture#7).
Changes
resource_files.py—locate_lang_directories:langcodes.tag_distance→
ovos_spec_tools.lang_distance(same<10threshold).VocabularyFile/IntentFileloading:ovos_utils.bracket_expansion.expand_template→sorted(ovos_spec_tools.expand(...)).skills/converse.py—_get_closest_lang:langcodes.closest_match+standardize_lang_tag+ a manual threshold →ovos_spec_tools.closest_lang(standardizes and thresholds internally).
pyproject.toml— addsovos-spec-tools>=0.0.1a2.All migrated symbols were internal imports — not re-exported — so no public
API changed and no deprecation shims were needed.
Behaviour changes — conformance with OVOS-INTENT-1
expand_templateleft a double space whenan optional segment was dropped (
hello [there] world→hello␣␣world);the conformant expander yields
hello world.MalformedTemplateinstead of beingsilently expanded leniently (no fallback added).
Deferred — follow-up (deliberately not done)
MustacheDialogRenderer/load_dialogsstill come fromovos_utils.dialog.MustacheDialogRendereris part of the publicSkillResourcesAPI (aproperty type and constructor argument); swapping it onto
ovos_spec_tools.DialogRendererrisks the skill resource API and is left asa separate change.
standardize_lang_taginconverse.py(message-language normalization,distinct from language matching) is left untouched.
Verification
test_resource_files.pypasses (28 tests). The migration touches three files(
resource_files.py,converse.py,pyproject.toml).🤖 Generated with Claude Code
Summary by CodeRabbit
Deprecations
Improvements
Behavior changes