fix: runtime requirements #628
Conversation
runtime requirements class property was not workign properly due to the class not being passed to the skill loader all skills were considered to need internet, offline skills were not loading early as intended!
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #628 +/- ##
==========================================
- Coverage 75.33% 72.69% -2.65%
==========================================
Files 15 15
Lines 3094 1553 -1541
==========================================
- Hits 2331 1129 -1202
+ Misses 763 424 -339
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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 (2)
ovos_core/skill_manager.py (2)
305-306: Fix inconsistent indentationThe indentation uses a mix of spaces and tabs. Maintain consistent indentation throughout the codebase.
- skill_loader = self._get_plugin_skill_loader(skill_id, init_bus=False, - skill_class=plug) + skill_loader = self._get_plugin_skill_loader( + skill_id, + init_bus=False, + skill_class=plug + )
Line range hint
331-347: Update docstring to include new parameterThe docstring should document the new
skill_classparameter.def _get_plugin_skill_loader(self, skill_id, init_bus=True, skill_class=None): """Get a plugin skill loader. Args: skill_id (str): ID of the skill. init_bus (bool): Whether to initialize the internal skill bus. + skill_class: The skill class to be loaded. Returns: PluginSkillLoader: Plugin skill loader instance. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_core/skill_manager.py(4 hunks)
🔇 Additional comments (2)
ovos_core/skill_manager.py (2)
359-359: LGTM!
The change correctly passes the skill class to the loader, completing the fix for runtime requirements.
Line range hint 305-359: Verify impact on existing skills
The changes to runtime requirements handling might affect how existing skills are loaded. Let's verify that no skills are incorrectly categorized.
✅ Verification successful
Runtime requirements implementation verified as correct
The code changes are properly implemented and the impact on existing skills is well-handled. The implementation:
- Correctly checks runtime requirements before loading skills
- Properly handles network/internet dependencies during skill loading
- Includes appropriate unload behavior when network/internet becomes unavailable
- Maintains consistent behavior between plugin skills and directory-based skills
The changes are safe and maintain the expected behavior for both new and existing skills.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for skills that might be affected by the runtime requirements change
# Find all skill plugins and their requirements
echo "Checking skill plugins for runtime requirements..."
ast-grep --pattern 'class $SKILL_CLASS {
$$$
@property
def runtime_requirements(self):
$$$
}'
# Check for any skills reporting internet requirements
echo "Checking for skills with internet requirements..."
rg -l "requires_internet" -g "*.py"
# Check for any skills reporting network requirements
echo "Checking for skills with network requirements..."
rg -l "requires_network" -g "*.py"
Length of output: 557
Script:
#!/bin/bash
# Let's examine the actual implementation of runtime requirements in skill_manager.py
echo "Checking runtime requirements implementation..."
rg "runtime_requirements" -A 10 -B 10 ovos_core/skill_manager.py
# Let's also check for any test files that might validate this behavior
echo "Checking test files for runtime requirements validation..."
fd "test.*\.py" --exec rg "runtime_requirements|requires_internet|requires_network" {}
Length of output: 4869
runtime requirements class property was not workign properly due to the class not being passed to the skill loader
all skills were considered to need internet, offline skills were not loading early as intended!
Summary by CodeRabbit
New Features
Bug Fixes