deprecate ready setting #554
Conversation
|
Why deprecate this? It is used in many skills and integrations to determine when core services are ready for normal usage |
edited original post to include some extra info about this |
JarbasAl
force-pushed
the
deprecate_ready
branch
2 times, most recently
from
f3a731c
to
2f2425c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #554 +/- ##
==========================================
+ Coverage 75.33% 76.43% +1.09%
==========================================
Files 15 15
Lines 3094 1553 -1541
==========================================
- Hits 2331 1187 -1144
+ Misses 763 366 -397
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
WalkthroughThe changes in this pull request enhance the skill management functionality within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🧰 Additional context used🔇 Additional comments (4)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
ovos_core/skill_manager.py (1)
372-372: Correct typo in commentIn the comment on line 372, "dont" should be "don't" to properly use the contraction.
Apply this diff to correct the typo:
- # trigger a sync so we dont need to wait for the plugin to volunteer info + # trigger a sync so we don't need to wait for the plugin to volunteer info
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- ovos_core/skill_manager.py (2 hunks)
- test/unittests/skills/test_manager.py (0 hunks)
- test/unittests/skills/test_skill_manager.py (1 hunks)
💤 Files with no reviewable changes (1)
- test/unittests/skills/test_manager.py
🧰 Additional context used
🪛 Ruff
ovos_core/skill_manager.py
25-25:
time.sleepimported but unusedRemove unused import:
time.sleep(F401)
🔇 Additional comments (2)
test/unittests/skills/test_skill_manager.py (1)
Line range hint
1-291: Ensure comprehensive testing of SkillManager changesWhile the change in this file is minimal, it represents a significant shift in how skill initialization is handled. The removal of the 'mycroft.skills.initialized' event from the expected handlers in the
test_instantiatemethod aligns with the PR objectives.To ensure the changes to SkillManager are thoroughly tested:
- Review and update other test methods in this file if necessary to reflect any changes in SkillManager behavior.
- Consider adding new test cases to cover the updated initialization process and any new events or messages that replace 'mycroft.skills.initialized'.
- Run the full test suite and verify that all tests pass with the new changes.
Run the following script to check for any failing tests:
#!/bin/bash # Run the test suite for SkillManager python -m unittest test/unittests/skills/test_skill_manager.pyAs this change affects a core component of the system, consider performing integration tests to ensure that the new initialization process works correctly with other parts of the OpenVoiceOS framework.
ovos_core/skill_manager.py (1)
379-381: Emitmycroft.readyafter core initializationEmitting the
mycroft.readymessage after core initialization aligns with the updated definition that the core is ready to register intents. The additional log message enhances clarity for system readiness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
ovos_core/skill_manager.py (2)
Line range hint
481-534: Comprehensive improvements to skill loading processThe
_load_new_skillsmethod has undergone significant enhancements:
- Integration of both plugin skills and directory-based skills into a single loading process.
- Implementation of logic to replace plugin skills with local versions, providing greater flexibility in skill management.
- Addition of checks for network, internet, and GUI requirements before loading skills, ensuring that skills are only loaded when their dependencies are met.
These changes result in a more robust and flexible skill loading process. However, the complexity of this method has increased, which may make it more difficult to maintain or debug in the future.
Consider breaking down this method into smaller, more focused methods to improve readability and maintainability. For example:
def _load_new_skills(self, network=None, internet=None, gui=None): self._update_connection_status(network, internet, gui) self._load_plugin_skills() self._load_directory_skills() def _load_directory_skills(self): for skill_dir in self._get_skill_directories(): self._handle_skill_directory(skill_dir) def _handle_skill_directory(self, skill_dir): # Logic for handling a single skill directory ...This refactoring would make the code more modular and easier to understand.
Line range hint
1-724: Overall improvements to skill management systemThe changes made to the
SkillManagerclass represent a significant evolution in the skill management system:
- Enhanced flexibility in handling both plugin skills and directory-based skills.
- Improved security through the option of isolated bus connections for skills.
- More dynamic skill loading based on network, internet, and GUI availability.
- Better logging and observability of the skill loading process.
These improvements should result in a more robust, secure, and efficient skill management system. However, the increased complexity of some methods (particularly
_load_new_skills) may require additional attention to maintainability in the future.Consider the following architectural improvements for future iterations:
- Further modularization of complex methods to enhance maintainability.
- Implementation of a more event-driven architecture for skill loading and unloading, which could improve responsiveness to system state changes.
- Development of a comprehensive testing suite to ensure the reliability of the new skill management features, especially around the dynamic loading and unloading of skills based on system conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- ovos_core/skill_manager.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
ovos_core/skill_manager.py (4)
23-26: Update imports to reflect new skill loading mechanismThe changes to the import statements reflect a shift in the skill loading mechanism. The addition of
SKILL_MAIN_MODULE,SkillLoader, andPluginSkillLoaderfromovos_workshop.skill_launcherindicates a more modular approach to skill loading.
Line range hint
383-397: Enhanced network and internet-dependent skill loadingThe modifications to
_load_on_networkand_load_on_internetmethods introduce two key improvements:
- The check for
self._detected_installed_skillsprevents unnecessary processing when no skills are installed, potentially improving performance.- The addition of log messages provides better visibility into the skill loading process, which can aid in debugging and monitoring.
These changes enhance both the efficiency and observability of the skill loading process.
Line range hint
467-480: New method for creating isolated skill loadersThe introduction of
_get_plugin_skill_loadermethod is a significant improvement in skill management:
- It allows for the creation of isolated MessageBusClient instances per skill, enhancing security by preventing skills from manipulating each other's bus connections.
- The use of a configuration option (
shared_connection) provides flexibility in how bus connections are managed.This change addresses potential security concerns, as mentioned in the comment about the BusBricker skill. However, be aware that creating separate connections for each skill may have performance implications if many skills are loaded.
To verify the impact and usage of this new method, run the following script:
372-381: Improved skill loading process and readiness signalingThe changes in the
runmethod represent a significant improvement in the skill loading process:
- Removal of the
wait_for_internetlogic allows for more dynamic skill loading based on network status.- The addition of
_sync_skill_loading_statesuggests a more proactive approach to managing skill states based on system conditions.- Earlier emission of the
mycroft.readymessage may improve system responsiveness.These changes should result in a more efficient and responsive skill loading process. However, ensure that all components relying on the
mycroft.readymessage are prepared for its earlier emission.To verify the impact of these changes, run the following script:
✅ Verification successful
Action Needed: Correct the shell script to properly search for
mycroft.readyin Python files.To address the issue with the previous script, please run the following corrected script:
Action Needed: Run the updated shell script to search for
mycroft.readyin Python files.Please execute the following corrected script:
Verified: No external components are affected by the earlier emission of
mycroft.ready.All usages of
mycroft.readyare confined toovos_core/skill_manager.py, ensuring that the changes do not impact other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for components that might be affected by earlier 'mycroft.ready' emission rg -i "mycroft.ready" --type python -C 5Length of output: 158
Script:
#!/bin/bash # Check for components that might be affected by earlier 'mycroft.ready' emission rg -i "mycroft.ready" -t python -C 5Length of output: 67
Script:
#!/bin/bash # Check for components that might be affected by earlier 'mycroft.ready' emission rg -i "mycroft.ready" -g "*.py" -C 5Length of output: 708
JarbasAl
requested review from
goldyfruit,
j1nx,
mikejgray,
NeonDaniel,
builderjer and
a team
JarbasAl
force-pushed
the
deprecate_ready
branch
2 times, most recently
from
377f7d0
to
6d9ff11
Compare
I have used it in Neon without changes for probably more than a year now; changes to default configuration could be an alternative solution compared to removing configuration support completely. If the meaning of "mycroft.ready" is changing here, I would ask that this emit something less ambiguous (maybe "mycroft.intent_service.ready", or implement ProcessStatus directly for consistency with other modules that report state. |
extracted from OpenVoiceOS/ovos-core#554
|
moved message to skill OpenVoiceOS/skill-ovos-boot-finished#15 since this is in requirements this is no longer a breaking change, just an optional component |
extracted from OpenVoiceOS/ovos-core#554
JarbasAl
force-pushed
the
deprecate_ready
branch
from
fa1d498
to
b2a0518
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
ovos_core/skill_manager.py (5)
Line range hint
170-186: Improved network and connection handlingThe changes to network and connection handling are well-implemented:
- The
_sync_skill_loading_statemethod now usesovos.PHAL.internet_checkfor more reliable connectivity checks.- New methods for handling network, internet, and GUI connections provide granular control over skill loading.
These improvements enhance the robustness of the skill management process based on connectivity status.
Consider adding logging statements in the new connection handling methods to improve debugging capabilities.
Also applies to: 275-318
396-416: Improved skill loading processThe modifications to the skill loading process are well-implemented:
- The
runmethod has been simplified, removing the deprecatedload_prioritycall.- The
_load_new_skillsmethod now handles plugin skills and local skill replacements effectively.- New methods for loading skills based on network, internet, and GUI availability provide more granular control.
These changes enhance the flexibility and efficiency of the skill loading process.
Consider extracting the common logic in
_load_on_network,_load_on_internet, and_load_on_startupinto a separate method to reduce code duplication. For example:def _load_skills_based_on_connectivity(self, network=False, internet=False): if self._detected_installed_skills: LOG.info(f'Loading skills that require {"internet and " if internet else ""}{"network" if network else "offline"}...') self._load_new_skills(network=network, internet=internet) if internet: self._internet_loaded.set() if network: self._network_loaded.set() def _load_on_network(self): self._load_skills_based_on_connectivity(network=True) def _load_on_internet(self): self._load_skills_based_on_connectivity(network=True, internet=True) def _load_on_startup(self): self._load_skills_based_on_connectivity()Also applies to: 486-566
Line range hint
605-639: Consider making skill directory paths configurableThe
_get_skill_directoriesmethod uses a hardcoded path "/opt/mycroft/skills". To improve flexibility, consider making this path configurable.Replace the hardcoded path with a configuration option:
def _get_skill_directories(self): skillmap = {} valid_skill_roots = self.config.get('skills', {}).get('paths', ["/opt/mycroft/skills"]) + get_skill_directories() # ... rest of the method
Line range hint
625-628: Improve skill directory validationThe TODO comment suggests a need for a better way to validate skill directories:
# TODO: all python packages must have __init__.py! Better way? # check if folder is a skill (must have __init__.py) if SKILL_MAIN_MODULE in os.listdir(skill_dir):Consider implementing a more robust method to validate skill directories, such as checking for a specific skill manifest file or structure.
Line range hint
679-694: Handle external skills in send_skill_listThe
send_skill_listmethod doesn't account for external skills or OVOSAbstractApp/Hivemind skills.To provide a complete skill list, implement handling for these types of skills. This may require adding a new method to discover and include external skills in the list.
🛑 Comments failed to post (1)
test/unittests/test_skill_manager.py (1)
121-121: 💡 Codebase verification
'mycroft.skills.initialized' event is still in use
The
'mycroft.skills.initialized'event is still being emitted inovos_core/skill_manager.py. Ensure that this event is either fully deprecated and removed from all relevant parts of the codebase or that its continued usage is intentional and properly documented.
ovos_core/skill_manager.py:self.bus.emit(Message('mycroft.skills.initialized'))🔗 Analysis chain
Verify the removal of 'mycroft.skills.initialized' event
The removal of the 'mycroft.skills.initialized' event from the expected results aligns with the PR objective to redefine the usage of the
mycroft.readymessage. This change indicates that the SkillManager no longer emits or listens for this event.To ensure this change doesn't introduce unintended consequences:
- Verify that no other components in the system rely on the 'mycroft.skills.initialized' event.
- Update documentation to reflect this change in behavior.
- Consider adding a deprecation warning if any skills or integrations might still be using this event.
The change is consistent with the PR's goal of changing how readiness is determined in the system. However, ensure that this modification is communicated clearly to developers who might be relying on this event in their skills or integrations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of 'mycroft.skills.initialized' in the codebase rg "mycroft\.skills\.initialized" --type pythonLength of output: 226
Script:
#!/bin/bash # Search for any remaining usage of 'mycroft.skills.initialized' in the codebase rg "mycroft\.skills\.initialized" --type pyLength of output: 213
JarbasAl
force-pushed
the
deprecate_ready
branch
from
d9e3463
to
a8e6fdc
Compare
companion to OpenVoiceOS/ovos-config#166
NOTES:
mycroft.readymessage was emitted almost randomly and was not dependendable onmycroft.readyis moved to finished booting skill and no longer blocks loading of ovos-coremycroft.readyin combination with ProcessUtils linked above to determine when to loadtests moved to other repos:
Summary by CodeRabbit
New Features
Bug Fixes
Tests