-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat:pipeline plugin factory #570
base: dev
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve a significant refactor of 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (3)
ovos_core/intent_services/stop_service.py (1)
235-239
: Consider including the confidence score inmatch_data
for consistencyIn the return statement on lines 235-239,
match_data
is set to an empty dictionary. In previous return statements,match_data
includes{"conf": conf}
. For consistency and to provide helpful confidence information, consider including the confidence score.Apply this diff to include
conf
inmatch_data
:return PipelineMatch(handled=True, - # emit instead of intent message {"conf": conf}, - match_data={}, + match_data={"conf": conf}, skill_id=None, utterance=utterance)ovos_core/intent_services/converse_service.py (1)
Line range hint
48-49
: Replace backslashes with parentheses for line continuationUsing backslashes for line continuation is discouraged according to PEP 8 style guidelines. It's recommended to use parentheses for better readability and maintainability. For example, you can rewrite:
acmode = self.config.get("converse_activation") or \ ConverseActivationMode.ACCEPT_ALLas
acmode = ( self.config.get("converse_activation") or ConverseActivationMode.ACCEPT_ALL )Apply this diff to improve the code style:
- acmode = self.config.get("converse_activation") or \ - ConverseActivationMode.ACCEPT_ALL + acmode = ( + self.config.get("converse_activation") or + ConverseActivationMode.ACCEPT_ALL + )ovos_core/intent_services/__init__.py (1)
133-134
: Track Deprecation Schedule forMAP
There's a TODO comment indicating that
MAP
should be deprecated aroundovos-core
version 2.0.0. To ensure this deprecation is properly managed, consider opening a GitHub issue to track the required changes and update any documentation accordingly.Would you like assistance in creating the GitHub issue to track this deprecation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- ovos_core/intent_services/init.py (10 hunks)
- ovos_core/intent_services/converse_service.py (3 hunks)
- ovos_core/intent_services/fallback_service.py (5 hunks)
- ovos_core/intent_services/stop_service.py (6 hunks)
🧰 Additional context used
🪛 Ruff
ovos_core/intent_services/__init__.py
23-23:
ovos_core.intent_services.converse_service.ConverseService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
24-24:
ovos_core.intent_services.fallback_service.FallbackService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
25-25:
ovos_core.intent_services.stop_service.StopService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
🔇 Additional comments (20)
ovos_core/intent_services/fallback_service.py (1)
36-36
: Updated inheritance aligns with new pipeline designThe
FallbackService
class now inherits fromPipelineStageConfidenceMatcher
, which is appropriate given the refactoring towards a generalized pipeline approach.ovos_core/intent_services/stop_service.py (13)
5-5
: Import statements are appropriateThe addition of
Optional
,Dict
,List
, andUnion
from thetyping
module ensures proper type annotations and improves code readability.
13-13
: Appropriate import of pipeline componentsImporting
PipelineMatch
andPipelineStageConfidenceMatcher
aligns with the updated pipeline architecture and reflects the new class hierarchy.
22-24
: Updated class inheritance aligns with the new pipeline architectureChanging the inheritance of
StopService
toPipelineStageConfidenceMatcher
appropriately reflects its role within the pipeline framework and ensures compatibility with the new matching process.
25-28
: Initialization logic is appropriateThe
__init__
method correctly initializes theStopService
with the providedconfig
or defaults from the configuration file, and it properly calls the superclass initializer with the correct parameters.
Line range hint
119-123
: Method signature updated correctlyThe
match_high
method signature now returnsOptional[PipelineMatch]
, aligning it with the new pipeline matching structure. This change ensures consistency in how matches are handled across the pipeline.
148-151
: Correct use ofPipelineMatch
in return statementThe return statement in
match_high
correctly constructs aPipelineMatch
object withhandled=True
, includes the confidence score inmatch_data
, and provides the necessary fields. This adheres to the expected structure for pipeline matches.
161-164
: Proper handling of skill stoppingWithin the loop, the logic properly checks for blacklisted skills and successfully stops the skill if possible, returning a
PipelineMatch
with the appropriate data.
167-170
: Methodmatch_medium
signature updated appropriatelyThe
match_medium
method now correctly returnsOptional[PipelineMatch]
, consistent with the updated pipeline design and ensuring uniformity across matching methods.
193-193
: Delegation tomatch_low
method is appropriateThe
match_medium
method delegates tomatch_low
when certain conditions are met, which is an effective reuse of code and maintains consistency in matching logic.
195-198
: Methodmatch_low
signature updated appropriatelyThe
match_low
method's signature and return type have been updated to returnOptional[PipelineMatch]
, aligning it with the new pipeline framework.
228-231
: Correct construction ofPipelineMatch
objectThe return statement in
match_low
correctly builds aPipelineMatch
with the relevantconf
value inmatch_data
, ensuring the match information is accurately conveyed.
241-251
: Language matching logic is soundThe
_get_closest_lang
method correctly identifies the closest language match usinglangcodes.closest_match
and includes appropriate checks based on the score. This ensures that language resources are accurately utilized.
22-24
: Verify that all usages ofStopService
are updated to reflect the new class hierarchy and method signaturesSince
StopService
now inherits fromPipelineStageConfidenceMatcher
and methods likematch_stop_high
have been renamed (e.g., tomatch_high
), please ensure that all references toStopService
and its methods throughout the codebase have been updated accordingly.Run the following script to identify usages of
StopService
and its old method names:✅ Verification successful
All usages of
StopService
and its outdated methods have been successfully updated. No references toStopService
or the old method names were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of StopService and outdated method names. # Find all instantiations of StopService rg --type py 'StopService\(' # Find all references to the old method names that may need updating rg --type py 'match_stop_high|match_stop_medium|match_stop_low'Length of output: 189
ovos_core/intent_services/converse_service.py (1)
23-25
: Ensure 'bus' and 'config' are properly handled whenNone
in__init__
In the
__init__
method, bothbus
andconfig
parameters default toNone
. Please verify that passingNone
values tosuper().__init__(bus, config)
does not cause unintended behavior, and that the parent classPipelineStageMatcher
can handleNone
values appropriately. Ifbus
orconfig
are required, consider adding checks or default initializations to prevent potential runtime errors.ovos_core/intent_services/__init__.py (5)
208-214
: Ensure Correct Handling ofmatch
TypesThe logic flow between lines 208 and 214 checks for instances of
PipelineMatch
andIntentMatch
. Consider adding anelse
clause to handle cases wherematch
doesn't match either type, or confirm that all possiblematch
types are accounted for.Please verify that all potential
match
types are handled appropriately to prevent unintended behavior.
304-306
: Handle Blacklisted Intents CorrectlyEnsure that the check for blacklisted intents in the session is functioning as intended. The current logic checks
if match.match_type and match.match_type in sess.blacklisted_intents
, which may skip valid matches if not carefully managed.Please confirm that
sess.blacklisted_intents
contains the correct intent identifiers and that legitimate intents are not being inadvertently ignored.
398-401
: Ensureintent_data
is Properly ConstructedWhen constructing
intent_data
, ensure that all necessary fields are included and thatmatch.match_data
is correctly structured. This will prevent potential issues when the data is consumed by other parts of the system.Please verify that
match.match_data
contains all required keys and that the resultingintent_data
is correctly formatted.
424-425
: Properly ShutdownOVOSPipelineFactory
Ensure that calling
OVOSPipelineFactory.shutdown()
properly releases all resources and performs necessary cleanup. This is important to prevent resource leaks or unexpected behavior upon shutdown.Please confirm that
OVOSPipelineFactory.shutdown()
adequately handles the shutdown process.
Line range hint
338-374
: Review Usage of Static Methods for Context HandlersThe methods
handle_add_context
,handle_remove_context
, andhandle_clear_context
have been converted to@staticmethods
. While this is appropriate if they do not useself
, ensure that their registration with the message bus and any calls elsewhere in the code are updated accordingly.Please verify that these static methods are registered and invoked correctly throughout the codebase.
You can run the following shell script to search for their usages:
✅ Verification successful
Verified: Static Methods are Registered Correctly
The static methods
handle_add_context
,handle_remove_context
, andhandle_clear_context
are properly registered and unregistered with the message bus withinovos_core/intent_services/__init__.py
. No issues were found regarding their registration or invocation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of the context handler methods to ensure they are called correctly. # Search for method calls to the context handlers rg --type python 'handle_add_context|handle_remove_context|handle_clear_context'Length of output: 113
Script:
#!/bin/bash # Description: Find all usages of the context handler methods to ensure they are called correctly. # Search for method calls to the context handlers in Python files rg 'handle_add_context|handle_remove_context|handle_clear_context' --glob '**/*.py'Length of output: 962
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #570 +/- ##
==========================================
- Coverage 75.33% 73.32% -2.02%
==========================================
Files 15 15
Lines 3094 1518 -1576
==========================================
- Hits 2331 1113 -1218
+ Misses 763 405 -358
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
requirements/requirements.txt (1)
6-13
: Summary of dependency updatesThis PR updates several key dependencies to new major versions:
padacioso
: 1.0.0+ovos-adapt-parser
: 1.0.0+ovos_ocp_pipeline_plugin
: 1.0.0+ovos-common-query-pipeline-plugin
: 1.0.0+ovos-plugin-manager
: 0.5.0+These updates align with the PR objectives and related PRs mentioned in the description. However, major version bumps often introduce breaking changes. It's crucial to ensure that:
- The codebase has been updated to be compatible with these new versions.
- All features depending on these libraries have been thoroughly tested.
- Any deprecated or removed functionality has been addressed.
- The changes introduced by these new versions align with the project's goals and requirements.
Consider running comprehensive integration tests and reviewing the changelogs of each updated dependency to identify and address any potential issues.
ovos_core/intent_services/fallback_service.py (3)
36-42
: LGTM: Class inheritance and constructor updated appropriatelyThe changes to the class inheritance and constructor signature align well with the new pipeline structure. The use of
Configuration().get("skills", {}).get("fallbacks", {})
for default configuration is a good practice.However, consider using a default argument for
config
to improve readability:def __init__(self, bus: Optional[Union[MessageBusClient, FakeBus]] = None, config: Optional[Dict] = None): config = config or Configuration().get("skills", {}).get("fallbacks", {}) super().__init__(bus, config)This change would make the default behavior more explicit in the method signature.
Line range hint
168-201
: LGTM with suggestion: Return type updated, but add safety checkThe change from
Optional[IntentMatch]
toOptional[PipelineMatch]
aligns well with the new pipeline structure. However, when creating thePipelineMatch
object, there's a potentialIndexError
ifutterances
is empty.Consider adding a safety check:
if utterances: return PipelineMatch(match_data={}, skill_id=skill_id, utterance=utterances[0]) return NoneThis change would prevent a potential
IndexError
ifutterances
is empty.
219-229
: LGTM with suggestion: Deprecated methods added for backward compatibilityThe addition of deprecated methods
low_prio
,medium_prio
, andhigh_prio
is a good practice for maintaining backward compatibility. However, consider adding aDeprecationWarning
in addition to the@deprecated
decorator:import warnings @deprecated("'low_prio' has been renamed to 'match_low'", "2.0.0") def low_prio(self, utterances: List[str], lang: str, message: Message = None) -> Optional[PipelineMatch]: warnings.warn("'low_prio' is deprecated, use 'match_low' instead", DeprecationWarning, stacklevel=2) return self.match_low(utterances, lang, message)This change would provide immediate feedback to developers using deprecated methods during runtime.
ovos_core/intent_services/converse_service.py (3)
23-26
: LGTM: Improved constructor with configuration optionThe updated constructor now accepts an optional
config
parameter, allowing for more flexible initialization and configuration of theConverseService
. This is a good practice that improves modularity and testability.Consider adding type hints for the
config
parameter in the constructor to improve code readability and maintainability. You can update the signature as follows:def __init__(self, bus: Optional[Union[MessageBusClient, FakeBus]] = None, config: Optional[Dict[str, Any]] = None):This change would require adding
Any
to the imports from thetyping
module.
Line range hint
319-348
: LGTM: Improved method naming and return type, but potential IndexErrorThe renaming of
converse_with_skills
tomatch
and the change in return type toOptional[PipelineMatch]
align well with the new pipeline structure and suggest a more standardized approach to handling matches across the pipeline.However, there's a potential
IndexError
when accessingutterances[0]
on line 347. To prevent this, consider modifying the code to handle emptyutterances
lists safely. Apply this diff to fix the issue:return PipelineMatch(handled=state != UtteranceState.RESPONSE, match_data={}, skill_id=skill_id, - utterance=utterances[0]) + utterance=utterances[0] if utterances else None)This change ensures that the code won't raise an
IndexError
ifutterances
is empty.
Line range hint
1-423
: Overall LGTM: Solid refactoring towards pipeline architectureThe changes in this file represent a significant step towards a pipeline-based architecture for the
ConverseService
. The refactoring improves code structure, maintainability, and aligns with modern Python practices. Key improvements include:
- Updated class inheritance and imports
- More flexible constructor with configuration options
- Renamed and improved
match
method (formerlyconverse_with_skills
)- Addition of a deprecated method for backward compatibility
These changes should positively impact the overall quality and extensibility of the codebase.
As you continue this refactoring effort, consider the following suggestions:
- Ensure consistent use of type hints throughout the codebase to improve readability and catch potential type-related issues early.
- Document the new pipeline architecture and how different components interact to help other developers understand and maintain the system.
- Update any related tests to reflect these changes and ensure full coverage of the new functionality.
test/unittests/test_intent_service.py (1)
Line range hint
63-74
: Manage locale changes to prevent side effects in testsIn the
test_no_lang_in_message
method, the locale is changed usingsetup_locale("it-it")
andsetup_locale("en-US")
. Modifying global state like the locale within tests can lead to side effects that affect other tests.Consider using the
setUp
andtearDown
methods to manage locale changes safely:class TestLanguageExtraction(TestCase): + def setUp(self): + self.original_locale = Configuration().get('lang', 'en-US') + + def tearDown(self): + setup_locale(self.original_locale) @mock.patch.dict(Configuration._Configuration__patch, BASE_CONF) def test_no_lang_in_message(self): """No lang in message should result in lang from active locale.""" - setup_locale("it-it") + setup_locale("it-it") msg = Message('test msg', data={}) self.assertEqual(get_message_lang(msg), 'it-IT') - setup_locale("en-US") + setup_locale("en-US") self.assertEqual(get_message_lang(msg), 'en-US')This ensures that any changes to the locale are reset after each test, preventing interference with other tests.
ovos_core/intent_services/stop_service.py (1)
244-251
: Handle cases where no close language match is foundCurrently, if no close match is found (
score
>= 10),_get_closest_lang
returnsNone
, which upstream methods handle. However, explicitly logging or handling this case could improve debugging and user feedback.Consider adding a log message when no close language match is found:
if score < 10: return closest else: LOG.debug(f"No close language match found for '{lang}'") return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- ovos_core/intent_services/init.py (12 hunks)
- ovos_core/intent_services/converse_service.py (4 hunks)
- ovos_core/intent_services/fallback_service.py (5 hunks)
- ovos_core/intent_services/stop_service.py (7 hunks)
- requirements/lgpl.txt (1 hunks)
- requirements/requirements.txt (1 hunks)
- test/unittests/test_intent_service.py (1 hunks)
🧰 Additional context used
🪛 Ruff
ovos_core/intent_services/__init__.py
22-22:
ovos_config.locale.get_full_lang_code
imported but unused(F401)
23-23:
ovos_core.intent_services.converse_service.ConverseService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
24-24:
ovos_core.intent_services.fallback_service.FallbackService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
25-25:
ovos_core.intent_services.stop_service.StopService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
96-96: Ambiguous variable name:
l
(E741)
🔇 Additional comments (23)
requirements/lgpl.txt (1)
1-1
: Approved: Version bump for ovos_padatious, verify compatibilityThe update to
ovos_padatious>=1.0.0,<2.0.0
is a significant version bump. This change adheres to semantic versioning principles, which is good. However, moving to a 1.0 version might introduce breaking changes or new features.Please ensure that:
- The codebase is compatible with the new version of
ovos_padatious
.- Any breaking changes in
ovos_padatious
1.0 have been addressed in this project.- The project takes advantage of any new features or improvements in the 1.0 version.
To verify the impact, you can run:
This will help identify areas of the code that might need attention due to the version update.
✅ Verification successful
Verified: Version bump for ovos_padatious
The update to
ovos_padatious>=1.0.0,<2.0.0
has been successfully verified. The only usage ofovos_padatious
is inovos_core/intent_services/padatious_service.py
, and no incompatibilities were detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for imports and usages of ovos_padatious rg --type python 'from ovos_padatious import|import ovos_padatious' -A 10Length of output: 106
Script:
#!/bin/bash # Search for imports and usages of ovos_padatious in .py files rg -g "*.py" 'from ovos_padatious import|import ovos_padatious' -A 10Length of output: 71
Script:
#!/bin/bash # Search for any usage of ovos_padatious in the entire codebase rg 'ovos_padatious' -A 10Length of output: 258
requirements/requirements.txt (5)
13-13
: Verify compatibility with ovos-plugin-manager 0.5.0+The version range for
ovos-plugin-manager
has been updated from>=0.0.26,<1.0.0
to>=0.5.0,<1.0.0
. While this is not a major version bump, the significant jump in the minor version (from 0.0.26 to 0.5.0) suggests substantial changes. Ensure that the codebase is compatible with this new version range and that all features depending onovos-plugin-manager
have been tested thoroughly.Run the following script to check for any usage of potentially changed or new features:
#!/bin/bash # Description: Search for ovos-plugin-manager usage and potential compatibility issues # Test: Look for import statements and usage of ovos-plugin-manager rg --type python -e 'import ovos_plugin_manager' -e 'from ovos_plugin_manager' -e 'ovos_plugin_manager\.' -C 5 # Test: Check for any TODO or FIXME comments related to ovos-plugin-manager upgrade rg --type python -e 'TODO.*ovos-plugin-manager' -e 'FIXME.*ovos-plugin-manager'
9-9
: Verify compatibility with ovos-common-query-pipeline-plugin 1.0.0+The major version bump for
ovos-common-query-pipeline-plugin
(from<1.0.0
to>=1.0.0
) indicates significant changes. Ensure that the codebase is compatible with this new version and that all features depending onovos-common-query-pipeline-plugin
have been tested thoroughly.Run the following script to check for any usage of deprecated or removed features:
#!/bin/bash # Description: Search for ovos-common-query-pipeline-plugin usage and potential compatibility issues # Test: Look for import statements and usage of ovos-common-query-pipeline-plugin rg --type python -e 'import ovos_common_query_pipeline_plugin' -e 'from ovos_common_query_pipeline_plugin' -e 'ovos_common_query_pipeline_plugin\.' -C 5 # Test: Check for any TODO or FIXME comments related to ovos-common-query-pipeline-plugin upgrade rg --type python -e 'TODO.*ovos-common-query-pipeline-plugin' -e 'FIXME.*ovos-common-query-pipeline-plugin'
8-8
: Verify compatibility with ovos_ocp_pipeline_plugin 1.0.0+The major version bump for
ovos_ocp_pipeline_plugin
(from<1.0.0
to>=1.0.0
) suggests significant changes. Ensure that the codebase is compatible with this new version and that all features depending onovos_ocp_pipeline_plugin
have been tested thoroughly.Run the following script to check for any usage of deprecated or removed features:
#!/bin/bash # Description: Search for ovos_ocp_pipeline_plugin usage and potential compatibility issues # Test: Look for import statements and usage of ovos_ocp_pipeline_plugin rg --type python -e 'import ovos_ocp_pipeline_plugin' -e 'from ovos_ocp_pipeline_plugin' -e 'ovos_ocp_pipeline_plugin\.' -C 5 # Test: Check for any TODO or FIXME comments related to ovos_ocp_pipeline_plugin upgrade rg --type python -e 'TODO.*ovos_ocp_pipeline_plugin' -e 'FIXME.*ovos_ocp_pipeline_plugin'
6-6
: Verify compatibility with padacioso 1.0.0+The major version bump for
padacioso
(from<1.0.0
to>=1.0.0
) suggests significant changes. Ensure that the codebase is compatible with this new version and that all features depending onpadacioso
have been tested thoroughly.Run the following script to check for any usage of deprecated or removed features:
✅ Verification successful
Padacioso version upgrade verified
The
padacioso
dependency has been updated to version>=1.0.0, <2.0.0
. Automated checks found its usage inovos_core/intent_services/padacioso_service.py
with no relatedTODO
orFIXME
comments. The upgrade appears compatible with the current codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for padacioso usage and potential compatibility issues # Test: Look for import statements and usage of padacioso rg --type python -e 'import padacioso' -e 'from padacioso' -e 'padacioso\.' -C 5 # Test: Check for any TODO or FIXME comments related to padacioso upgrade rg --type python -e 'TODO.*padacioso' -e 'FIXME.*padacioso'Length of output: 206
Script:
#!/bin/bash # Description: Search for padacioso usage in Python files and check for related TODO/FIXME comments # Test: Look for import statements and usage of padacioso in .py files rg -g '*.py' -e 'import padacioso' -e 'from padacioso' -e 'padacioso\.' -C 5 # Test: Check for any TODO or FIXME comments related to padacioso upgrade in .py files rg -g '*.py' -e 'TODO.*padacioso' -e 'FIXME.*padacioso'Length of output: 450
7-7
: Verify compatibility with ovos-adapt-parser 1.0.0+The major version bump for
ovos-adapt-parser
(from<1.0.0
to>=1.0.0
) indicates significant changes. Ensure that the codebase is compatible with this new version and that all features depending onovos-adapt-parser
have been tested thoroughly.Run the following script to check for any usage of deprecated or removed features:
ovos_core/intent_services/fallback_service.py (8)
19-31
: LGTM: Import statements updated appropriatelyThe new import statements align with the changes made to the class structure and provide necessary type hinting support. The addition of
PipelineStageConfidenceMatcher
is particularly important as it's now the base class forFallbackService
.
Line range hint
52-82
: LGTM: Configuration access updated consistentlyThe changes in
handle_register_fallback
and_fallback_allowed
methods to useself.config
instead ofself.fallback_config
are consistent with the new configuration management approach. This update improves code consistency and aligns with the class's new structure.🧰 Tools
🪛 Ruff
78-83: Combine
if
branches using logicalor
operatorCombine
if
branches(SIM114)
150-150
: LGTM: Configuration access updatedThe change to access
max_skill_runtime
fromself.config
is consistent with the new configuration management approach and improves code consistency.
204-208
: LGTM: Method renamed and return type updatedThe renaming of
high_prio
tomatch_high
and the update of the return type toOptional[PipelineMatch]
are consistent with the new naming convention and align with the changes in the pipeline structure.
209-213
: LGTM: Method renamed and return type updatedThe renaming of
medium_prio
tomatch_medium
and the update of the return type toOptional[PipelineMatch]
are consistent with the new naming convention and align with the changes in the pipeline structure.
214-218
: LGTM: Method renamed and return type updatedThe renaming of
low_prio
tomatch_low
and the update of the return type toOptional[PipelineMatch]
are consistent with the new naming convention and align with the changes in the pipeline structure.
231-235
: LGTM: Backward compatible property addedThe addition of the
fallback_config
property with a deprecation warning is a good approach for maintaining backward compatibility while encouraging the use ofself.config
directly. This change allows for a smoother transition to the new configuration management approach.
236-239
: LGTM: Backward compatible setter addedThe addition of the
fallback_config
setter with a deprecation warning complements the getter property and provides full backward compatibility. This approach allows for a graceful transition to the new configuration management system while still supporting legacy code that might attempt to setfallback_config
.ovos_core/intent_services/converse_service.py (2)
3-3
: LGTM: Improved class structure and importsThe updated imports and change in class inheritance to
PipelineStageMatcher
indicate a move towards a more structured pipeline approach. This change likely improves the overall architecture and aligns with a larger refactoring effort.Also applies to: 13-13, 20-20
410-413
: LGTM: Good backward compatibility approachThe addition of a deprecated
converse_with_skills
method that calls the newmatch
method is a good practice. It maintains backward compatibility while encouraging users to migrate to the new method name.test/unittests/test_intent_service.py (1)
Line range hint
76-83
: Good addition of language extraction testsThe
test_lang_exists
method effectively tests that when a language code is provided in the message data, it is correctly used byget_message_lang
. This enhances test coverage for language handling.ovos_core/intent_services/stop_service.py (6)
25-28
: Initialization aligns with new class inheritanceThe
__init__
method has been correctly updated to accommodate the new inheritance fromPipelineStageConfidenceMatcher
. Passingconfig
andbus
to the superclass ensures proper initialization.
Line range hint
119-151
: Return type and implementation updated appropriately inmatch_high
The
match_high
method now returns anOptional[PipelineMatch]
, and the return statement has been updated to create aPipelineMatch
object with the necessary fields. This change aligns with the new pipeline structure.
161-164
: Consistent update of return statements in stop matching methodsThe return statements in the stop matching methods (
match_high
,match_medium
,match_low
) have been consistently updated to returnPipelineMatch
objects, ensuring compatibility with the pipeline framework.
193-193
: Redirect tomatch_low
for unhandled cases inmatch_medium
The
match_medium
method appropriately callsself.match_low()
when a medium-confidence match isn't found, allowing for a lower-confidence match attempt. This ensures that the stop intent is thoroughly evaluated.
Line range hint
195-239
: Accurate implementation of fuzzy matching inmatch_low
The
match_low
method correctly implements fuzzy matching for stop intents, calculating confidence scores and handling matches based on the configured minimum confidence. The method also handles active skills appropriately.
241-251
: Efficient language matching in_get_closest_lang
The
_get_closest_lang
method effectively usesclosest_match
to find the closest available language in the_voc_cache
, with sensible thresholds for acceptable matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (4)
test/end2end/session/test_transformers.py (1)
Issues Found: Remaining
intent_service
References DetectedThe shell script identified multiple instances of
intent_service
across various parts of the codebase, including:
Scripts:
scripts/sync_translations.py
scripts/prepare_translations.py
Setup:
setup.py
Tests:
test/end2end/session/test_blacklist.py
test/end2end/minicroft.py
test/backwards_compat/test_ocp.py
Core Modules:
ovos_core/__main__.py
mycroft/skills/__main__.py
mycroft/skills/intent_service.py
mycroft/skills/intent_services/__init__.py
mycroft/skills/intent_services/padatious_service.py
mycroft/skills/intent_services/converse_service.py
mycroft/skills/intent_services/commonqa_service.py
mycroft/skills/intent_services/base.py
mycroft/skills/intent_services/fallback_service.py
mycroft/skills/__init__.py
ovos_core/intent_services/__init__.py
These unresolved references indicate that the refactoring from
intent_service
toadapt_pipeline
is not fully implemented, which may lead to inconsistencies and potential issues in the application's functionality.Recommended Actions:
- Update Remaining References: Ensure all instances of
intent_service
are appropriately updated toadapt_pipeline
or the new system as intended.- Review Dependent Modules: Examine modules like
setup.py
and core services to verify that dependencies are correctly managed post-refactoring.- Comprehensive Testing: After updates, perform thorough testing to ensure that the changes do not introduce regressions or unexpected behaviors.
🔗 Analysis chain
Line range hint
1-164
: Overall impact: Consistent updates with a suggestion for documentation.The changes in this file are consistent with the PR objectives, updating references from
intent_service
toadapt_pipeline
. The test logic and assertions remain intact, ensuring that the same functionality is still being tested.Consider the following suggestions:
- Update any relevant documentation to reflect the change from
intent_service
toadapt_pipeline
.- Ensure that any developer guides or API documentation are updated to reflect this architectural change.
- Consider adding a comment in the test file explaining the reason for the change, which could help future maintainers understand the context of these updates.
To ensure all necessary updates have been made, run the following script:
If any occurrences are found, please review them to determine if they also need to be updated as part of this refactoring.
To address the error with the
rg
command, please use the correct file type identifier for Python files. Here's the updated script:Please run this updated script to verify if there are any lingering references to
intent_service
in your codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to intent_service in documentation and other Python files # Expect: No occurrences of intent_service in contexts where adapt_pipeline should be used rg --type md 'intent_service' rg --type python 'intent_service' -g '!test_transformers.py'Length of output: 121
Script:
#!/bin/bash # Description: Check for any remaining references to intent_service in documentation and other Python files # Expect: No occurrences of intent_service in contexts where adapt_pipeline should be used rg --type md 'intent_service' rg --type py 'intent_service' -g '!test_transformers.py'Length of output: 3449
test/end2end/routing/test_session.py (1)
Line range hint
1-193
: Summary of changes in test_session.pyThe changes in this file reflect a significant architectural shift in the OpenVoiceOS project, moving OCP (Open Voice OS Common Play) functionality from
intent_service
toadapt_pipeline
. Key points:
- The
unittest
module is now explicitly imported.- The
test_no_session
method inTestOCPRouting
is temporarily skipped due to these changes.- All references to
self.core.intent_service.ocp
have been updated toself.core.adapt_pipeline.ocp
.These changes are consistent throughout the file, but it's crucial to ensure that:
- The skipped test is updated and re-enabled as soon as possible.
- Similar changes are applied consistently across the entire test suite and main codebase.
- The implications of this architectural change are documented for other developers.
Consider adding a comment at the top of the file explaining the architectural change and why the test is currently skipped. This will provide context for other developers who may work on this file in the future.
test/end2end/session/test_blacklist.py (2)
4-4
: Consider removing redundant importThe
unittest
module is already implicitly available asTestCase
is imported fromunittest
. Unless you're using otherunittest
features directly, this import can be removed to keep the imports clean and minimal.
141-143
: Approved: Test skipped due to architectural changesThe addition of the
@unittest.skip
decorator with a TODO comment is a good practice to highlight that this test needs to be updated due to architectural changes. The changes fromintent_service
toadapt_pipeline
reflect these architectural updates.To ensure this TODO is not forgotten, would you like me to create a GitHub issue to track the task of updating this test? This will help in maintaining test coverage as the system evolves.
Also applies to: 168-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- mycroft/skills/intent_services/init.py (1 hunks)
- test/end2end/routing/test_session.py (3 hunks)
- test/end2end/session/test_blacklist.py (3 hunks)
- test/end2end/session/test_ocp.py (39 hunks)
- test/end2end/session/test_transformers.py (2 hunks)
🧰 Additional context used
🪛 Ruff
mycroft/skills/intent_services/__init__.py
1-1:
ovos_core.intent_services.ConverseService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
1-1:
ovos_core.intent_services.FallbackService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
2-2:
ovos_adapt.opm.AdaptPipeline
imported but unused(F401)
3-3:
padacioso.opm.PadaciosoPipeline
imported but unused(F401)
4-4:
ovos_commonqa.opm.CommonQAService
imported but unused(F401)
5-5:
ovos_plugin_manager.templates.pipeline.IntentMatch
imported but unused(F401)
6-6:
ovos_workshop.intents.Intent
imported but unused(F401)
6-6:
ovos_workshop.intents.IntentBuilder
imported but unused(F401)
6-6:
ovos_workshop.intents.Intent
imported but unused(F401)
9-9:
ovos_padatious.opm.PadatiousMatcher
imported but unused; consider usingimportlib.util.find_spec
to test for availability(F401)
🔇 Additional comments (15)
mycroft/skills/intent_services/__init__.py (8)
Line range hint
1-14
: Summary of changes and recommendationsThe import statements in this file have been significantly refactored, likely as part of a larger effort to reorganize the codebase structure. The changes appear to maintain backward compatibility through the use of aliases.
Key points:
- Several imports have been updated to use new source modules.
- Aliases have been introduced to maintain compatibility with existing code.
- New imports have been added, potentially introducing new functionality.
Recommendations:
- Verify the usage of all imported components as suggested in the previous comments.
- Remove the duplicate
Intent
import.- Consider adding these imports to
__all__
if they are meant to be exported from this module.- After verifying usage, remove any unused imports to keep the codebase clean.
These changes appear to be well-structured and consistent with the refactoring objectives. Once the recommendations are addressed, the code will be in good shape.
🧰 Tools
🪛 Ruff
1-1:
ovos_core.intent_services.ConverseService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
1-1:
ovos_core.intent_services.FallbackService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
2-2:
ovos_adapt.opm.AdaptPipeline
imported but unused(F401)
3-3:
padacioso.opm.PadaciosoPipeline
imported but unused(F401)
4-4:
ovos_commonqa.opm.CommonQAService
imported but unused(F401)
5-5:
ovos_plugin_manager.templates.pipeline.IntentMatch
imported but unused(F401)
6-6:
ovos_workshop.intents.Intent
imported but unused(F401)
6-6:
ovos_workshop.intents.IntentBuilder
imported but unused(F401)
6-6:
ovos_workshop.intents.Intent
imported but unused(F401)
9-9:
ovos_padatious.opm.PadatiousMatcher
imported but unused; consider usingimportlib.util.find_spec
to test for availability(F401)
4-4
: Approve import change for CommonQAService and verify its usageThe import for
CommonQAService
has been updated to useovos_commonqa.opm
. This change appears to be part of the larger refactoring effort.To ensure this import is necessary, please run the following script:
#!/bin/bash # Description: Check for usage of CommonQAService in the codebase echo "Searching for CommonQAService usage:" rg --type python "CommonQAService" -g "!*/__init__.py"If no usage is found, consider removing this import to keep the codebase clean.
🧰 Tools
🪛 Ruff
4-4:
ovos_commonqa.opm.CommonQAService
imported but unused(F401)
6-6
:⚠️ Potential issueApprove import changes, remove duplicate, and verify usage
The imports for
Intent
,IntentBuilder
, andAdaptIntent
have been updated to useovos_workshop.intents
. This change appears to be part of the larger refactoring effort.There's a duplicate import of
Intent
. Please remove one of them:-from ovos_workshop.intents import Intent as AdaptIntent, IntentBuilder, Intent +from ovos_workshop.intents import Intent as AdaptIntent, IntentBuilderTo ensure these imports are necessary, please run the following script:
#!/bin/bash # Description: Check for usage of Intent, IntentBuilder, and AdaptIntent in the codebase echo "Searching for Intent usage:" rg --type python "\bIntent\b" -g "!*/__init__.py" echo "\nSearching for IntentBuilder usage:" rg --type python "IntentBuilder" -g "!*/__init__.py" echo "\nSearching for AdaptIntent usage:" rg --type python "AdaptIntent" -g "!*/__init__.py"If any of these are unused, consider removing the corresponding import to keep the codebase clean.
🧰 Tools
🪛 Ruff
6-6:
ovos_workshop.intents.Intent
imported but unused(F401)
6-6:
ovos_workshop.intents.IntentBuilder
imported but unused(F401)
6-6:
ovos_workshop.intents.Intent
imported but unused(F401)
1-1
: Verify usage of ConverseService and FallbackServiceThe static analysis tool suggests that
ConverseService
andFallbackService
are imported but unused. However, these appear to be core components of the intent services.Please run the following script to check for their usage in the codebase:
If these services are indeed unused, consider removing them or adding them to
__all__
if they are meant to be exported.🧰 Tools
🪛 Ruff
1-1:
ovos_core.intent_services.ConverseService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
1-1:
ovos_core.intent_services.FallbackService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
Line range hint
9-14
: Approve changes to PadatiousService import and verify PadatiousMatcher usageThe import for
PadatiousService
has been updated to usePadatiousPipeline
fromovos_padatious.opm
with an alias. The fallback import has also been updated accordingly. These changes are consistent with the overall refactoring effort.To ensure the
PadatiousMatcher
import is necessary, please run the following script:If no usage is found, consider removing the
PadatiousMatcher
import to keep the codebase clean.✅ Verification successful
Re-run the verification script with corrected parameters
Please use the following script to check for the usage of
PadatiousMatcher
in the codebase:
Confirm
PadatiousMatcher
is actively used and should remainThe
PadatiousMatcher
is utilized in the following files:
ovos_core/intent_services/padatious_service.py
mycroft/skills/intent_services/__init__.py
mycroft/skills/intent_services/padatious_service.py
Therefore, the import of
PadatiousMatcher
should be retained to ensure proper functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of PadatiousMatcher in the codebase echo "Searching for PadatiousMatcher usage:" rg --type python "PadatiousMatcher" -g "!*/__init__.py"Length of output: 266
Script:
#!/bin/bash # Description: Check for usage of PadatiousMatcher in the codebase echo "Searching for PadatiousMatcher usage:" rg --type py "PadatiousMatcher" -g "!*/__init__.py"Length of output: 564
🧰 Tools
🪛 Ruff
1-1:
ovos_core.intent_services.ConverseService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
1-1:
ovos_core.intent_services.FallbackService
imported but unused; consider removing, adding to__all__
, or using a redundant alias(F401)
2-2:
ovos_adapt.opm.AdaptPipeline
imported but unused(F401)
3-3:
padacioso.opm.PadaciosoPipeline
imported but unused(F401)
4-4:
ovos_commonqa.opm.CommonQAService
imported but unused(F401)
5-5:
ovos_plugin_manager.templates.pipeline.IntentMatch
imported but unused(F401)
6-6:
ovos_workshop.intents.Intent
imported but unused(F401)
6-6:
ovos_workshop.intents.IntentBuilder
imported but unused(F401)
6-6:
ovos_workshop.intents.Intent
imported but unused(F401)
9-9:
ovos_padatious.opm.PadatiousMatcher
imported but unused; consider usingimportlib.util.find_spec
to test for availability(F401)
3-3
: Approve addition of PadaciosoService import and verify its usageA new import for
PadaciosoPipeline
frompadacioso.opm
has been added with an aliasPadaciosoService
. This addition appears to be part of the refactoring effort.To ensure this import is necessary, please run the following script:
If no usage is found, consider removing this import to keep the codebase clean.
✅ Verification successful
Approve addition of PadaciosoService import and confirm its usage
The import for
PadaciosoService
is utilized withinovos_core/intent_services/padacioso_service.py
, ensuring that it is necessary for the application's functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of PadaciosoService in the codebase echo "Searching for PadaciosoService usage:" rg --type python "PadaciosoService" -g "!*/__init__.py"Length of output: 262
Script:
#!/bin/bash # Description: Check for usage of PadaciosoService in the codebase echo "Searching for PadaciosoService usage:" rg --type py "PadaciosoService" -g "!*/__init__.py"Length of output: 511
🧰 Tools
🪛 Ruff
3-3:
padacioso.opm.PadaciosoPipeline
imported but unused(F401)
5-5
: Approve import change for IntentMatch and verify its usageThe import for
IntentMatch
has been updated to useovos_plugin_manager.templates.pipeline
. This change appears to be part of the larger refactoring effort.To ensure this import is necessary, please run the following script:
If no usage is found, consider removing this import to keep the codebase clean.
🧰 Tools
🪛 Ruff
5-5:
ovos_plugin_manager.templates.pipeline.IntentMatch
imported but unused(F401)
2-2
: Approve import change for AdaptService and verify its usageThe import for
AdaptService
has been updated to useAdaptPipeline
fromovos_adapt.opm
with an alias. This change appears to be part of a larger refactoring effort.To ensure this import is necessary, please run the following script:
If no usage is found, consider removing this import to keep the codebase clean.
🧰 Tools
🪛 Ruff
2-2:
ovos_adapt.opm.AdaptPipeline
imported but unused(F401)
test/end2end/session/test_transformers.py (2)
100-104
: LGTM: Pipeline references updated correctly with a minor suggestion.The changes from
self.core.intent_service
toself.core.adapt_pipeline
are consistent with the refactoring mentioned in the PR description and the changes in thetest_cancel
method. These updates correctly reflect the new structure for accessing loaded metadata plugins.Consider splitting the long assertion on lines 103-104 for improved readability:
self.assertIn( 'ovos-metadata-test-plugin', self.core.adapt_pipeline.metadata_plugins.loaded_plugins, self.core.adapt_pipeline.metadata_plugins.find_plugins() )To ensure consistency across the codebase, let's verify other occurrences of this pattern:
#!/bin/bash # Description: Check for any remaining references to intent_service.metadata_plugins # Expect: No occurrences of intent_service.metadata_plugins rg --type python 'intent_service\.metadata_plugins'
31-31
: LGTM: Pipeline reference updated correctly.The change from
self.core.intent_service
toself.core.adapt_pipeline
is consistent with the refactoring mentioned in the PR description. This update correctly reflects the new structure for accessing loaded utterance plugins.To ensure consistency across the codebase, let's verify other occurrences of this pattern:
✅ Verification successful
All references to
intent_service.utterance_plugins
have been successfully removed from the codebase, ensuring consistency with the updated pipeline structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to intent_service.utterance_plugins # Expect: No occurrences of intent_service.utterance_plugins rg --type python 'intent_service\.utterance_plugins'Length of output: 178
Script:
#!/bin/bash # Description: Check for any remaining references to intent_service.utterance_plugins # Expect: No occurrences of intent_service.utterance_plugins rg --type py 'intent_service\.utterance_plugins'Length of output: 50
test/end2end/routing/test_session.py (3)
2-2
: LGTM: Explicit import of unittest module.The addition of
import unittest
is appropriate as it's used later in the file for the@unittest.skip
decorator. Explicit imports improve code readability and maintainability.
128-130
: Session management updated to use adapt_pipeline.The change from
self.core.intent_service.ocp
toself.core.adapt_pipeline.ocp
for session management is consistent with the earlier changes. This reflects the architectural shift in how OCP sessions are managed.To ensure this change is applied consistently throughout the test suite, run the following script:
This will help identify any other test files that may need similar updates.
✅ Verification successful
Session management change verified successfully.
All references to
intent_service.ocp.ocp_sessions
have been removed, andadapt_pipeline.ocp.ocp_sessions
is consistently used across the test suite. The architectural shift in session management is correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to intent_service.ocp in session management echo "Checking for remaining references to intent_service.ocp in session management:" rg "intent_service\.ocp\.ocp_sessions" --type python echo "Checking for new references to adapt_pipeline.ocp in session management:" rg "adapt_pipeline\.ocp\.ocp_sessions" --type pythonLength of output: 493
Script:
#!/bin/bash # Description: Check for any remaining references to intent_service.ocp in session management echo "Checking for remaining references to intent_service.ocp in session management in Python files:" rg "intent_service\.ocp\.ocp_sessions" -g "*.py" echo "Checking for new references to adapt_pipeline.ocp in session management in Python files:" rg "adapt_pipeline\.ocp\.ocp_sessions" -g "*.py"Length of output: 4276
98-100
: Skipped test needs updating due to architectural changes.The
@unittest.skip
decorator has been added to temporarily disable this test. This is appropriate given the architectural changes, but it's important to address this TODO as soon as possible.The assertion change from
self.core.intent_service.ocp
toself.core.adapt_pipeline.ocp
reflects the new location of theocp
attribute. Ensure this change is consistent with the overall architectural updates.To verify the consistency of this change across the codebase, run the following script:
This will help ensure that all relevant parts of the codebase have been updated consistently.
✅ Verification successful
All references updated correctly; changes are consistent across the codebase.
The
@unittest.skip
decorator appropriately disables the test due to the architectural changes. The assertion has been successfully updated fromself.core.intent_service.ocp
toself.core.adapt_pipeline.ocp
, and the shell script confirms that there are no remaining references tointent_service.ocp
whileadapt_pipeline.ocp
is consistently used throughout the test files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to intent_service.ocp and new references to adapt_pipeline.ocp echo "Checking for remaining references to intent_service.ocp:" rg "intent_service\.ocp" --type python echo "Checking for new references to adapt_pipeline.ocp:" rg "adapt_pipeline\.ocp" --type pythonLength of output: 377
Script:
#!/bin/bash # Description: Check for any remaining references to intent_service.ocp and new references to adapt_pipeline.ocp echo "Checking for remaining references to intent_service.ocp:" rg "intent_service\.ocp" -t python echo "Checking for new references to adapt_pipeline.ocp:" rg "adapt_pipeline\.ocp" -t pythonLength of output: 369
Script:
#!/bin/bash # Description: Check for any remaining references to intent_service.ocp and new references to adapt_pipeline.ocp using file extension patterns echo "Checking for remaining references to intent_service.ocp:" rg "intent_service\.ocp" -g "*.py" echo "Checking for new references to adapt_pipeline.ocp:" rg "adapt_pipeline\.ocp" -g "*.py"Length of output: 6814
test/end2end/session/test_ocp.py (2)
213-214
: Ensure Extractors Match Expected ValuesIn your assertion,
available_extractors
is set to["test"]
. Verify that this matches the intended extractors within your test environment. Ifavailable_extractors
is meant to include more items, update the assertion accordingly.Run the following script to list all extractors:
#!/bin/bash # Description: Verify the extractors available in the codebase. grep -r 'def available_extractors' .
160-161
: Confirm Correct Use ofavailable_extractors
Ensure that
available_extractors()
returns the correct list of extractors expected in this test. Ifavailable_extractors()
is dynamic, consider mocking its return value to have a predictable test outcome.You can run the following script to verify the extractors:
8244319
to
fdb1275
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
ovos_core/intent_services/fallback_service.py (1)
35-36
: Consider documenting pipeline stage integrationAs this class is now part of the pipeline architecture, consider adding documentation about:
- The role of this stage in the pipeline
- Expected input/output contracts
- Integration points with other pipeline stages
ovos_core/intent_services/converse_service.py (3)
3-15
: LGTM! Consider enhancing type hints for config parameterThe class refactor and import organization look good. The constructor properly handles optional parameters and follows configuration best practices.
Consider being more specific with the config type hint:
- def __init__(self, bus: Optional[Union[MessageBusClient, FakeBus]] = None, - config: Optional[Dict] = None): + def __init__(self, bus: Optional[Union[MessageBusClient, FakeBus]] = None, + config: Optional[Dict[str, Any]] = None):Also applies to: 19-25
Line range hint
318-347
: Fix potential IndexError in match methodThe method could raise an IndexError when accessing
utterances[0]
if the utterances list is empty. This issue was previously identified but hasn't been addressed.Apply this fix:
return PipelineMatch(handled=state != UtteranceState.RESPONSE, match_data={}, skill_id=skill_id, - utterance=utterances[0]) + utterance=utterances[0] if utterances else "")
Line range hint
237-241
: Consider extracting timeout values as constantsThe timeout values are currently hardcoded. Consider extracting these as class or module-level constants for better maintainability.
+ # Default timeout values in seconds + DEFAULT_SKILL_TIMEOUT = 300 + DEFAULT_MAX_SKILL_RUNTIME = 10 + DEFAULT_CONVERSE_RESPONSE_TIMEOUT = 0.5 class ConverseService(PipelineStageMatcher): def __init__(self, bus: Optional[Union[MessageBusClient, FakeBus]] = None, config: Optional[Dict] = None):Then update the references:
- def_timeout = self.config.get("timeout", 300) + def_timeout = self.config.get("timeout", DEFAULT_SKILL_TIMEOUT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
ovos_core/intent_services/__init__.py
(5 hunks)ovos_core/intent_services/converse_service.py
(3 hunks)ovos_core/intent_services/fallback_service.py
(4 hunks)ovos_core/intent_services/stop_service.py
(6 hunks)
🧰 Additional context used
🪛 Ruff
ovos_core/intent_services/__init__.py
24-24: ovos_core.intent_services.converse_service.ConverseService
imported but unused; consider removing, adding to __all__
, or using a redundant alias
(F401)
25-25: ovos_core.intent_services.fallback_service.FallbackService
imported but unused; consider removing, adding to __all__
, or using a redundant alias
(F401)
26-26: ovos_core.intent_services.stop_service.StopService
imported but unused; consider removing, adding to __all__
, or using a redundant alias
(F401)
🔇 Additional comments (13)
ovos_core/intent_services/fallback_service.py (5)
38-41
: LGTM: Constructor improvements enhance type safety and configuration handling
The updated constructor with type hints and safer configuration access is a good improvement. The use of Configuration().get("skills", {}).get("fallbacks", {})
prevents potential KeyError exceptions.
Line range hint 51-57
: LGTM: Well-implemented priority override system
The priority override system is well-implemented with appropriate logging for transparency when priorities are modified.
76-82
: LGTM: Robust permission checking implementation
The fallback permission system is well-structured with clear separation between blacklist and whitelist modes.
🧰 Tools
🪛 Ruff
77-82: Combine if
branches using logical or
operator
Combine if
branches
(SIM114)
207-220
: LGTM: Well-structured method renaming with proper deprecation
The priority matching methods have been appropriately renamed for better clarity, with proper deprecation notices for backward compatibility.
Also applies to: 222-232
234-242
: LGTM: Clean deprecation of fallback_config property
The fallback_config property deprecation is well-implemented with clear migration path to using config directly.
ovos_core/intent_services/stop_service.py (5)
5-5
: LGTM: Import and class hierarchy changes look good!
The changes properly support type hints and the new pipeline architecture. The inheritance from PipelineStageConfidenceMatcher
aligns with the modular pipeline factory approach mentioned in the PR objectives.
Also applies to: 8-8, 11-11, 13-13, 16-16, 18-18, 22-22
25-28
: LGTM: Constructor changes are well-typed
The constructor properly handles optional parameters and maintains backward compatibility.
294-304
: LGTM: Deprecation warnings are properly implemented
The deprecated methods provide a clean migration path with clear version information.
Line range hint 22-304
: Verify cross-repository compatibility
This refactor is part of a larger change across multiple repositories. Please ensure that:
- The pipeline stage interface matches the expectations in ovos-plugin-manager
- The PipelineMatch structure is compatible with all companion PRs
- The deprecation timeline aligns across all affected repositories
#!/bin/bash
# Check for consistent interface usage across repos
rg "class.*PipelineStageConfidenceMatcher"
# Verify PipelineMatch structure usage
rg "PipelineMatch.*handled.*match_data.*skill_id.*utterance"
Line range hint 119-166
: Verify global stop behavior edge case
In match_high
, the global stop condition might be triggered incorrectly when:
is_stop
is Trueget_active_skills
returns an empty list due to a temporary condition (e.g., skills still initializing)
This could lead to unintended global stops when skills are temporarily unavailable.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check the skill initialization behavior.
Based on the code analysis, I can now provide a final response regarding the global stop behavior concern.
Global stop behavior is correctly implemented
The implementation in StopService.match_high
has proper safeguards against false global stops:
- The
get_active_skills
method only returns skill IDs from the session's active skills list, which is maintained by the skill manager and converse service - Skills go through a proper initialization and activation lifecycle:
- Skills emit "mycroft.skills.initialized" when ready
- The skill manager tracks active state via
skill_loader.active
- Skills are only added to active_skills after successful activation
The empty active skills check not len(self.get_active_skills(message))
is intentional and safe because:
- It only triggers global stop when combined with explicit "global_stop" vocab match
- Or when "stop" is matched but no skills are actively running (desired behavior)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing error reports or issues related to global stop behavior
rg -i "global.?stop.*error|global.?stop.*issue" .github/
# Check for any existing defensive programming around skill initialization
rg "skill.*initializ|skill.*start" -A 5
Length of output: 47344
Script:
#!/bin/bash
# Check for skill initialization and ready state handling
ast-grep --pattern 'self.bus.emit(Message("mycroft.skills.initialized"))'
# Check for skill status tracking
rg "skill.*ready|skill.*status" -A 3
# Check how active skills are tracked
ast-grep --pattern 'def get_active_skills'
# Check for any defensive programming around skill availability
rg "skill.*available|skill.*active" -A 3
Length of output: 31547
ovos_core/intent_services/converse_service.py (1)
409-411
: LGTM! Well-handled deprecation
The deprecation of converse_with_skills
is properly implemented with:
- Clear deprecation notice
- Version information
- Correct forwarding to the new
match
method
ovos_core/intent_services/__init__.py (2)
409-530
: LGTM! Well-structured deprecation handling
The deprecated code section is well-organized with:
- Clear deprecation messages
- Consistent version numbers
- Proper stubs for backward compatibility
51-51
:
Fix truncated comment
The comment appears to be truncated: "pre-loa"
- OVOSPipelineFactory.create(use_cache=True, bus=self.bus) # pre-loa
+ OVOSPipelineFactory.create(use_cache=True, bus=self.bus) # pre-load pipelines
Likely invalid or redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
test/end2end/minicroft.py (1)
Line range hint
16-22
: Consider adding test cases for pipeline factory integrationSince
MiniCroft
is a test utility, it's important to ensure it properly tests the new pipeline factory architecture. Consider adding specific test cases that verify:
- Pipeline plugin initialization
- Plugin configuration handling
- Error scenarios in the pipeline
Would you like me to help create additional test cases for these scenarios?
test/unittests/test_intent_service.py (2)
97-97
: LGTM! Good architectural improvement.The change to use
bus.emit
for vocabulary registration better reflects the real-world usage pattern and aligns with the event-driven architecture. This makes the test more valuable by exercising the actual message passing mechanism used in production.
101-101
: Consider using a more robust serialization method.While using
bus.emit
for intent registration is good, directly usingintent.__dict__
for serialization could be fragile if theIntentBuilder
's internal structure changes. Consider using a dedicated serialization method or property that provides a stable interface.Example approach:
# In IntentBuilder class @property def serialized(self): return { 'name': self.intent_name, 'requirements': self.requires, 'optional': self.optional, # ... other necessary fields } # In test msg = Message('register_intent', intent.serialized)ovos_core/intent_services/__init__.py (2)
46-48
: Fix incomplete commentThe comment "pre-loa" appears to be incomplete.
-OVOSPipelineFactory.create(use_cache=True, bus=self.bus) # pre-loa +OVOSPipelineFactory.create(use_cache=True, bus=self.bus) # pre-load pipelines
408-490
: Consider cleanup of deprecated codeThere is a significant amount of deprecated code that should be considered for removal in a future version. All items are properly marked with deprecation notices and version information, but maintaining this much deprecated code increases maintenance burden.
Consider:
- Creating a tracking issue for removal of deprecated code in version 1.0.0
- Documenting migration paths for users in the changelog
- Providing migration scripts if possible
ovos_core/intent_services/fallback_service.py (2)
Line range hint
152-156
: Update configuration file reference from 'mycroft.conf' to 'ovos.conf'The warning message refers to
'mycroft.conf'
, which may be outdated in the context of OpenVoiceOS. Consider updating it to'ovos.conf'
to reflect the current configuration file name.
234-242
: Align deprecation version numbers in warning messagesThe deprecation warnings for
fallback_config
mention version"1.0.0"
, while the deprecation decorators for methodslow_prio
,medium_prio
, andhigh_prio
mention"2.0.0"
. For consistency, consider aligning the version numbers across all deprecation warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
ovos_core/intent_services/__init__.py
(5 hunks)ovos_core/intent_services/fallback_service.py
(4 hunks)test/end2end/minicroft.py
(1 hunks)test/unittests/test_intent_service.py
(1 hunks)
🧰 Additional context used
🪛 Ruff
ovos_core/intent_services/__init__.py
24-24: ovos_core.intent_services.converse_service.ConverseService
imported but unused; consider removing, adding to __all__
, or using a redundant alias
(F401)
25-25: ovos_core.intent_services.fallback_service.FallbackService
imported but unused; consider removing, adding to __all__
, or using a redundant alias
(F401)
26-26: ovos_core.intent_services.stop_service.StopService
imported but unused; consider removing, adding to __all__
, or using a redundant alias
(F401)
🔇 Additional comments (2)
test/end2end/minicroft.py (1)
21-21
: Verify fallback handling after removal of _register_intent_services
The direct instantiation of IntentService
simplifies the code, but we should verify that fallback handling (previously done in the removed _register_intent_services
method) is now properly handled by the new pipeline architecture.
✅ Verification successful
Let me try a different search strategy to verify the fallback handling.
Let me check the actual implementation of the fallback handling in the new architecture.
Based on the code analysis, I can now generate the final response:
Fallback handling is properly implemented in the new pipeline architecture
The fallback functionality is fully preserved in the new architecture:
FallbackService
is now aPipelineStageConfidenceMatcher
that handles fallback skills with proper priority ranges- Fallback registration/deregistration is handled via messagebus events
- Test cases verify the complete fallback flow, including:
- Proper handling of fallback priorities
- Session management
- Skill activation/deactivation
- Timeout handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that fallback handling is implemented in the new pipeline architecture
# Check for fallback related code in IntentService
echo "Checking IntentService implementation for fallback handling..."
rg -A 5 "fallback" --type python
# Look for FallbackSkill usage
echo "Checking FallbackSkill usage..."
ast-grep --pattern 'class $_ (FallbackSkill)'
Length of output: 319
Script:
#!/bin/bash
# Search for fallback related code without file type restriction
echo "Checking fallback handling implementation..."
rg -l "fallback"
# Search for intent service related code
echo -e "\nChecking IntentService implementation..."
rg -A 5 "class IntentService"
# Look for the removed _register_intent_services method
echo -e "\nChecking for previous _register_intent_services implementation..."
git log -p | rg "_register_intent_services"
Length of output: 2552
Script:
#!/bin/bash
# Check the content of fallback service implementation
echo "Checking fallback service implementation..."
cat ovos_core/intent_services/fallback_service.py
# Check the IntentService initialization and fallback registration
echo -e "\nChecking IntentService initialization..."
cat ovos_core/intent_services/__init__.py
# Check test cases to verify fallback functionality
echo -e "\nChecking fallback test cases..."
cat test/end2end/session/test_fallback.py
Length of output: 51164
ovos_core/intent_services/__init__.py (1)
Line range hint 1-570
: Overall architecture looks solid
The transition to a pipeline architecture is well-implemented with proper deprecation notices and backward compatibility. The code is well-structured and maintains good separation of concerns.
Let's verify the pipeline plugin system is properly set up:
✅ Verification successful
Let me gather more information about the pipeline architecture to verify its implementation:
Let me gather one final piece of information about the plugin system:
Pipeline architecture successfully verified
The code implements a robust plugin-based pipeline architecture:
- Proper initialization of
OVOSPipelineFactory
with plugin discovery - Clean separation between pipeline stages via the plugin manager
- Correct handling of pipeline configuration through sessions
- Appropriate deprecation notices for legacy components
- Well-structured plugin templates and base classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify pipeline plugin system setup
# Check for pipeline plugin implementations
rg -l "class.*PipelineMatcher.*:"
# Check for pipeline factory usage
rg "OVOSPipelineFactory\.create"
Length of output: 308
Script:
#!/bin/bash
# Check for pipeline plugin manager imports and usage
rg "from ovos_plugin_manager.pipeline" -A 2
# Check for pipeline related configuration
rg "pipeline.*=.*\[" -A 3
# Look for pipeline matcher base classes
ast-grep --pattern 'class $_ (PipelineMatcher|BasePipelineMatcher):
$$$'
Length of output: 17335
Script:
#!/bin/bash
# Check for plugin manager configuration and initialization
rg "OVOSPipelineFactory" -B 2 -A 2
# Check for plugin related imports
rg "from ovos_plugin_manager" -A 2
Length of output: 9435
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
ovos_core/intent_services/__init__.py (1)
47-47
: Fix incomplete commentThe comment appears to be cut off:
-OVOSPipelineFactory.create(use_cache=True, bus=self.bus) # pre-loa +OVOSPipelineFactory.create(use_cache=True, bus=self.bus) # pre-load
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_core/intent_services/__init__.py
(5 hunks)
🧰 Additional context used
🪛 Ruff
ovos_core/intent_services/__init__.py
23-23: ovos_core.intent_services.converse_service.ConverseService
imported but unused; consider removing, adding to __all__
, or using a redundant alias
(F401)
24-24: ovos_core.intent_services.fallback_service.FallbackService
imported but unused; consider removing, adding to __all__
, or using a redundant alias
(F401)
25-25: ovos_core.intent_services.stop_service.StopService
imported but unused; consider removing, adding to __all__
, or using a redundant alias
(F401)
🔇 Additional comments (2)
ovos_core/intent_services/__init__.py (2)
Line range hint 377-397
: LGTM! Intent handling changes look good
The changes to handle_get_intent and shutdown methods properly integrate with the new pipeline architecture.
408-490
: LGTM! Deprecation handling is well implemented
The deprecated properties and methods are properly marked with:
- Clear deprecation messages indicating the replacement functionality
- Version numbers for deprecation
- Proper return type annotations
76f506c
to
2900533
Compare
loads pipeline plugins from config 🎉 no longer tied to adapt/padatious
106a717
to
7248243
Compare
partially split into #571
needs OpenVoiceOS/ovos-plugin-manager#270
companion PRS:
moved tests:
Summary by CodeRabbit
New Features
Bug Fixes
Deprecations
Refactor
MiniCroft
class for better clarity and efficiency.