refactor!: drop old pipeline plugins and deprecated methods #690
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update refactors the intent service infrastructure, centralizing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MessageBus
participant IntentService
participant PipelinePlugin
participant SkillManager
User->>MessageBus: recognizer_loop:utterance
MessageBus->>IntentService: handle_utterance
IntentService->>PipelinePlugin: match utterance (via pipeline)
PipelinePlugin-->>IntentService: IntentHandlerMatch or None
alt Match found
IntentService->>SkillManager: activate skill (if needed)
IntentService->>MessageBus: emit intent message
else No match
IntentService->>MessageBus: complete_intent_failure
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
JarbasAl
force-pushed
the
drop_old_pipelines
branch
from
47ccb37 to
a58d86c
Compare
loads pipeline plugins from config 🎉 no longer tied to adapt/padatious
JarbasAl
force-pushed
the
drop_old_pipelines
branch
from
bd49fb0 to
ee2f4ee
Compare
…ll loading from folder
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Docstrings generation was requested by @JarbasAl. * #690 (comment) The following files were modified: * `ovos_core/intent_services/converse_service.py` * `ovos_core/intent_services/fallback_service.py` * `ovos_core/intent_services/service.py` * `ovos_core/intent_services/stop_service.py` * `ovos_core/skill_manager.py` * `ovos_core/transformers.py` * `test/end2end/test_fallback.py` * `test/end2end/test_helloworld.py` * `test/end2end/test_no_skills.py` * `test/unittests/test_manager.py` * `test/unittests/test_skill_manager.py`
|
Note Generated docstrings for this pull request at #697 |
github-actions
bot
added
breaking
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
test/unittests/test_intent_service.py (1)
1-84: 💡 Verification agent🧩 Analysis chain
Verify test coverage for the refactored IntentService.
The removal of
TestIntentServiceApialigns with the IntentService refactoring. However, ensure that the new implementation has adequate test coverage through the mentioned end-to-end tests.
🏁 Script executed:
#!/bin/bash # Description: Check for test coverage of the new IntentService implementation # Find tests that cover the new IntentService echo "Searching for tests of the new IntentService..." rg -l "IntentService|intent.*service" test/ --type py # Check if end-to-end tests exist as mentioned in the summary echo -e "\nChecking for end-to-end tests..." fd -e py . test/end2end/ --exec head -20 {} \; | grep -E "(fallback|intent|IntentService)" -B2 -A2 # Look for the new service.py module echo -e "\nVerifying new IntentService module exists..." fd "service.py" ovos_core/intent_services/Length of output: 941
Add end-to-end tests for the new IntentService implementation
Our quick audit shows:
- Only one unit test touches ContextManager and language extraction (
test/unittests/test_intent_service.py), but nothing directly exercisingovos_core/intent_services/service.py.- The end-to-end suite under
test/end2end/only contains fallback tests—no scenarios drive the core IntentService flow.Please add dedicated tests that:
- Invoke the new
IntentService(e.g. via a minimal skill or fake bus with sample utterances).- Confirm intent matching, handler dispatch, and session lifecycle end-to-end.
- Live under
test/end2end/(e.g.test/end2end/test_intent_service.py).This will ensure the refactored service is fully exercised.
🧰 Tools
🪛 Ruff (0.11.9)
15-15:
timeimported but unusedRemove unused import:
time(F401)
16-16:
unittestimported but unusedRemove unused import:
unittest(F401)
26-26:
ovos_core.intent_services.IntentServiceimported but unusedRemove unused import:
ovos_core.intent_services.IntentService(F401)
27-27:
ovos_utils.fakebus.FakeBusimported but unusedRemove unused import:
ovos_utils.fakebus.FakeBus(F401)
28-28:
ovos_workshop.intents.IntentBuilderimported but unusedRemove unused import:
ovos_workshop.intents.IntentBuilder(F401)
🧹 Nitpick comments (10)
requirements/mycroft.txt (1)
2-5: Audit core component version alignment
Major bumps forovos_PHAL,ovos-audio[extras],ovos-audio, andovos-gui[extras]require verifying that Mycroft integrations and downstream services still function. Consider consolidating the duplicate audio entries (plain vs. extras) to simplify the requirements file.🧰 Tools
🪛 LanguageTool
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...ed to do ovos_PHAL[extras]>=0.2.9,<1.0.0 ovos-audio[extras]>=1.0.1,<2.0.0 ovos-au...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~5-~5: A punctuation mark might be missing here.
Context: ....1,<2.0.0 ovos-audio>=1.0.1,<2.0.0 ovos-gui[extras]>=1.3.3,<2.0.0 ovos-messagebus>=0.0.7,<1.0.0 ovos-dink...(AI_EN_LECTOR_MISSING_PUNCTUATION)
ovos_core/intent_services/fallback_service.py (2)
76-81: Simplify conditional logic by combining branches.The two conditional branches can be combined using logical or for better readability.
- if opmode == FallbackMode.BLACKLIST and skill_id in \ - self.config.get("fallback_blacklist", []): - return False - elif opmode == FallbackMode.WHITELIST and skill_id not in \ - self.config.get("fallback_whitelist", []): - return False - return True + if (opmode == FallbackMode.BLACKLIST and skill_id in self.config.get("fallback_blacklist", [])) or \ + (opmode == FallbackMode.WHITELIST and skill_id not in self.config.get("fallback_whitelist", [])): + return False + return True🧰 Tools
🪛 Ruff (0.11.9)
76-81: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
🪛 Pylint (3.3.7)
[refactor] 76-81: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
155-155: Rename unused loop variable.The
priovariable is not used within the loop body.- for skill_id, prio in sorted_handlers: + for skill_id, _ in sorted_handlers:🧰 Tools
🪛 Ruff (0.11.9)
155-155: Loop control variable
prionot used within loop bodyRename unused
prioto_prio(B007)
test/unittests/test_intent_service.py (1)
16-16: Remove unused import.The
unittestimport is no longer needed after removing the TestIntentServiceApi class.-import unittest🧰 Tools
🪛 Ruff (0.11.9)
16-16:
unittestimported but unusedRemove unused import:
unittest(F401)
test/end2end/test_fallback.py (3)
14-15: Consider extracting the skill ID to a test fixture or constant.The skill ID is hardcoded here. For better maintainability, consider defining it as a class constant or loading it from a test configuration.
class TestFallback(TestCase): + FALLBACK_SKILL_ID = "ovos-skill-fallback-unknown.openvoiceos" def setUp(self): LOG.set_level("DEBUG") - self.skill_id = "ovos-skill-fallback-unknown.openvoiceos" + self.skill_id = self.FALLBACK_SKILL_ID self.minicroft = get_minicroft([self.skill_id]) # reuse for speed, but beware if skills keeping internal state
34-34: Clarify the comment about routing exception.The comment "for routing tests this is an exception" is unclear. Consider expanding it to explain why
ovos.skills.fallback.pingneeds to keep its original source in this specific test scenario.
54-54: Consider parameterizing the fallback handler name.The handler name "UnknownSkill.handle_fallback" is hardcoded. If the skill's implementation changes, this test will break. Consider making it configurable or deriving it from the skill metadata.
ovos_core/skill_manager.py (2)
349-349: Remove unnecessary f-string prefixes.These strings don't contain any placeholders, so the f-string prefix is not needed.
- Message(f'mycroft.intents.is_ready', + Message('mycroft.intents.is_ready',- LOG.debug(f"pipelines trained and ready to go") + LOG.debug("pipelines trained and ready to go")Also applies to: 454-454
🧰 Tools
🪛 Ruff (0.11.9)
349-349: f-string without any placeholders
Remove extraneous
fprefix(F541)
410-418: Missing implementations for skill unloading methods.Three methods have TODO comments indicating missing implementations for unloading skills when network, internet, or GUI disconnects. This could lead to skills remaining loaded when they shouldn't be.
Would you like me to help implement these methods or create an issue to track this technical debt?
ovos_core/intent_services/service.py (1)
460-462: Use descriptive variable names in list comprehension.The single-letter variable name reduces readability.
- langs += [l for l in get_valid_languages() if l != lang] + langs += [valid_lang for valid_lang in get_valid_languages() if valid_lang != lang]🧰 Tools
🪛 Ruff (0.11.9)
462-462: Ambiguous variable name:
l(E741)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.github/workflows/unit_tests.yml(1 hunks)ovos_core/intent_services/__init__.py(1 hunks)ovos_core/intent_services/adapt_service.py(0 hunks)ovos_core/intent_services/commonqa_service.py(0 hunks)ovos_core/intent_services/converse_service.py(5 hunks)ovos_core/intent_services/fallback_service.py(5 hunks)ovos_core/intent_services/ocp_service.py(0 hunks)ovos_core/intent_services/padacioso_service.py(0 hunks)ovos_core/intent_services/padatious_service.py(0 hunks)ovos_core/intent_services/service.py(1 hunks)ovos_core/intent_services/stop_service.py(8 hunks)ovos_core/skill_manager.py(10 hunks)ovos_core/transformers.py(2 hunks)requirements/lgpl.txt(1 hunks)requirements/mycroft.txt(1 hunks)requirements/plugins.txt(1 hunks)requirements/requirements.txt(1 hunks)requirements/skills-audio.txt(1 hunks)requirements/skills-desktop.txt(1 hunks)requirements/skills-en.txt(1 hunks)requirements/skills-essential.txt(1 hunks)requirements/skills-extra.txt(1 hunks)requirements/skills-gui.txt(1 hunks)requirements/skills-internet.txt(1 hunks)requirements/skills-media.txt(1 hunks)requirements/tests.txt(1 hunks)setup.py(1 hunks)test/end2end/test_fallback.py(1 hunks)test/end2end/test_helloworld.py(6 hunks)test/end2end/test_no_skills.py(2 hunks)test/unittests/test_intent_service.py(1 hunks)test/unittests/test_manager.py(0 hunks)test/unittests/test_skill_manager.py(3 hunks)
💤 Files with no reviewable changes (6)
- ovos_core/intent_services/padacioso_service.py
- ovos_core/intent_services/padatious_service.py
- ovos_core/intent_services/adapt_service.py
- ovos_core/intent_services/commonqa_service.py
- ovos_core/intent_services/ocp_service.py
- test/unittests/test_manager.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
ovos_core/intent_services/__init__.py (1)
ovos_core/intent_services/service.py (1)
IntentService(59-604)
test/end2end/test_no_skills.py (2)
test/end2end/test_helloworld.py (6)
setUp(11-14)setUp(140-143)setUp(268-271)tearDown(16-19)tearDown(145-148)tearDown(273-276)ovos_core/skill_manager.py (1)
stop(539-549)
test/end2end/test_helloworld.py (2)
test/end2end/test_no_skills.py (2)
setUp(11-13)tearDown(15-18)ovos_core/skill_manager.py (1)
stop(539-549)
🪛 LanguageTool
requirements/skills-desktop.txt
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...kill-application-launcher>=0.5.14,<1.0.0 ovos-skill-wallpapers>=1.0.2,<3.0.0 ovos...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
requirements/mycroft.txt
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...ed to do ovos_PHAL[extras]>=0.2.9,<1.0.0 ovos-audio[extras]>=1.0.1,<2.0.0 ovos-au...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~5-~5: A punctuation mark might be missing here.
Context: ....1,<2.0.0 ovos-audio>=1.0.1,<2.0.0 ovos-gui[extras]>=1.3.3,<2.0.0 ovos-messagebus>=0.0.7,<1.0.0 ovos-dink...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
🪛 Ruff (0.11.9)
test/unittests/test_intent_service.py
16-16: unittest imported but unused
Remove unused import: unittest
(F401)
ovos_core/intent_services/__init__.py
1-1: ovos_core.intent_services.service.IntentService imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
ovos_core/skill_manager.py
18-18: threading.Lock imported but unused
Remove unused import: threading.Lock
(F401)
349-349: f-string without any placeholders
Remove extraneous f prefix
(F541)
454-454: f-string without any placeholders
Remove extraneous f prefix
(F541)
ovos_core/intent_services/fallback_service.py
76-81: Combine if branches using logical or operator
Combine if branches
(SIM114)
85-85: Do not perform function call FallbackRange in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
155-155: Loop control variable prio not used within loop body
Rename unused prio to _prio
(B007)
ovos_core/intent_services/stop_service.py
18-18: ovos_utils.log.deprecated imported but unused
Remove unused import: ovos_utils.log.deprecated
(F401)
ovos_core/intent_services/service.py
155-155: Ambiguous variable name: l
(E741)
462-462: Ambiguous variable name: l
(E741)
478-478: Do not use bare except
(E722)
590-590: Do not use bare except
(E722)
595-595: Do not use bare except
(E722)
🪛 Pylint (3.3.7)
ovos_core/intent_services/fallback_service.py
[refactor] 76-81: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
ovos_core/intent_services/service.py
[refactor] 59-59: Too many instance attributes (8/7)
(R0902)
[refactor] 66-66: Too many arguments (9/5)
(R0913)
[refactor] 66-66: Too many positional arguments (9/5)
(R0917)
[refactor] 162-167: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 407-407: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🔇 Additional comments (32)
.github/workflows/unit_tests.yml (2)
44-47: Intentional drop of Python version matrix?
Ensure that switching to a fixed Python 3.11 build fits the project's supported runtime policy. Confirm whether multi-version testing is no longer required or if documentation and compatibility guarantees should be updated accordingly.
44-47: Streamlined CI with single runtime
Locking CI to Python 3.11 reduces complexity and speeds up feedback loops, which aligns with the refactor removing legacy plugins.requirements/skills-gui.txt (1)
1-1: Upgrade ovos-skill-homescreen minimum version
Bumped the minimum required version to>=3.0.3to stay in sync with other skill packages.requirements/skills-desktop.txt (1)
2-2: Bump ovos-skill-application-launcher to 0.5.14
Updated the minimum version to ensure compatibility with the refactored intent service infrastructure.🧰 Tools
🪛 LanguageTool
[uncategorized] ~2-~2: A comma might be missing here.
Context: ...kill-application-launcher>=0.5.14,<1.0.0 ovos-skill-wallpapers>=1.0.2,<3.0.0 ovos...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
requirements/skills-en.txt (2)
3-3: Add translation placeholder comment
This clearly marks which skills need translation before moving toskill-extras.txt.
4-4: Include ovos-skill-days-in-history
Added the skill with version constraint>=0.3.11,<1.0.0, matching its removal fromskills-extra.txt.requirements/skills-audio.txt (1)
6-7: Update ovos-skill-volume and ovos-skill-naptime versions
Bumped to newer minimum versions (>=0.1.16and>=0.3.15) as part of the coordinated skill dependency upgrades.requirements/skills-essential.txt (2)
2-2: Upgrade ovos-skill-fallback-unknown minimum version
Bumped from>=0.1.5to>=0.1.9to align fallback behavior with core updates.
4-5: Bump ovos-skill-personal and ovos-skill-date-time versions
Updated to>=0.1.19and>=1.1.3,<2.0.0respectively for compatibility with the new IntentService implementation.requirements/requirements.txt (2)
8-8: Verifyovos-plugin-managermajor version bump
The bump to>=1.0.3,<2.0.0from pre-1.0 may introduce breaking changes. Confirm compatibility with existing plugin integrations and update integration tests accordingly.
10-10: Confirmovos-workshopupdate aligns with refactored pipeline plugins
Upgrading to>=7.0.2,<8.0.0is consistent with the new intent service architecture, but ensure no deprecated workshop APIs are still referenced.requirements/skills-media.txt (1)
3-6: Validate media skill version bumps
Ensure that raising the minimum versions forovos-skill-news,ovos-skill-pyradios,ovos-skill-local-media, andovos-skill-youtube-musicdoes not drop any required features or introduce new dependency conflicts. Run media skill test suites to catch regressions.requirements/skills-internet.txt (1)
2-7: Validate internet skill upgrades
The new minimum versions for weather, DDG, Wolfie, Wikipedia, WikiHow, and Speedtest skills may include API changes or added dependencies. Verify functionality—especially offline fallback behavior—and update tests as needed.requirements/skills-extra.txt (2)
2-2: Reviewovos-skill-wordnetbump
Updating to>=0.2.5could introduce changes in dependencies or behavior; ensure it remains compatible with other NLP components in the pipeline.
5-10: Confirm removal ofovos-skill-days-in-historyand other bumps
It appearsovos-skill-days-in-historywas removed—ensure no residual references exist in docs or code. Also validate the bumped versions for number-facts, ISS-location, cmd, moviemaster, confucius-quotes, and icanhazdadjokes for backward compatibility.requirements/mycroft.txt (1)
7-7: Verifyovos-dinkum-listenerbump
Upgrading to>=0.4.1,<1.0.0may include API changes; run listener integration tests to detect any breaking changes.ovos_core/intent_services/fallback_service.py (1)
160-168: Clean implementation of the new IntentHandlerMatch return.The return statement correctly constructs an IntentHandlerMatch object with all necessary fields, properly aligning with the new pipeline architecture.
requirements/tests.txt (1)
8-8: Dependency update looks good.The ovoscope version update from 0.3.0 to 0.4.0 aligns with the broader refactoring efforts in this PR.
requirements/lgpl.txt (1)
1-2: Dependency updates and formatting improvements.The ovos_padatious version bump and fann2 formatting cleanup are appropriate changes that support the broader refactoring effort.
setup.py (1)
102-103: New standalone intent service entry point.The addition of the
ovos-intent-serviceconsole script enables launching the refactored intent service independently, supporting the new modular architecture.test/unittests/test_skill_manager.py (2)
92-94: LGTM! Attribute rename aligns with skill manager refactoring.The update from
skill_loaderstoplugin_skillscorrectly reflects the removal of legacy skill loading mechanisms and standardization on plugin-based skills only.
156-158: Consistent test updates for the new skill management architecture.All test methods have been properly updated to use
plugin_skillsinstead of the deprecatedskill_loadersattribute, maintaining test functionality while aligning with the refactored skill manager.Also applies to: 172-173
ovos_core/intent_services/__init__.py (1)
1-1: LGTM! Successful modularization of IntentService.This refactoring centralizes the IntentService implementation in a dedicated module while maintaining the public API. The static analysis warning about the unused import is a false positive - this is a valid re-export pattern that allows consumers to import
IntentServicefromovos_core.intent_serviceswhile the implementation lives in the service module.🧰 Tools
🪛 Ruff (0.11.9)
1-1:
ovos_core.intent_services.service.IntentServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
ovos_core/transformers.py (2)
1-1: Approve type system simplification.Removing the
Unionimport andPipelineMatchimport correctly reflects the standardization onIntentHandlerMatchobjects across the intent service ecosystem.Also applies to: 7-7
206-206: LGTM! Method signature standardization.The transform method now exclusively accepts
IntentHandlerMatchobjects, aligning with the broader refactoring where pipeline plugins have been standardized to return this unified match type.test/end2end/test_no_skills.py (2)
11-18: Excellent test infrastructure improvements.The addition of setUp and tearDown methods with shared minicroft instance reuse follows best practices for test performance optimization while maintaining proper cleanup. The pattern is consistent with other end-to-end test files.
25-25: Proper utilization of shared test instance.Both test methods correctly use the shared minicroft instance, improving test execution speed while maintaining isolation through proper setup/teardown.
Also applies to: 48-48
requirements/plugins.txt (3)
5-5: Version bump for utterance normalizer.The update from
>=0.2.1to>=0.2.2likely includes bug fixes or improvements needed by the refactored intent service.
9-16: Well-organized pipeline plugins section.The new pipeline plugins section provides clear categorization and includes all the necessary plugins for the refactored intent service architecture. Version constraints are appropriately maintained.
17-19: Good addition of intent transformer plugins section.The new intent transformer plugins section supports the enhanced transformer functionality in the refactored intent service, providing proper categorization for these specialized plugins.
🧰 Tools
🪛 LanguageTool
[locale-violation] ~18-~18: “template” é um estrangeirismo. É preferível dizer “modelo”.
Context: ...0 # intent transformer plugins keyword-template-matcher>=0.0.1,<1.0.0 ahocorasick-ner>=...(PT_BARBARISMS_REPLACE_TEMPLATE)
ovos_core/intent_services/converse_service.py (1)
266-333: Well-structured match method implementation.The refactored match method properly handles both get_response and converse modes, with clear separation of concerns and appropriate use of the new IntentHandlerMatch return type.
ovos_core/intent_services/stop_service.py (1)
198-213: Verify the intentional use of fixed skill_id for all stop matches.The
skill_idinIntentHandlerMatchis always set to "stop.openvoiceos" even when stopping specific skills. The actual skill being stopped is only reflected in thematch_type. Is this intentional?Please confirm if this design is intentional. If not, consider using the actual skill_id being stopped:
return IntentHandlerMatch( match_type=f"{skill_id}.stop", match_data={"conf": conf}, updated_session=sess, utterance=utterance, - skill_id="stop.openvoiceos" + skill_id=skill_id )Also applies to: 295-301, 305-310
github-actions
bot
added
breaking
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
ovos_core/intent_services/service.py (5)
478-478: Replace bare except with specific exception handling.🧰 Tools
🪛 Ruff (0.11.9)
478-478: Do not use bare
except(E722)
590-597: Improve exception handling in shutdown method.
155-155: Improve variable naming for clarity.🧰 Tools
🪛 Ruff (0.11.9)
155-155: Ambiguous variable name:
l(E741)
162-167: Remove unnecessary else clause after return.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 162-167: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
407-496: Fix inconsistent return statements in handle_utterance.🧰 Tools
🪛 Ruff (0.11.9)
462-462: Ambiguous variable name:
l(E741)
478-478: Do not use bare
except(E722)
🪛 Pylint (3.3.7)
[refactor] 407-407: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🧹 Nitpick comments (3)
ovos_core/skill_manager.py (1)
456-456: Remove extraneous f-prefix from string literal.The string literal doesn't contain any placeholders, so the f-prefix is unnecessary.
- LOG.debug(f"pipelines trained and ready to go") + LOG.debug("pipelines trained and ready to go")🧰 Tools
🪛 Ruff (0.11.9)
456-456: f-string without any placeholders
Remove extraneous
fprefix(F541)
ovos_core/intent_services/fallback_service.py (2)
76-81: Combine conditional branches for better readability.The if-elif structure can be simplified by combining the conditions with logical operators.
- if opmode == FallbackMode.BLACKLIST and skill_id in \ - self.config.get("fallback_blacklist", []): - return False - elif opmode == FallbackMode.WHITELIST and skill_id not in \ - self.config.get("fallback_whitelist", []): - return False + if ((opmode == FallbackMode.BLACKLIST and skill_id in self.config.get("fallback_blacklist", [])) or + (opmode == FallbackMode.WHITELIST and skill_id not in self.config.get("fallback_whitelist", []))): + return False🧰 Tools
🪛 Ruff (0.11.9)
76-81: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
🪛 Pylint (3.3.7)
[refactor] 76-81: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
157-157: Rename unused loop variable for clarity.The
priovariable is not used within the loop body.- for skill_id, prio in sorted_handlers: + for skill_id, _prio in sorted_handlers:🧰 Tools
🪛 Ruff (0.11.9)
157-157: Loop control variable
prionot used within loop bodyRename unused
prioto_prio(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ovos_core/intent_services/converse_service.py(5 hunks)ovos_core/intent_services/fallback_service.py(5 hunks)ovos_core/intent_services/service.py(1 hunks)ovos_core/intent_services/stop_service.py(8 hunks)ovos_core/skill_manager.py(10 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
ovos_core/skill_manager.py
456-456: f-string without any placeholders
Remove extraneous f prefix
(F541)
ovos_core/intent_services/fallback_service.py
76-81: Combine if branches using logical or operator
Combine if branches
(SIM114)
157-157: Loop control variable prio not used within loop body
Rename unused prio to _prio
(B007)
ovos_core/intent_services/service.py
155-155: Ambiguous variable name: l
(E741)
462-462: Ambiguous variable name: l
(E741)
478-478: Do not use bare except
(E722)
🪛 Pylint (3.3.7)
ovos_core/intent_services/fallback_service.py
[refactor] 76-81: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
ovos_core/intent_services/service.py
[refactor] 59-59: Too many instance attributes (8/7)
(R0902)
[refactor] 66-66: Too many arguments (9/5)
(R0913)
[refactor] 66-66: Too many positional arguments (9/5)
(R0917)
[refactor] 162-167: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 407-407: Either all return statements in a function should return an expression, or none of them should.
(R1710)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit_tests
- GitHub Check: license_tests
- GitHub Check: build_tests (3.11)
🔇 Additional comments (10)
ovos_core/skill_manager.py (2)
346-356: LGTM! Loop-based implementation resolves the stack overflow risk.The recursive implementation has been successfully replaced with a loop that prevents potential stack overflow issues while waiting for the intent service. The addition of proper timeout handling and error checking improves robustness.
278-288: Verify plugin skills management approach.The refactoring to use
plugin_skillsinstead ofskill_loaderslooks clean and consistent. The logic properly checks for existing skills before loading new ones.ovos_core/intent_services/fallback_service.py (1)
85-91: LGTM! Mutable default argument issue resolved.The mutable default argument has been properly fixed by using
Optional[FallbackRange] = Noneand initializing inside the function. This prevents potential issues with shared mutable state.ovos_core/intent_services/service.py (1)
66-128: LGTM! Well-structured initialization and plugin loading.The IntentService initialization is well-designed with proper status management, plugin loading, and event handler registration. The pipeline plugin loading with error handling is robust.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 66-66: Too many arguments (9/5)
(R0913)
[refactor] 66-66: Too many positional arguments (9/5)
(R0917)
ovos_core/intent_services/stop_service.py (3)
25-28: LGTM! Constructor follows new pipeline pattern.The constructor properly initializes with optional bus and config parameters, following the consistent pattern established across the refactored intent services.
137-150: LGTM! Good asynchronous event handling design.The new
handle_stop_confirmationmethod provides clean asynchronous handling of stop responses, replacing synchronous waiting patterns. The error handling and event emission logic is well-structured.
151-214: LGTM! Properly implemented pipeline matcher interface.The renamed methods (
match_high,match_medium,match_low) correctly implement theConfidenceMatcherPipelineinterface and returnIntentHandlerMatchobjects with appropriate metadata.ovos_core/intent_services/converse_service.py (3)
247-247: LGTM! Message data modification issue resolved.The message data is now properly copied using the spread operator
{**message.data, "skill_id": skill_id}instead of modifying the original data in place, preventing potential side effects.
264-331: LGTM! Well-implemented state-aware converse matching.The new
matchmethod properly handles different utterance states (RESPONSE vs INTENT), provides better skill filtering, and returns appropriateIntentHandlerMatchobjects. The separation of get_response handling from regular converse logic is a good design improvement.
21-24: LGTM! Constructor follows consistent pattern.The constructor properly adopts the new optional parameter pattern consistent with other refactored intent services.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/end2end/test_converse.py (1)
34-95: Expected message sequences are comprehensive but potentially brittleThe detailed expected message flows provide excellent test coverage but may be fragile if message formats change. Consider if some assertions could be more flexible while maintaining test value.
For maintainability, consider extracting common message patterns or using partial message matching for non-critical fields:
# Example: Create helper methods for common message patterns def _create_activation_message(self, skill_id): return Message(f"{skill_id}.activate", data={}, context={"skill_id": skill_id}) def _create_handler_start_message(self, skill_id, handler_name): return Message("mycroft.skill.handler.start", data={"name": handler_name}, context={"skill_id": skill_id})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
requirements/skills-audio.txt(1 hunks)requirements/skills-essential.txt(1 hunks)requirements/tests.txt(1 hunks)test/end2end/test_converse.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- requirements/tests.txt
- requirements/skills-essential.txt
- requirements/skills-audio.txt
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unit_tests
- GitHub Check: build_tests (3.10)
🔇 Additional comments (4)
test/end2end/test_converse.py (4)
1-8: LGTM - Clean imports and dependenciesThe imports are well-organized and appropriate for an end-to-end test, using the correct testing framework and message handling utilities.
12-21: Consider test isolation concernsThe setup comment mentions potential state issues when reusing minicroft instances. While this optimizes test speed, it could lead to test flakiness if the parrot skill maintains internal state between test runs.
Consider adding state cleanup between tests or documenting the state management expectations for the parrot skill:
def setUp(self): LOG.set_level("DEBUG") self.skill_id = "ovos-skill-parrot.openvoiceos" self.minicroft = get_minicroft([self.skill_id]) # reuse for speed, but beware if skills keeping internal state + # TODO: Add skill state reset if needed to ensure test isolation
97-109: Test configuration looks comprehensiveThe End2EndTest configuration properly handles message flow verification with appropriate flip points and activation tracking. The timeout of 10 seconds should be sufficient for most scenarios.
22-33:Details
✅ Verification successful
Verify pipeline plugin availability
The session configuration specifies pipeline plugins that need to be available in the test environment. Ensure these plugins exist and are properly configured.
🏁 Script executed:
#!/bin/bash # Description: Verify that the specified pipeline plugins are available in the codebase # Expected: Find configuration or implementation files for both plugins echo "Searching for ovos-converse-pipeline-plugin..." rg -i "ovos-converse-pipeline" --type py echo "Searching for ovos-padatious-pipeline-plugin-high..." rg -i "ovos-padatious-pipeline.*high" --type pyLength of output: 1269
Pipeline plugin availability confirmed
Both
ovos-converse-pipeline-pluginandovos-padatious-pipeline-plugin-highare present insetup.pyand registered inovos_core/intent_services/service.py. No further action required.
github-actions
bot
added
breaking
Summary by CodeRabbit
ovos-intent-service) for running the intent service independently.