fix:delayed_padatious_training #627
Conversation
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces modifications to two main components: the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
ovos_core/intent_services/__init__.py (1)
95-96: Consider moving the configuration to ovos-config.The
instant_trainconfiguration is hardcoded here with a TODO comment indicating it should be moved toovos-config. This change aligns with the delayed training implementation in the skill manager.Would you like me to help create a PR to add this configuration to
ovos-config?ovos_core/skill_manager.py (1)
513-518: LGTM: Added delayed padatious training.The implementation efficiently triggers padatious training only when new skills are loaded, waiting for the training to complete before proceeding.
Consider adding a timeout to
wait_for_responseto prevent potential deadlocks if the training response is not received.- self.bus.wait_for_response(Message("padatious:train"), - "mycroft.skills.trained") + self.bus.wait_for_response(Message("padatious:train"), + "mycroft.skills.trained", + timeout=300) # 5 minutes timeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ovos_core/intent_services/__init__.py(1 hunks)ovos_core/skill_manager.py(4 hunks)
🔇 Additional comments (2)
ovos_core/skill_manager.py (2)
291-291: LGTM: Added tracking of newly loaded skills.
The addition of the loaded_new flag helps track when new skills are loaded, which is used to determine if intent training is needed.
Also applies to: 313-314
479-479: LGTM: Added tracking of newly loaded skills.
The addition of the loaded_new flag helps track when new skills are loaded from both plugin skills and local skills.
Also applies to: 511-512
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #627 +/- ##
==========================================
- Coverage 75.33% 72.33% -3.01%
==========================================
Files 15 15
Lines 3094 1569 -1525
==========================================
- Hits 2331 1135 -1196
+ Misses 763 434 -329
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
🧹 Outside diff range and nitpick comments (1)
ovos_core/skill_manager.py (1)
Line range hint
728-766: Consider cleaning up deprecated methods in a future PR.The file contains several deprecated methods at the end that could be removed in a future PR to improve code maintainability:
load_priorityis_device_readyhandle_check_device_readinesscheck_services_ready
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_core/skill_manager.py(7 hunks)
🔇 Additional comments (2)
ovos_core/skill_manager.py (2)
Line range hint 291-315: LGTM! Well-structured tracking of newly loaded skills.
The addition of the loaded_new flag effectively tracks when new skills are loaded, which is then used to determine if intent training is needed. The implementation is clean and maintains the existing functionality while adding this new capability.
Line range hint 334-350: LGTM! Good enhancement to plugin skill loader initialization.
The addition of the optional skill_class parameter provides more flexibility in skill loading while maintaining backward compatibility. This is a clean implementation that allows for direct skill class assignment when available.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
ovos_core/skill_manager.py (2)
520-526: Consider making the Padatious training timeout configurableThe timeout for the Padatious training response is currently hardcoded to 60 seconds. Consider making this timeout configurable through the settings to allow flexibility based on different deployment environments and training times.
Line range hint
334-350: Consider passingskill_classtoPluginSkillLoaderconstructorCurrently,
skill_classis assigned to theloaderafter initialization. If possible, consider modifying thePluginSkillLoaderconstructor to acceptskill_classas a parameter. This would make the initialization more cohesive and ensure that any initialization logic depending onskill_classis properly handled.ovos_core/intent_services/__init__.py (2)
96-97: Address the TODO: Read 'instant_train' from configurationYou have added a TODO to read
'instant_train'from the configuration once the default value is added toovos-config. Considering implementing this now to enhance configurability and avoid hardcoding values in the code.Would you like assistance in updating the code to read
'instant_train'from the configuration?
99-99: Remove unnecessary f-string without placeholdersThe log message at line 99 uses an f-string without any placeholders. Consider removing the
fprefix to avoid unnecessary string processing.Apply this diff to fix the issue:
- LOG.error(f'Failed to create padatious intent handlers, padatious not installed') + LOG.error('Failed to create padatious intent handlers, padatious not installed')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ovos_core/intent_services/__init__.py(4 hunks)ovos_core/skill_manager.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
ovos_core/intent_services/__init__.py
100-100: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (1)
ovos_core/skill_manager.py (1)
515-530: Impressive implementation of timeout and error handling for Padatious training
The addition of timeout and error handling around the Padatious training request ensures that the system does not hang indefinitely and properly logs any issues during training.
JarbasAl
changed the title
JarbasAl
force-pushed
the
delay_padatious
branch
from
0fb9caa to
fad1b76
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
ovos_core/intent_services/__init__.py (3)
96-97: Consider making 'instant_train' configurable via ovos-configCurrently,
instant_trainis hardcoded toFalse, which limits flexibility. Reading this value from the configuration allows for dynamic adjustments without code changes.Apply this diff to read
instant_trainfrom the configuration:# TODO - read from config once default value has been added to ovos-config -self.config["padatious"]["instant_train"] = False +self.config["padatious"]["instant_train"] = self.config["padatious"].get("instant_train", False)
100-100: Remove unnecessary 'f' prefix fromLOG.errormessageThe f-string in the
LOG.errorcall does not contain any placeholders, so thefprefix is unnecessary.Apply this diff to remove the unnecessary
fprefix:except ImportError: - LOG.error(f'Failed to create padatious intent handlers, padatious not installed') + LOG.error('Failed to create padatious intent handlers, padatious not installed')🧰 Tools
🪛 Ruff (0.8.0)
100-100: f-string without any placeholders
Remove extraneous
fprefix(F541)
108-109: Improve log message formatting for clarityThe log message could be more grammatically correct and clearer.
Apply this diff to enhance the log message:
elif "disable_padacioso" not in self.config: LOG.debug("Padacioso pipeline is disabled, only padatious is loaded. " - "set 'disable_padacioso': false in mycroft.conf if you want it to load alongside padatious") + "Set 'disable_padacioso': false in mycroft.conf if you want it to load alongside padatious.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ovos_core/intent_services/__init__.py(4 hunks)ovos_core/skill_manager.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ovos_core/skill_manager.py
🧰 Additional context used
🪛 Ruff (0.8.0)
ovos_core/intent_services/__init__.py
100-100: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (1)
ovos_core/intent_services/__init__.py (1)
211-217:
Add a check before accessing attributes of padatious_matcher
When updating the matchers dictionary, ensure that padatious_matcher is not None to prevent potential AttributeError.
Apply this diff to include a safety check:
if self._padatious_service is not None:
matchers.update({
"padatious_high": padatious_matcher.match_high,
"padatious_medium": padatious_matcher.match_medium,
"padatious_low": padatious_matcher.match_low,
})
+else:
+ LOG.debug("Padatious matcher is not available; skipping padatious matchers.")Likely invalid or redundant comment.
JarbasAl
force-pushed
the
delay_padatious
branch
from
fad1b76 to
503dd83
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ovos_core/intent_services/__init__.py (1)
96-97: Consider documenting theinstant_trainconfiguration option.The new
instant_trainconfiguration option for padatious service is introduced with a default value ofFalse. While the implementation is correct, it would be beneficial to add documentation explaining its purpose and impact.Consider adding a comment like:
if "instant_train" not in self.config["padatious"]: + # Controls whether padatious should train immediately when new intents are registered self.config["padatious"]["instant_train"] = False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
ovos_core/intent_services/__init__.py(2 hunks)ovos_core/skill_manager.py(5 hunks)requirements/lgpl.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ovos_core/skill_manager.py
🔇 Additional comments (3)
requirements/lgpl.txt (1)
1-1: LGTM! Version constraint update aligns with new features.
The version bump for ovos_padatious to >=1.1.0 is necessary to support the new instant_train configuration option while maintaining compatibility with existing features.
ovos_core/intent_services/__init__.py (2)
19-21: LGTM! Import organization improved.
The imports for CommonQAService and PadaciosoService are now placed with other pipeline-related imports, improving code organization.
96-97: Verify dependency chain for instant training feature.
The new instant_train configuration option requires ovos_padatious>=1.1.0. Let's verify this dependency is properly handled across the codebase.
✅ Verification successful
Dependencies are properly handled for instant_train feature
The verification shows that:
- The required dependency
ovos_padatious>=1.1.0is correctly specified inrequirements/lgpl.txt - The feature is properly guarded with a default value of
False - The imports from
ovos_padatiousare consistent across the codebase - The feature is only used within the padatious service initialization, which has proper error handling for missing dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the minimum version requirement is consistently defined
rg -l "ovos_padatious.*1\.1\.0"
# Check for any potential version conflicts
rg "ovos_padatious.*[0-9]+\.[0-9]+\.[0-9]+"
Length of output: 153
Script:
#!/bin/bash
# Check if instant_train is used in the codebase and how it's handled
rg -A 5 "instant_train"
# Check for any padatious imports or usage
ast-grep --pattern 'from ovos_padatious import $_'
ast-grep --pattern 'import ovos_padatious'
# Check setup.py for dependencies
cat setup.py
Length of output: 6113
JarbasAl
force-pushed
the
delay_padatious
branch
from
21a5f7a to
cb50d5a
Compare
needs OpenVoiceOS/ovos-padatious-pipeline-plugin#29
Summary by CodeRabbit
New Features
instant_trainconfiguration option set toFalse.ovos_padatiousto>=1.1.0, <2.0.0.Bug Fixes