chore: kill queued deprecation-warning hot-path reads - #879
Conversation
… in hot path > 🤖 Auto-generated by Claude Fable 5 (claude-fable-5) via Claude Code — NOT human-reviewed. Verify before acting. converse_service.py and stop_service.py read the legacy bus-client Session.active_skills and Session.utterance_states views on every converse/stop pass, which now log a deprecation warning on each access. Both are write-through shims over the canonical active_handlers and response_mode fields (verified against the installed ovos-bus-client 2.8.2a1 session.py), so every read site is swapped for the equivalent canonical-field expression with identical output. The ConverseService.active_skills property/setter is left untouched: it is not on the hot path (unreferenced elsewhere in the repo) and rewriting it would change its own public return shape rather than just kill a warning. Full unittest suite (test/unittests) is unchanged: 456 passed, 6 xfailed, 4 warnings before and after. A direct probe against the installed bus-client confirms the swapped expressions trigger zero deprecation log calls where the old accessors triggered two.
Greetings, human! The automated checks are complete. 👾I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthChecking for any cluttered files or folders. 🧹 ✅ All required files present. Latest Version: ✅ 📚 DocsEnsuring the codebase remains stable and healthy. 🛡️ ✅ All required documentation files present. ✅ 🔒 Security (pip-audit)Locking the doors and checking the windows... 🔒 ✅ No known vulnerabilities found (113 packages scanned). 🔎 Type CheckThe data is in, and it's looking interesting! 🧐 ❌ mypy: 300 error(s) found
Errors (showing first 10/300)🏷️ Release PreviewEnsuring the 'Thanks' section includes your name! 🤝 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🌍 Locale BuildI've finished the analysis you requested. 💡 ✅ Locale properly configured (64 files, 17 languages) Locale directories found:
Localization coverage:
pyproject.toml: ✅
Build manifest: ✅ 31 locale files included in package 🔌 Plugin DetectionI've checked the plugin's 'platform' compatibility matrix. 💻 ❌ Plugin Status: ERRORS (1) Plugin Info:
OPM Detection:
Entry Point Validation:
⊘ No Issues:
📊 CoverageEnsuring our safety net is wide enough. 🕸️ Files below 80% coverage (8 files)
Full report: download the 🔨 Build TestsEverything is bolted down and ready to go. 🔩 ✅ All versions pass
🔌 Skill Tests (ovoscope)Scanning the conversational landscape for anomalies. 🕵️ ✅ 13/37 passed ❌ **TestAdaptIntent** — 0/4
❌ **TestCancelIntentMidSentence** — 0/1
❌ **TestConverse** — 0/1
❌ **TestDeactivate** — 2/3
❌ **TestFallback** — 0/1
❌ **TestIntentPipelineRouting** — 0/4
❌ **TestLangDisambiguation** — 0/4
❌ **TestLegacyIntentIdBackCompat** — 0/2
❌ **TestNoSkills** — 0/2
❌ **TestPadatiousIntent** — 0/4
🚌 Bus CoverageEnsuring every intent and event is reached by the test suite. 🎯 🔴 Coverage Summary
📊 Per-Skill Breakdown
🔍 Detailed Message Type Breakdown
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughConverse and stop services now use ChangesSession state migration
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ 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 |
…ll (#881) Standing rule: every repo carries a version-stamped, newest-first log of what changed since the last stable release, reset at each stable. This adds it for ovos-core (since 2.1.1). Filled a docs gap along the way: converse-fallback.md described only the sequential per-skill converse request, not the OVOS-CONVERSE-1 broadcast contest poll added in #863. > 🤖 Auto-generated by Claude Fable 5 (claude-fable-5) via Claude Code — NOT human-reviewed. Verify before acting. > Verified against current source: context-lock fix (#868), _LegacyStopBridge > presence and its removal note, registry-first session fold (#858), > converse broadcast poll (#863), round-correlation guard (#862). #864 and > #879 confirmed open/unmerged via gh pr view before being listed as > pending. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
converse_service.py and stop_service.py read the legacy bus-client Session.active_skills and Session.utterance_states views on every converse/stop pass. Both are now deprecated write-through shims over the canonical active_handlers and response_mode fields, so every hot-path read fired a deprecation log on each access.
I swapped each site for the equivalent canonical-field expression, verified against the installed ovos-bus-client 2.8.2a1 shim implementation to make sure the mapping is exact:
[skill[0] for skill in session.active_skills]->[h["skill_id"] for h in session.active_handlers](stop_service.get_active_skills, converse_service.get_active_skills)len(sess.active_skills)->len(sess.active_handlers)(stop_service global-stop log line)sess.utterance_states.get(skill_id, UtteranceState.INTENT)->UtteranceState.RESPONSE if sess.response_mode and sess.response_mode.get("skill_id") == skill_id else UtteranceState.INTENT(stop_service, two sites)session.utterance_states.get(skill_id, UtteranceState.INTENT) == UtteranceState.INTENT->not (session.response_mode and session.response_mode.get("skill_id") == skill_id)(converse_service._collect_converse_skills)session.utterance_states.get(skill_id, UtteranceState.INTENT) == UtteranceState.RESPONSE->session.response_mode and session.response_mode.get("skill_id") == skill_id(converse_service.match)session.active_skills = [...]read+write in_check_converse_timeout-> rewritten directly againstsession.active_handlers(same dict keys,skill_id/activated_at)Left untouched:
ConverseService.active_skillsproperty/setter (lines ~187-195). It isn't referenced anywhere else in the repo, so it isn't on the hot path, and rewriting it would change its own public return shape (list of pairs vs list of handler dicts) rather than just kill a deprecation warning — that's a different kind of change than what was asked, so I left it alone.Verification: full
test/unittestssuite is unchanged, 456 passed / 6 xfailed / 4 warnings before and after (those 4 warnings are unrelated pre-existingovos_config.localedeprecations, not from this change). A direct probe against the installed bus-client confirms the old accessors fire two deprecation log calls and the new expressions fire zero.Test plan
pytest test/unittests -W default -q— 456 passed, 6 xfailed, 4 warnings, before and afterSummary by CodeRabbit