fix:usage without skills installed #555
Conversation
WalkthroughThe changes in Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
companion to OpenVoiceOS/ovos-config#166 includes #555
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
ovos_core/skill_manager.py (3)
119-122: LGTM! Consider adding a debug log for when skills are detected.The addition of the
_detected_installed_skillsattribute and the warning log for when no skills are detected is a good improvement. It will help with debugging, especially in standalone mode.Consider adding a debug log when skills are detected, for consistency:
self._detected_installed_skills = bool(find_skill_plugins()) if not self._detected_installed_skills: LOG.warning("No installed skills detected! if you are running skills in standalone mode ignore this warning," " otherwise you probably want to install skills first!") +else: + LOG.debug(f"Detected {len(find_skill_plugins())} installed skills")
618-620: LGTM! Consistent improvement for offline skill loading.The addition of the
_detected_installed_skillscheck before loading offline skills is consistent with the changes made to other loading methods. This optimization prevents unnecessary processing when no skills are installed.For consistency with the other loading methods, consider updating the log message:
- LOG.info('Loading offline skills...') + LOG.info('Loading skills that do not require network or internet...')
Line range hint
119-620: Great improvements! Consider adding a method to check for installed skills.The addition of the
_detected_installed_skillsattribute and its usage across different loading methods effectively addresses the issue of unnecessary processing when no skills are installed. This change aligns well with the PR objectives and improves the overall efficiency of the SkillManager.To further improve code maintainability and reduce duplication, consider extracting the skill detection logic into a separate method:
def _has_installed_skills(self): if not hasattr(self, '_detected_installed_skills'): self._detected_installed_skills = bool(find_skill_plugins()) if not self._detected_installed_skills: LOG.warning("No installed skills detected! If you are running skills in standalone mode, ignore this warning; " "otherwise, you probably want to install skills first!") else: LOG.debug(f"Detected {len(find_skill_plugins())} installed skills") return self._detected_installed_skillsThen, you can use this method in the loading functions:
if self._has_installed_skills(): LOG.info('Loading skills that ...') self._load_new_skills(...)This approach would centralize the skill detection logic and make it easier to modify or extend in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- ovos_core/skill_manager.py (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
ovos_core/skill_manager.py (2)
567-569: LGTM! Good optimization for network-dependent skill loading.The addition of the
_detected_installed_skillscheck before loading network-dependent skills is a good optimization. It prevents unnecessary processing when no skills are installed.
574-576: LGTM! Consistent improvement for internet-dependent skill loading.The addition of the
_detected_installed_skillscheck before loading internet-dependent skills is consistent with the changes made to_load_on_network. This optimization prevents unnecessary processing when no skills are installed.
in setups without skills installed the trained event was never emitted, causing OVOS to hang forever and never reporting ready noticed in docker when running skills in standalone mode (individual container per skill) adds a check to see if any skills are installed, to make logs more informative
JarbasAl
force-pushed
the
fix/noskills
branch
from
ce178c0
to
0d12a4d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #555 +/- ##
==========================================
- Coverage 75.33% 74.82% -0.52%
==========================================
Files 15 15
Lines 3094 3043 -51
==========================================
- Hits 2331 2277 -54
- Misses 763 766 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ovos_core/skill_manager.py (4 hunks)
- test/unittests/skills/test_skill_manager.py (0 hunks)
💤 Files with no reviewable changes (1)
- test/unittests/skills/test_skill_manager.py
🧰 Additional context used
🔇 Additional comments (4)
ovos_core/skill_manager.py (4)
200-200: Registration of device readiness handler is appropriateThe addition of the event handler
handle_check_device_readinessimproves the system's ability to verify device readiness upon skill initialization. This change enhances the startup process.
567-569: Conditionally load network-dependent skillsAdding the condition
if self._detected_installed_skillsbefore loading network-dependent skills ensures that the system attempts to load these skills only if they are installed. This prevents unnecessary operations and potential errors when no skills are present.
574-576: Conditionally load internet-dependent skillsThe inclusion of
if self._detected_installed_skillsbefore loading skills that require internet access is appropriate. It ensures that the system efficiently handles scenarios where no skills are installed by avoiding redundant loading attempts.
618-620: Conditionally load skills on startupThe check for
self._detected_installed_skillsbefore loading skills on startup is a good addition. It prevents the system from attempting to load skills when none are installed, improving startup efficiency and avoiding unnecessary processing.
companion to OpenVoiceOS/ovos-config#166 includes #555
in setups without skills installed the trained event was never emitted, causing OVOS to hang forever and never reporting ready, but we have no need to wait here, it was only there to delay padatious training to after all skills loaded, but this no longer happens at once on boot in OVOS like it did in mycroft
noticed in docker when running skills in standalone mode (individual container per skill)
adds a check to see if any skills are installed, to make logs more informative
relates to #554
Summary by CodeRabbit
handle_initial_trainingmethod for improved clarity.