fix:move code to dependencies #571
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the transition from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IntentService
participant FallbackService
participant PipelineMatch
User->>IntentService: Send Intent
IntentService->>PipelineMatch: Process Intent
PipelineMatch-->>IntentService: Return Match
IntentService->>FallbackService: Handle Fallback
FallbackService-->>IntentService: Return Fallback Response
IntentService-->>User: Respond with Result
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
requirements/requirements.txt (1)
6-13: Summary: Dependency updates align with PR objectives but require careful verification.The updates to
padacioso,ovos-adapt-parser,ovos_ocp_pipeline_plugin,ovos-common-query-pipeline-plugin, andovos-plugin-manageralign with the PR objectives to update requirements related to the latest pipeline plugins signature. However, the major version bumps for most of these dependencies necessitate careful verification to ensure compatibility with the existing codebase.Recommendations:
- Run the provided verification scripts for each major version bump.
- Review the changelogs or release notes for each updated dependency to understand any breaking changes or new features.
- Consider updating the project's documentation to reflect these dependency changes and any necessary code adjustments.
- If possible, run the project's test suite to catch any immediate issues caused by these updates.
📜 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 (3 hunks)
- ovos_core/intent_services/fallback_service.py (4 hunks)
- ovos_core/intent_services/stop_service.py (9 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
286-286: Ambiguous variable name:
l(E741)
🔇 Additional comments (33)
requirements/lgpl.txt (2)
2-2: Verify if fann2 version constraint is still appropriateWhile the
fann2dependency remains unchanged, it's worth considering if it needs an update in light of the other changes in the project.Please verify:
- Is the current version constraint (
>=1.0.7, < 1.1.0) still appropriate for the project's needs?- Are there any newer versions of
fann2that might offer improvements or bug fixes relevant to this project?- Is this version of
fann2compatible with the updated version ofovos_padatious?Run the following script to check the latest version of
fann2and its compatibility:#!/bin/bash # Description: Check the latest version of fann2 and its compatibility # Test: Check the latest version of fann2 available on PyPI echo "Latest version of fann2 on PyPI:" pip index versions fann2 | head -n 1 # Test: Search for fann2 usage in the project echo "Searching for fann2 usage in the project:" rg --type python -i 'from fann2 import|import fann2' # Note: Review these results manually to determine if an update is necessary
1-1: Significant version update for ovos_padatious - verify compatibilityThe update to
ovos_padatious>=1.0.2,<2.0.0is a major version bump, which often implies breaking changes or significant new features. This aligns with the PR objective of updating requirements related to the latest pipeline plugins signature.To ensure smooth integration:
- Verify that this version is compatible with the rest of the project.
- Check if any code changes are required to accommodate this update.
- Review the changelog or release notes for
ovos_padatiousto understand the changes introduced in this major version update.Run the following script to check for potential incompatibilities:
requirements/requirements.txt (5)
13-13: LGTM: Minor version update for ovos-plugin-manager.The update from
>=0.0.26,<1.0.0to>=0.5.0,<1.0.0forovos-plugin-manageris a minor version bump. This change aligns with the PR objectives and should maintain backwards compatibility.
8-8: Verify compatibility with major version update of ovos_ocp_pipeline_plugin.The update from
>=0.1.3,<1.0.0to>=1.0.0,<2.0.0forovos_ocp_pipeline_pluginis a major version bump. This change aligns with the PR objectives to update requirements related to the latest pipeline plugins signature, but it's crucial to ensure compatibility with the rest of the codebase.Please run the following script to check for any potential breaking changes:
#!/bin/bash # Description: Check for usage of ovos_ocp_pipeline_plugin and potential breaking changes # Test: Search for ovos_ocp_pipeline_plugin usage echo "Searching for ovos_ocp_pipeline_plugin usage:" rg --type python 'from ovos_ocp_pipeline_plugin import' -A 5 # Test: Check for any deprecated or removed features echo "Checking for potential breaking changes:" rg --type python 'ovos_ocp_pipeline_plugin\.' -A 5
9-9: Verify compatibility with major version update of ovos-common-query-pipeline-plugin.The update from
>=0.1.4,<1.0.0to>=1.0.0,<2.0.0forovos-common-query-pipeline-pluginis a major version bump. This change aligns with the PR objectives to update requirements related to the latest pipeline plugins signature, but it's crucial to ensure compatibility with the rest of the codebase.Please run the following script to check for any potential breaking changes:
#!/bin/bash # Description: Check for usage of ovos-common-query-pipeline-plugin and potential breaking changes # Test: Search for ovos-common-query-pipeline-plugin usage echo "Searching for ovos-common-query-pipeline-plugin usage:" rg --type python 'from ovos_common_query_pipeline_plugin import' -A 5 # Test: Check for any deprecated or removed features echo "Checking for potential breaking changes:" rg --type python 'ovos_common_query_pipeline_plugin\.' -A 5
7-7: Verify compatibility with major version update of ovos-adapt-parser.The update from
>=0.1.3,<1.0.0to>=1.0.2,<2.0.0forovos-adapt-parseris a major version bump. This change aligns with the PR objectives, but it's crucial to ensure compatibility with the rest of the codebase.Please run the following script to check for any potential breaking changes:
#!/bin/bash # Description: Check for usage of ovos-adapt-parser and potential breaking changes # Test: Search for ovos-adapt-parser usage echo "Searching for ovos-adapt-parser usage:" rg --type python 'from ovos_adapt_parser import' -A 5 # Test: Check for any deprecated or removed features echo "Checking for potential breaking changes:" rg --type python 'ovos_adapt_parser\.' -A 5
6-6: Verify compatibility with major version update of padacioso.The update from
>=0.2.4,<1.0.0to>=1.0.0,<2.0.0forpadaciosois a major version bump. This change aligns with the PR objectives, but it's crucial to ensure compatibility with the rest of the codebase.Please run the following script to check for any potential breaking changes:
test/unittests/test_intent_service.py (3)
Line range hint
1-323: Summary: Changes align with PR objectives, verifications recommended.The changes in this file align with the PR objectives of updating imports and requirements related to the latest pipeline plugins signature. The main changes include:
- Updated import statement for
ContextManager.- Configuration update in the
test_shutdownmethod to disable Padatious.Both changes have been approved, but it's recommended to run the suggested verification scripts to ensure these changes don't negatively impact other parts of the codebase or the test assertions.
Line range hint
323-323: LGTM! Verify the test assertion for removed message types.The
IntentServiceinstantiation now includes a configuration to disable Padatious. This change aligns with the PR objective of updating requirements related to the latest pipeline plugins signature.To ensure the test assertion for removed message types is still accurate, run the following script:
#!/bin/bash # Description: Verify the set of removed message types in the test_shutdown method # Test: Extract the set of removed message types from the test removed_types=$(rg --no-line-number -o "'[^']+'" test/unittests/test_intent_service.py | sort | uniq) # Test: Compare with the actual removed types in the IntentService shutdown method actual_removed=$(rg --no-line-number -o "self\.bus\.remove\('[^']+'" ovos_core/intent_services/intent_service.py | sed "s/self\.bus\.remove('//" | sort | uniq) echo "Removed types in test:" echo "$removed_types" echo "Actual removed types:" echo "$actual_removed" echo "Differences:" diff <(echo "$removed_types") <(echo "$actual_removed")Please review the output and update the test assertion if necessary.
18-18: LGTM! Verify impact on other parts of the codebase.The import statement has been updated to use
IntentContextManagerfromovos_bus_client.sessioninstead ofContextManagerfromovos_adapt.opm. This change maintains backwards compatibility by using an alias.To ensure this change doesn't affect other parts of the codebase, run the following script:
✅ Verification successful
Further Verification Required: Missing File Detected
The script encountered an error locating
ovos_core/intent_services/intent_service.py. Please verify the existence and correct path of this file in the repository.
Action Required: Correct the File Search Command
The previous script used an incorrect flag with
fd. Please use the corrected command below to search forintent_service.py:
Further Verification Needed: Inspect
intent_service.pyforContextManagerUsageTo ensure the import change is correctly applied in
mycroft/skills/intent_service.py, please run the following script:
Verified: No remaining references to
ContextManagerfoundThe import change has been successfully applied. There are no remaining imports or usages of
ContextManagerfromovos_adapt.opminmycroft/skills/intent_service.py.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports of ContextManager from ovos_adapt.opm # Test: Search for old import statement rg --type python "from ovos_adapt.opm import ContextManager" # Test: Search for usage of ContextManager without import rg --type python "ContextManager" -g '!test/unittests/test_intent_service.py'Length of output: 13229
Script:
#!/bin/bash # Description: Search for the 'intent_service.py' file to verify its existence and correct path. fd --type f --name "intent_service.py"Length of output: 289
Script:
#!/bin/bash # Description: Search for the 'intent_service.py' file to verify its existence and correct path. fd "intent_service.py" --type fLength of output: 102
Script:
#!/bin/bash # Description: Verify the import and usage of ContextManager in 'mycroft/skills/intent_service.py' # Check for the new import statement rg "from ovos_bus_client.session import IntentContextManager as ContextManager" mycroft/skills/intent_service.py # Check for any old import statements rg "from ovos_adapt.opm import ContextManager" mycroft/skills/intent_service.py # Check for usage of ContextManager in the file rg "ContextManager" mycroft/skills/intent_service.pyLength of output: 249
ovos_core/intent_services/fallback_service.py (7)
24-24: Import Pipeline Classes for Fallback IntegrationThe addition of
PipelineMatchandPipelinePluginimports is appropriate for integrating pipeline functionality into theFallbackService.
28-28: ImportFallbackModefor Enhanced Configuration HandlingIncluding
FallbackModefromovos_workshop.permissionsenables better handling of fallback modes as per the configuration settings.
Line range hint
165-176: Update Return Type toOptional[PipelineMatch]in_fallback_rangeThe return type of
_fallback_rangehas been updated toOptional[PipelineMatch], reflecting the transition fromIntentMatchtoPipelineMatch. The docstring has also been correctly updated to match this change.
201-204: Update Return Type toOptional[PipelineMatch]inhigh_prioThe return type of
high_priohas been updated toOptional[PipelineMatch], aligning with the changes made in_fallback_range.
206-209: Update Return Type toOptional[PipelineMatch]inmedium_prioThe return type of
medium_priohas been changed toOptional[PipelineMatch], ensuring consistency across the fallback methods.
211-214: Update Return Type toOptional[PipelineMatch]inlow_prioThe return type of
low_priohas been updated toOptional[PipelineMatch], matching the updates in the other priority methods.
196-198: Verify ifmatch_dataShould Be Populated inPipelineMatchThe
PipelineMatchis being returned with an emptymatch_datadictionary. Please verify whethermatch_datashould include relevant match details to ensure proper downstream processing in the pipeline.To investigate how
PipelineMatchinstances are typically constructed, you can run the following script to find usages and see howmatch_datais populated:ovos_core/intent_services/stop_service.py (11)
6-6: Approved: Importing 'closest_match' from 'langcodes'The import of
closest_matchis necessary for the_get_closest_langmethod used in language matching.
12-12: Approved: Import of 'PipelineMatch' and 'PipelinePlugin'The addition of
PipelineMatchandPipelinePluginimports aligns with the updated class inheritance and method return types.
116-116: Approved: Updated return type in 'match_stop_high' methodThe method signature now returns
Optional[PipelineMatch], reflecting the transition to the new pipeline architecture.
125-125: Approved: Updated docstring to reflect new return typeThe docstring correctly states that the method returns a
PipelineMatchif handled, otherwiseNone.
145-148: Approved: Correct instantiation of 'PipelineMatch'The
PipelineMatchis properly constructed with the necessary arguments when emitting a global stop.
158-161: Approved: Return statement updated to use 'PipelineMatch'The method returns a
PipelineMatchinstance with appropriate data when a skill successfully handles the stop request.
164-164: Approved: Updated return type in 'match_stop_medium' methodThe method signature has been updated to return
Optional[PipelineMatch], consistent with the pipeline changes.
174-174: Approved: Updated docstring to reflect new return typeThe docstring now accurately describes the return value as a
PipelineMatchif handled.
204-204: Approved: Updated return type in 'match_stop_low' methodThe method now correctly returns
Optional[PipelineMatch], aligning with the updated architecture.
213-213: Approved: Updated docstring to reflect new return typeThe docstring correctly states that the method returns a
PipelineMatchif handled, otherwiseNone.
237-240: Approved: Return statement uses 'PipelineMatch' with appropriate dataThe
PipelineMatchis instantiated correctly when a skill handles the stop request.ovos_core/intent_services/converse_service.py (3)
10-10: ImportedPipelineMatchandPipelinePluginThe added import statement brings in
PipelineMatchandPipelinePlugin, which are necessary for the updated return types and class inheritance.
14-14: ImportedConverseModeandConverseActivationModeThese imports are required for handling conversation permissions and activation modes within the
ConverseService.
315-315: Updated return type toOptional[PipelineMatch]The method
converse_with_skillsnow returns anOptional[PipelineMatch]instead ofOptional[IntentMatch], aligning with the updated pipeline structure.ovos_core/intent_services/__init__.py (2)
621-624:⚠️ Potential issueVerify attribute availability in both match types
When accessing
match.match_typeandmatch.match_data, ensure these attributes are present in bothIntentHandlerMatchandPipelineMatchinstances. Missing attributes might lead to exceptions during execution.Refer to the previous comments regarding attribute checks and modifications to ensure consistency across match types.
399-402:⚠️ Potential issueEnsure all match types have the required attributes
Since
matchcan be an instance of eitherIntentHandlerMatchorPipelineMatch, it's important to ensure that both classes have thematch_dataandmatch_typeattributes used in this method. Accessing missing attributes could lead toAttributeErrorat runtime.To verify that both classes have the necessary attributes, you can run:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
ovos_core/intent_services/stop_service.py (1)
204-204: LGTM: Method updated to use PipelineMatch. Consider removing redundant comment.The
match_stop_lowmethod has been successfully updated to usePipelineMatchinstead ofIntentMatch. The changes are consistent throughout the method, and the existing functionality has been preserved.However, the comment on line 239 ("emit instead of intent message") is now redundant and potentially confusing. Consider removing this comment to improve code clarity.
Also applies to: 213-213, 237-240, 244-248
ovos_core/intent_services/__init__.py (3)
286-293: LGTM with a minor suggestion: Improve variable naming in list comprehension.The use of
standardize_lang_taginstead ofget_full_lang_codeis a good improvement in language handling. However, the single-letter variablelin the list comprehension could be made more descriptive for better readability.Consider renaming the variable for clarity:
-valid_langs = [standardize_lang_tag(l) for l in valid_langs] +valid_langs = [standardize_lang_tag(lang) for lang in valid_langs]🧰 Tools
🪛 Ruff
286-286: Ambiguous variable name:
l(E741)
Line range hint
376-402: LGTM with a suggestion: Update docstring to reflect new parameter type.The changes to
_emit_match_messageto handle bothIntentHandlerMatchandPipelineMatchobjects are good improvements, expanding the flexibility of the intent matching system.Please update the docstring to reflect the new parameter type:
def _emit_match_message(self, match: Union[IntentHandlerMatch, PipelineMatch], message: Message): """Update the message data with the matched utterance information and activate the corresponding skill if available. Args: - match (IntentHandlerMatch): The matched utterance object. + match (Union[IntentHandlerMatch, PipelineMatch]): The matched utterance object. message (Message): The messagebus data. """
486-488: LGTM with a suggestion: Add a safety check for attribute access.The change to use
match.match_typeinstead ofmatch.intent_typealigns with the newUnion[IntentHandlerMatch, PipelineMatch]type. However, there's a potential forAttributeErrorifmatchdoesn't have amatch_typeattribute.Consider adding a safety check:
- if isinstance(match, IntentHandlerMatch) and match.match_type in sess.blacklisted_intents: + if isinstance(match, IntentHandlerMatch) and hasattr(match, 'match_type') and match.match_type in sess.blacklisted_intents:This change will prevent potential
AttributeErrors ifmatchdoesn't have amatch_typeattribute.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- ovos_core/intent_services/init.py (12 hunks)
- ovos_core/intent_services/stop_service.py (9 hunks)
🧰 Additional context used
🪛 Ruff
ovos_core/intent_services/__init__.py
286-286: Ambiguous variable name:
l(E741)
🔇 Additional comments (10)
ovos_core/intent_services/stop_service.py (6)
12-12: LGTM: Import statement updated correctly.The addition of
PipelineMatchandPipelinePluginimports is consistent with the changes in the method signatures and aligns with the PR objective of updating imports related to the latest pipeline plugins signature.
116-116: LGTM: Method updated to use PipelineMatch.The
match_stop_highmethod has been successfully updated to usePipelineMatchinstead ofIntentMatch. The changes are consistent throughout the method, and the existing functionality has been preserved.Also applies to: 125-125, 145-148, 158-161
164-164: LGTM: Method signature updated correctly.The
match_stop_mediummethod signature has been properly updated to returnOptional[PipelineMatch]. The method body remains unchanged, which is appropriate as it delegates tomatch_stop_lowfor the actual matching logic.Also applies to: 174-174
Line range hint
21-21: Verify: Should StopService inherit from PipelinePlugin?The PR objectives mention updating to the latest pipeline plugins signature, but the
StopServiceclass doesn't seem to inherit fromPipelinePlugin. Please verify if this is intentional or if the class should be updated to inherit fromPipelinePlugin.If it should inherit from
PipelinePlugin, consider updating the class definition as follows:class StopService(PipelinePlugin):This change would align the class with the latest pipeline plugins signature mentioned in the PR objectives.
Line range hint
1-290: Overall: Changes are consistent, but verify complete alignment with pipeline plugins signatureThe changes in this file consistently update the usage of
IntentMatchtoPipelineMatchacross all relevant methods. The existing functionality is preserved while adapting to the new structure. These updates align with the PR objectives of updating imports and requirements related to the latest pipeline plugins signature.However, to ensure full alignment with the latest pipeline plugins signature:
- Verify if the
StopServiceclass should inherit fromPipelinePlugin.- Double-check if there are any other pipeline-related updates required in this file or elsewhere in the project.
These verifications will help ensure that the PR fully meets its objectives and that the
StopServiceis correctly integrated with the latest pipeline plugin architecture.
145-148: 🛠️ Refactor suggestionSuggestion: Include 'conf' in 'match_data' for consistency
For consistency with other return statements, consider including the confidence score
confin thematch_datafield when returningPipelineMatchafter emitting the global stop.Apply this change:
return PipelineMatch(handled=True, - match_data={}, + match_data={"conf": conf}, skill_id=None, utterance=utterance)This change will make the return value consistent with other
PipelineMatchinstances in the class.ovos_core/intent_services/__init__.py (4)
15-18: LGTM: Import and type hint updates enhance functionality and type safety.The addition of
Unionfromtypingand the import ofOCPPipelineMatchersuggest an expansion of intent matching capabilities. The updated type hint for_emit_match_messageimproves type safety. These changes are beneficial for maintainability and extensibility of the codebase.Also applies to: 29-33
Line range hint
353-373: LGTM: Improved session validation logic.The conversion of
_validate_sessionto a static method and the use ofstandardize_lang_tagfor language standardization are good improvements. These changes enhance the consistency of language handling throughout the codebase.
Line range hint
562-600: LGTM: Improved context handling with static methods.The conversion of
handle_add_context,handle_remove_context, andhandle_clear_contextto static methods is a good improvement. This change enhances code organization and potentially improves performance by eliminating unnecessary instance method calls.
621-624: LGTM: Consistent updates to intent handling.The changes in
handle_get_intentto usematch.match_typeandmatch.match_dataare consistent with the earlier updates to the matching system. These modifications ensure that the method works correctly with the newUnion[IntentHandlerMatch, PipelineMatch]type.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #571 +/- ##
==========================================
- Coverage 75.33% 72.86% -2.48%
==========================================
Files 15 15
Lines 3094 1537 -1557
==========================================
- Hits 2331 1120 -1211
+ Misses 763 417 -346
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
mycroft/skills/intent_services/__init__.py (2)
Line range hint
1-14: Summary of changes and potential impactThe changes in this file reflect a significant restructuring of the project's dependencies and import paths. Overall, these changes appear to improve modularity and organization by moving services and classes to more specific packages.
Key points:
- Services like AdaptService, PadaciosoService, and CommonQAService are now imported from their respective packages rather than from ovos_core.intent_services.
- IntentMatch is now imported from ovos_plugin_manager.templates.pipeline.
- Intent-related classes (AdaptIntent, IntentBuilder, Intent) are now imported from ovos_workshop.intents.
- A fallback mechanism is implemented for PadatiousService.
These changes may have the following impacts:
- Improved modularity and separation of concerns in the project structure.
- Potential need for updates in documentation and dependent code to reflect new import paths.
- Possible subtle changes in behavior due to the fallback mechanism for PadatiousService.
Recommendations:
- Thoroughly test the system to ensure all services work as expected with the new import structure.
- Update project documentation to reflect the new package structure and import paths.
- Review and update any code that depends on these imports throughout the project.
- Consider addressing the unused imports flagged by the static analysis tool.
To maintain the benefits of this restructuring:
- Establish clear guidelines for managing dependencies and import structures in the project.
- Regularly review and update dependencies to ensure they align with the project's architectural goals.
- Consider implementing automated tests to verify the correct functioning of all services after structural changes.
🧰 Tools
🪛 Ruff
1-1:
ovos_core.intent_services.fallback_service.FallbackServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
ovos_core.intent_services.converse_service.ConverseServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
ovos_adapt.opm.AdaptPipelineimported but unused(F401)
4-4:
padacioso.opm.PadaciosoPipelineimported but unused(F401)
5-5:
ovos_commonqa.opm.CommonQAServiceimported but unused(F401)
6-6:
ovos_plugin_manager.templates.pipeline.IntentMatchimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
7-7:
ovos_workshop.intents.IntentBuilderimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
10-10:
ovos_padatious.opm.PadatiousMatcherimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
Remove Unused
PadatiousMatcherImportThe import of
PadatiousMatcherinmycroft/skills/intent_services/__init__.pydoes not appear to be used within the module. Consider removing it to clean up the codebase.Additionally, please manually verify the compatibility between
PadatiousServiceandPadaciosoServiceto ensure consistent behavior across different installations.🔗 Analysis chain
Line range hint
10-14: Approve fallback mechanism and suggest verificationThe try-except block provides a good fallback mechanism if ovos_padatious is not installed. This approach ensures that the code can still function even if the preferred package is unavailable.
However, there are a few points to consider:
Verify that PadaciosoService (used as a fallback) is fully compatible with PadatiousService to ensure consistent behavior across different installations.
The import of PadatiousMatcher appears to be unused. Consider removing it if it's not needed elsewhere in the module.
To ensure the fallback mechanism works as intended and to verify the usage of PadatiousService, run the following script:
If PadatiousMatcher is needed, verify its usage with:
Based on the results, update the imports accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of PadatiousService and PadaciosoService echo "Checking usage of PadatiousService:" rg --type python "PadatiousService" -C 3 echo "Checking usage of PadaciosoService:" rg --type python "PadaciosoService" -C 3Length of output: 400
Script:
#!/bin/bash # Description: Check the usage of PadatiousMatcher echo "Checking usage of PadatiousMatcher:" rg --type python "PadatiousMatcher" -C 3Length of output: 152
Script:
#!/bin/bash # Description: Check the usage of PadatiousService and PadaciosoService echo "Checking usage of PadatiousService:" rg --type py "PadatiousService" -C 3 echo "Checking usage of PadaciosoService:" rg --type py "PadaciosoService" -C 3Length of output: 4979
Script:
#!/bin/bash # Description: Check the usage of PadatiousMatcher echo "Checking usage of PadatiousMatcher:" rg --type py "PadatiousMatcher" -C 3Length of output: 1351
🧰 Tools
🪛 Ruff
1-1:
ovos_core.intent_services.fallback_service.FallbackServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
ovos_core.intent_services.converse_service.ConverseServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
ovos_adapt.opm.AdaptPipelineimported but unused(F401)
4-4:
padacioso.opm.PadaciosoPipelineimported but unused(F401)
5-5:
ovos_commonqa.opm.CommonQAServiceimported but unused(F401)
6-6:
ovos_plugin_manager.templates.pipeline.IntentMatchimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
7-7:
ovos_workshop.intents.IntentBuilderimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
10-10:
ovos_padatious.opm.PadatiousMatcherimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
test/unittests/test_intent_service.py (2)
134-142: LGTM! Consider using a list comprehension for message deserialization.The changes to use FakeBus and the custom message handler look good. They provide a more standardized and efficient way to mock the bus and track emitted messages.
Consider using a list comprehension for message deserialization in the
on_msgfunction:- def on_msg(m): - self.bus.emitted_msgs.append(Message.deserialize(m)) + self.bus.on("message", lambda m: self.bus.emitted_msgs.append(Message.deserialize(m)))This change would make the code more concise and Pythonic.
192-192: LGTM! Consider refactoring repeated assertions.The changes to use
self.bus.emitted_msgs[-1]are consistent across all test methods and align with the new FakeBus implementation.To reduce code repetition and improve maintainability, consider refactoring the repeated assertions into a helper method:
def assert_last_emitted_message(self, expected_data): self.assertEqual(self.bus.emitted_msgs[-1].data, expected_data)Then, you can use this helper method in your tests:
self.assert_last_emitted_message({'intent': None})This refactoring would make the tests more concise and easier to maintain.
Also applies to: 202-202, 210-210, 218-218, 233-233, 245-245
ovos_core/intent_services/__init__.py (4)
286-286: Improve readability by renaming the variable 'l'The use of 'l' as a variable name can be confusing as it may be mistaken for the number '1' or capital 'I'. Consider renaming it to a more descriptive name, such as 'lang_code'.
Apply this diff to improve readability:
-valid_langs = [standardize_lang_tag(l) for l in valid_langs] +valid_langs = [standardize_lang_tag(lang_code) for lang_code in valid_langs]🧰 Tools
🪛 Ruff
286-286: Ambiguous variable name:
l(E741)
Line range hint
376-402: LGTM: Updated _emit_match_message to handle new match typesThe changes in this method successfully accommodate both IntentHandlerMatch and PipelineMatch types, aligning with the transition mentioned in the PR summary. This improves the flexibility of the method.
Consider updating the method's docstring to reflect the new parameter type:
- def _emit_match_message(self, match: IntentHandlerMatch, message: Message): + def _emit_match_message(self, match: Union[IntentHandlerMatch, PipelineMatch], message: Message): """Update the message data with the matched utterance information and activate the corresponding skill if available. Args: - match (IntentHandlerMatch): The matched utterance object. + match (Union[IntentHandlerMatch, PipelineMatch]): The matched utterance object. message (Message): The messagebus data. """
486-488: LGTM: Improved type checking in handle_utteranceThe changes enhance type safety by using
isinstancechecks and make the blacklisted intents check more specific toIntentHandlerMatchtypes.For consistency with the previous checks, consider using
hasattrto check for thematch_typeattribute:- if isinstance(match, IntentHandlerMatch) and match.match_type in sess.blacklisted_intents: + if isinstance(match, IntentHandlerMatch) and hasattr(match, 'match_type') and match.match_type in sess.blacklisted_intents:This change would make the code more robust against potential future changes in the
IntentHandlerMatchclass.
621-624: LGTM: Improved intent data handling in handle_get_intentThe changes in
handle_get_intentmethod align well with the newPipelineMatchandIntentHandlerMatchstructures. The intent data is now more consistently structured and includes valuable additional information.For added robustness, consider adding a check for the existence of
match_data:if match: - if match.match_type: + if match.match_type and hasattr(match, 'match_data'): intent_data = match.match_data intent_data["intent_name"] = match.match_type intent_data["intent_service"] = pipelineThis change would prevent potential AttributeError if
match_datais not present in future match objects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- mycroft/skills/intent_services/init.py (1 hunks)
- mycroft/skills/intent_services/adapt_service.py (1 hunks)
- ovos_core/intent_services/init.py (13 hunks)
- test/unittests/test_intent_service.py (9 hunks)
🧰 Additional context used
🪛 Ruff
mycroft/skills/intent_services/__init__.py
1-1:
ovos_core.intent_services.fallback_service.FallbackServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
ovos_core.intent_services.converse_service.ConverseServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
ovos_adapt.opm.AdaptPipelineimported but unused(F401)
4-4:
padacioso.opm.PadaciosoPipelineimported but unused(F401)
5-5:
ovos_commonqa.opm.CommonQAServiceimported but unused(F401)
6-6:
ovos_plugin_manager.templates.pipeline.IntentMatchimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
7-7:
ovos_workshop.intents.IntentBuilderimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
10-10:
ovos_padatious.opm.PadatiousMatcherimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
mycroft/skills/intent_services/adapt_service.py
18-18:
ovos_adapt.opm.AdaptPipelineimported but unusedRemove unused import:
ovos_adapt.opm.AdaptPipeline(F401)
19-19:
ovos_bus_client.session.IntentContextManagerFrameimported but unusedRemove unused import
(F401)
20-20:
ovos_bus_client.session.IntentContextManagerimported but unusedRemove unused import
(F401)
ovos_core/intent_services/__init__.py
286-286: Ambiguous variable name:
l(E741)
🔇 Additional comments (12)
mycroft/skills/intent_services/__init__.py (3)
5-7: Approve updated import paths and suggest documentation reviewThe changes to import paths for CommonQAService, IntentMatch, AdaptIntent, IntentBuilder, and Intent are good. They align with the overall restructuring of the project, improving modularity and organization.
The move of AdaptIntent, IntentBuilder, and Intent from a local import to ovos_workshop.intents is particularly noteworthy and may require updates to documentation or dependent code.
To address the static analysis hints about unused imports and ensure these changes don't break existing functionality, let's verify their usage:
#!/bin/bash # Description: Check if the imported classes are used in the codebase echo "Checking usage of CommonQAService:" rg --type python "CommonQAService" -C 3 echo "Checking usage of IntentMatch:" rg --type python "IntentMatch" -C 3 echo "Checking usage of AdaptIntent:" rg --type python "AdaptIntent" -C 3 echo "Checking usage of IntentBuilder:" rg --type python "IntentBuilder" -C 3 echo "Checking usage of Intent:" rg --type python "Intent" -C 3If these classes are used elsewhere in the codebase, we can safely ignore the static analysis warnings. If they're not used, consider removing these imports or adding them to
__all__if they're meant to be part of the public API.Additionally, please review and update any documentation that references these classes to reflect their new import paths.
🧰 Tools
🪛 Ruff
5-5:
ovos_commonqa.opm.CommonQAServiceimported but unused(F401)
6-6:
ovos_plugin_manager.templates.pipeline.IntentMatchimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
7-7:
ovos_workshop.intents.IntentBuilderimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
3-4: Approve updated import paths for AdaptService and PadaciosoServiceThe changes to import AdaptService from ovos_adapt.opm and PadaciosoService from padacioso.opm are good. This restructuring likely improves modularity and separation of concerns in the project.
The use of aliases (AdaptPipeline as AdaptService and PadaciosoPipeline as PadaciosoService) maintains backward compatibility, which is a good practice.
To address the static analysis hints about unused imports and ensure these changes don't break existing functionality, let's verify their usage:
#!/bin/bash # Description: Check if AdaptService and PadaciosoService are used in the codebase echo "Checking usage of AdaptService:" rg --type python "AdaptService" -C 3 echo "Checking usage of PadaciosoService:" rg --type python "PadaciosoService" -C 3If these services are used elsewhere in the codebase, we can safely ignore the static analysis warnings. If they're not used, consider removing these imports or adding them to
__all__if they're meant to be part of the public API.🧰 Tools
🪛 Ruff
3-3:
ovos_adapt.opm.AdaptPipelineimported but unused(F401)
4-4:
padacioso.opm.PadaciosoPipelineimported but unused(F401)
1-2: Approve specific imports for FallbackService and ConverseServiceThe changes to import FallbackService and ConverseService from their specific submodules in ovos_core.intent_services are good. This approach makes the imports more explicit and can potentially improve code organization.
To address the static analysis hints about unused imports, let's verify their usage:
If these services are indeed used elsewhere in the codebase, we can safely ignore the static analysis warnings. If they're not used, consider removing these imports or adding them to
__all__if they're meant to be part of the public API.✅ Verification successful
Confirmed usage of FallbackService and ConverseService
The shell script results confirm that both
FallbackServiceandConverseServiceare actively used in the codebase. The static analysis warnings about unused imports are false positives and can be safely ignored.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if FallbackService and ConverseService are used in the codebase echo "Checking usage of FallbackService:" rg --type python "FallbackService" -C 3 echo "Checking usage of ConverseService:" rg --type python "ConverseService" -C 3Length of output: 370
Script:
#!/bin/bash # Description: Corrected script to check if FallbackService and ConverseService are used in the codebase echo "Checking usage of FallbackService:" rg "FallbackService" --type py -C 3 echo "Checking usage of ConverseService:" rg "ConverseService" --type py -C 3Length of output: 10789
🧰 Tools
🪛 Ruff
1-1:
ovos_core.intent_services.fallback_service.FallbackServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
ovos_core.intent_services.converse_service.ConverseServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
mycroft/skills/intent_services/adapt_service.py (1)
18-20: Verify the necessity of newly added importsThe following imports have been added but are flagged as unused by static analysis tools:
AdaptPipelineasAdaptServicefromovos_adapt.opmIntentContextManagerFrameasContextManagerFramefromovos_bus_client.sessionIntentContextManagerasContextManagerfromovos_bus_client.sessionPlease verify if these imports are necessary for the changes implemented in this file or other parts of the codebase. If they are not required, consider removing them to keep the imports clean and avoid potential confusion.
If you confirm that these imports are indeed unnecessary, I can assist in removing them. Would you like me to propose a fix?
To verify the usage of these imports across the codebase, you can run the following script:
This script will search for occurrences of these imported names across all Python files in the repository. The results will help determine if these imports are used elsewhere in the codebase.
🧰 Tools
🪛 Ruff
18-18:
ovos_adapt.opm.AdaptPipelineimported but unusedRemove unused import:
ovos_adapt.opm.AdaptPipeline(F401)
19-19:
ovos_bus_client.session.IntentContextManagerFrameimported but unusedRemove unused import
(F401)
20-20:
ovos_bus_client.session.IntentContextManagerimported but unusedRemove unused import
(F401)
test/unittests/test_intent_service.py (5)
159-159: LGTM! Consistent with new FakeBus usage.The change to use
self.bus.emitted_msgs[-1]is consistent with the new FakeBus implementation and simplifies the code.
170-170: LGTM! Consistent with new FakeBus usage.The change to use
self.bus.emitted_msgs[-1]is consistent with the new FakeBus implementation and the changes in other test methods.
181-181: LGTM! Consistent with new FakeBus usage.The change to use
self.bus.emitted_msgs[-1]is consistent with the new FakeBus implementation and the changes in other test methods.
Line range hint
1-258: Overall, the changes look good and align with the PR objectives.The updates to the test file
test/unittests/test_intent_service.pyare well-implemented and consistent. The transition to FakeBus improves the testing approach and aligns with the project's evolution. The changes in import statements and test methods are coherent and maintain the functionality of the tests.A few suggestions for further improvement:
- Consider using a list comprehension for message deserialization in the setUp method.
- Refactor the repeated assertions into a helper method to reduce code repetition and improve maintainability.
These minor improvements would enhance the code quality and make it more Pythonic.
18-18: LGTM! Verify usage of new imports.The import changes look good. The ContextManager import has been updated, and FakeBus has been added. These changes align with the PR objectives of updating imports.
Please run the following script to verify the usage of these new imports:
Also applies to: 23-23
✅ Verification successful
Import changes verified successfully.
The old import
from ovos_adapt.opm import ContextManagerhas been successfully removed. The new importsIntentContextManagerandFakeBusare correctly implemented and utilized across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of new imports # Test: Check for any remaining old imports of ContextManager rg --type python "from ovos_adapt.opm import ContextManager" # Test: Verify usage of IntentContextManager rg --type python "IntentContextManager" # Test: Verify usage of FakeBus rg --type python "FakeBus"Length of output: 222
Script:
#!/bin/bash # Description: Verify usage of new imports with corrected file type # Test: Check for any remaining old imports of ContextManager rg --type py "from ovos_adapt.opm import ContextManager" # Test: Verify usage of IntentContextManager rg --type py "IntentContextManager" # Test: Verify usage of FakeBus rg --type py "FakeBus"Length of output: 1785
Script:
#!/bin/bash # Description: Confirm removal of old imports and correct usage of new imports # Test: Ensure no remaining old imports of ContextManager from ovos_adapt.opm rg --type py "^from ovos_adapt\.opm import ContextManager$" # Test: Verify all imports of IntentContextManager are from ovos_bus_client.session rg --type py "^from ovos_bus_client\.session import IntentContextManager" # Test: Verify all imports of FakeBus are from ovos_utils.fakebus rg --type py "^from ovos_utils\.fakebus import FakeBus"Length of output: 521
ovos_core/intent_services/__init__.py (3)
15-33: LGTM: Import changes align with PR objectivesThe import changes reflect the updates to the latest pipeline plugins signature as mentioned in the PR objectives. The new imports and reorganization of existing ones improve the code structure and dependencies.
532-532: LGTM: Improved language handling in handle_register_vocabThe addition of the
langparameter to_adapt_service.register_vocabularyenhances multi-language support by ensuring the correct language is used when registering vocabulary.
Line range hint
562-600: LGTM: Improved session handling in context methodsThe changes to
handle_add_context,handle_remove_context, andhandle_clear_contextmethods are well-implemented:
- Converting them to static methods improves the design by reducing coupling.
- Using SessionManager for session handling aligns with best practices and likely improves consistency across the codebase.
These changes contribute to better maintainability and potentially easier testing of these methods.
update imports and requirements for latest pipeline plugins signature
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
mycroft/skills/intent_services/__init__.py (4)
1-2: Consider addressing unused imports.The static analysis tool indicates that
FallbackServiceandConverseServiceare imported but not used in this file. Consider one of the following actions:
- If these services are intended to be part of the public API of this module, add them to the
__all__list.- If they are imported for convenience in other parts of the codebase, consider adding a comment explaining this intention.
- If they are truly unused, consider removing these imports to keep the codebase clean.
🧰 Tools
🪛 Ruff
1-1:
ovos_core.intent_services.fallback_service.FallbackServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
ovos_core.intent_services.converse_service.ConverseServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
6-6: Consider addressing unused IntentMatch import.The import of
IntentMatchhas been updated to come fromovos_plugin_manager.templates.pipeline, which aligns with the PR objectives. However, the static analysis tool indicates that this import is unused in this file. Consider one of the following actions:
- If
IntentMatchis intended to be part of the public API of this module, add it to the__all__list.- If it is imported for convenience in other parts of the codebase, consider adding a comment explaining this intention.
- If it is truly unused, consider removing this import to keep the codebase clean.
🧰 Tools
🪛 Ruff
6-6:
ovos_plugin_manager.templates.pipeline.IntentMatchimported but unused(F401)
7-7: Consider addressing unused imports from ovos_workshop.intents.The import of
Intent,AdaptIntent, andIntentBuilderhas been updated to come fromovos_workshop.intents, which aligns with the PR objectives. However, the static analysis tool indicates that these imports are unused in this file. Consider the following actions:
- If these classes are intended to be part of the public API of this module, add them to the
__all__list.- If they are imported for convenience in other parts of the codebase, consider adding a comment explaining this intention.
- If they are truly unused, consider removing these imports to keep the codebase clean.
Additionally, note that
Intentis imported twice (once asAdaptIntentand once asIntent). This might be intentional, but it's worth verifying if both imports are necessary.🧰 Tools
🪛 Ruff
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
7-7:
ovos_workshop.intents.IntentBuilderimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
Line range hint
1-14: Overall import updates look good, but consider a general cleanup.The changes to the import statements align well with the PR objectives of updating to the latest pipeline plugins signature. The new import paths reflect the restructuring of the codebase and the transition to using the latest versions of various services.
However, there's a consistent pattern of unused imports throughout the file. To improve code cleanliness and maintainability, consider performing a general cleanup of the imports:
- Review each import and determine if it's necessary for this file or if it's imported for use in other parts of the codebase.
- For imports that are part of the public API of this module, add them to the
__all__list.- For imports that are used elsewhere but not in this file, consider adding a comment explaining their purpose.
- Remove any truly unused imports.
This cleanup will make the file's purpose clearer and reduce potential confusion for future developers working on this code.
🧰 Tools
🪛 Ruff
1-1:
ovos_core.intent_services.fallback_service.FallbackServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
ovos_core.intent_services.converse_service.ConverseServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
ovos_adapt.opm.AdaptPipelineimported but unused(F401)
4-4:
padacioso.opm.PadaciosoPipelineimported but unused(F401)
5-5:
ovos_commonqa.opm.CommonQAServiceimported but unused(F401)
6-6:
ovos_plugin_manager.templates.pipeline.IntentMatchimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
7-7:
ovos_workshop.intents.IntentBuilderimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
10-10:
ovos_padatious.opm.PadatiousMatcherimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
ovos_core/intent_services/__init__.py (3)
286-286: Improve variable naming for better readabilityWhile the change to
standardize_lang_tagis good, the use of the single-letter variablelin the list comprehension can be improved for better readability. Consider renaming it to a more descriptive name likelang_code.Apply this diff to enhance code clarity:
-valid_langs = [standardize_lang_tag(l) for l in valid_langs] +valid_langs = [standardize_lang_tag(lang_code) for lang_code in valid_langs]🧰 Tools
🪛 Ruff
286-286: Ambiguous variable name:
l(E741)
Line range hint
376-402: LGTM! Improved handling of different match typesThe changes to
_emit_match_messagereflect a more flexible intent matching system that can handle bothIntentHandlerMatchandPipelineMatchobjects. The new checks ensure appropriate handling for each type of match.Consider updating the method's docstring to reflect the new parameter type:
def _emit_match_message(self, match: Union[IntentHandlerMatch, PipelineMatch], message: Message): """Update the message data with the matched utterance information and activate the corresponding skill if available. Args: - match (IntentHandlerMatch): The matched utterance object. + match (Union[IntentHandlerMatch, PipelineMatch]): The matched utterance object. message (Message): The messagebus data. """
486-488: LGTM! Improved blacklist checking for IntentHandlerMatchThe addition of the
isinstance(match, IntentHandlerMatch)check ensures that the blacklist check is only performed forIntentHandlerMatchobjects, which is appropriate asPipelineMatchobjects might not have amatch_typeattribute.For improved clarity, consider using the
hasattrfunction to check for thematch_typeattribute:- if isinstance(match, IntentHandlerMatch) and match.match_type in sess.blacklisted_intents: + if hasattr(match, 'match_type') and match.match_type in sess.blacklisted_intents:This change would make the code more robust against potential future changes in the
IntentHandlerMatchorPipelineMatchclasses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- mycroft/skills/intent_services/init.py (1 hunks)
- mycroft/skills/intent_services/adapt_service.py (1 hunks)
- ovos_core/intent_services/init.py (13 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 (9 hunks)
- requirements/lgpl.txt (1 hunks)
- requirements/requirements.txt (1 hunks)
- test/unittests/test_intent_service.py (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ovos_core/intent_services/stop_service.py
- requirements/lgpl.txt
- requirements/requirements.txt
🧰 Additional context used
🪛 Ruff
mycroft/skills/intent_services/__init__.py
1-1:
ovos_core.intent_services.fallback_service.FallbackServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
ovos_core.intent_services.converse_service.ConverseServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
ovos_adapt.opm.AdaptPipelineimported but unused(F401)
4-4:
padacioso.opm.PadaciosoPipelineimported but unused(F401)
5-5:
ovos_commonqa.opm.CommonQAServiceimported but unused(F401)
6-6:
ovos_plugin_manager.templates.pipeline.IntentMatchimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
7-7:
ovos_workshop.intents.IntentBuilderimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
10-10:
ovos_padatious.opm.PadatiousMatcherimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
mycroft/skills/intent_services/adapt_service.py
18-18:
ovos_adapt.opm.AdaptPipelineimported but unusedRemove unused import:
ovos_adapt.opm.AdaptPipeline(F401)
19-19:
ovos_bus_client.session.IntentContextManagerFrameimported but unusedRemove unused import
(F401)
20-20:
ovos_bus_client.session.IntentContextManagerimported but unusedRemove unused import
(F401)
ovos_core/intent_services/__init__.py
286-286: Ambiguous variable name:
l(E741)
🔇 Additional comments (19)
mycroft/skills/intent_services/__init__.py (2)
3-5: Verify correct versions and consider addressing unused imports.The import paths for AdaptService, PadaciosoService, and CommonQAService have been updated, which aligns with the PR objectives. However, the static analysis tool indicates these imports are unused in this file. Consider the following:
- Verify that the imported versions match the ones specified in the companion PRs mentioned in the PR description.
- If these services are intended to be part of the public API of this module, add them to the
__all__list.- If they are imported for convenience in other parts of the codebase, consider adding a comment explaining this intention.
- If they are truly unused, consider removing these imports to keep the codebase clean.
To verify the correct versions are being imported, you can run:
🧰 Tools
🪛 Ruff
3-3:
ovos_adapt.opm.AdaptPipelineimported but unused(F401)
4-4:
padacioso.opm.PadaciosoPipelineimported but unused(F401)
5-5:
ovos_commonqa.opm.CommonQAServiceimported but unused(F401)
Line range hint
9-14: Consider addressing unused PadatiousMatcher import and verify fallback import.The import of
PadatiousServicehas been updated to first try importing fromovos_padatious.opm, with a fallback toovos_core.intent_services.padacioso_service. This change aligns with the PR objectives and provides backward compatibility. However, there are a couple of points to consider:
The static analysis tool indicates that
PadatiousMatcheris imported but unused. If it's not needed, consider removing it from the import statement.Verify that the fallback import is still necessary. If
ovos_padatiousis now a required dependency, you might be able to simplify this import by removing the try-except block.To verify if
ovos_padatiousis installed and its version, you can run:If
ovos_padatiousis always expected to be present, consider simplifying the import to:from ovos_padatious.opm import PadatiousPipeline as PadatiousService🧰 Tools
🪛 Ruff
1-1:
ovos_core.intent_services.fallback_service.FallbackServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
ovos_core.intent_services.converse_service.ConverseServiceimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
ovos_adapt.opm.AdaptPipelineimported but unused(F401)
4-4:
padacioso.opm.PadaciosoPipelineimported but unused(F401)
5-5:
ovos_commonqa.opm.CommonQAServiceimported but unused(F401)
6-6:
ovos_plugin_manager.templates.pipeline.IntentMatchimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
7-7:
ovos_workshop.intents.IntentBuilderimported but unused(F401)
7-7:
ovos_workshop.intents.Intentimported but unused(F401)
10-10:
ovos_padatious.opm.PadatiousMatcherimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
mycroft/skills/intent_services/adapt_service.py (1)
18-20: Review import changes and verify usageThe import changes align with the PR objectives of updating to the latest pipeline plugins signature. However, there are a few points to consider:
The transition from
ovos_adapttoovos_bus_clientfor context management is consistent with the intended updates.The static analysis tool suggests that these imports are unused:
AdaptPipeline(aliased asAdaptService)IntentContextManagerFrame(aliased asContextManagerFrame)IntentContextManager(aliased asContextManager)Please verify if these imports are used elsewhere in the file. If they are indeed unused, consider removing them to keep the imports clean. Run the following script to check for their usage:
- The alias
AdaptServiceforAdaptPipelinemight indicate a refactoring intention. Ensure this aligns with the overall project direction.If these imports are confirmed to be unused, you can remove them as follows:
-from ovos_adapt.opm import AdaptPipeline as AdaptService -from ovos_bus_client.session import IntentContextManagerFrame as ContextManagerFrame, \ - IntentContextManager as ContextManager✅ Verification successful
Remove unused import statements
The imports
AdaptService,ContextManagerFrame, andContextManagerare not used anywhere inadapt_service.py. Removing these unused imports will help keep the codebase clean and maintainable.- from ovos_adapt.opm import AdaptPipeline as AdaptService - from ovos_bus_client.session import IntentContextManagerFrame as ContextManagerFrame, \ - IntentContextManager as ContextManager🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of potentially unused imports # Test: Search for usage of AdaptService, ContextManagerFrame, and ContextManager echo "Searching for AdaptService usage:" rg --type python 'AdaptService' mycroft/skills/intent_services/adapt_service.py echo "Searching for ContextManagerFrame usage:" rg --type python 'ContextManagerFrame' mycroft/skills/intent_services/adapt_service.py echo "Searching for ContextManager usage:" rg --type python 'ContextManager' mycroft/skills/intent_services/adapt_service.pyLength of output: 590
Script:
#!/bin/bash # Description: Check for usage of potentially unused imports # Test: Search for usage of AdaptService, ContextManagerFrame, and ContextManager in the specified file. echo "Searching for AdaptService usage:" rg 'AdaptService' mycroft/skills/intent_services/adapt_service.py echo "Searching for ContextManagerFrame usage:" rg 'ContextManagerFrame' mycroft/skills/intent_services/adapt_service.py echo "Searching for ContextManager usage:" rg 'ContextManager' mycroft/skills/intent_services/adapt_service.pyLength of output: 731
🧰 Tools
🪛 Ruff
18-18:
ovos_adapt.opm.AdaptPipelineimported but unusedRemove unused import:
ovos_adapt.opm.AdaptPipeline(F401)
19-19:
ovos_bus_client.session.IntentContextManagerFrameimported but unusedRemove unused import
(F401)
20-20:
ovos_bus_client.session.IntentContextManagerimported but unusedRemove unused import
(F401)
ovos_core/intent_services/fallback_service.py (6)
199-202: LGTM! Correct usage of PipelineMatch.The
PipelineMatchobject is being correctly instantiated and returned, which is consistent with the transition fromIntentMatchtoPipelineMatch.
205-218: LGTM! Consistent updates to method signatures.The return types of
high_prio,medium_prio, andlow_priomethods have been correctly updated fromOptional[IntentMatch]toOptional[PipelineMatch], maintaining consistency with the transition to the new pipeline plugin architecture.
Line range hint
1-224: Overall assessment: Changes are consistent and well-implemented.The modifications in this file successfully transition from
IntentMatchtoPipelineMatch, aligning with the PR objectives. The changes are consistent across method signatures, return types, and object instantiations. TheFallbackServiceclass now inherits fromPipelinePlugin, further supporting the transition to the new pipeline plugin architecture.To ensure the changes are complete and correct, please verify the following:
- Confirm that all necessary methods required by
PipelinePluginare implemented in theFallbackServiceclass.- Double-check that there are no remaining references to
IntentMatchin the file or in any dependent files.- Ensure that any documentation or comments related to these methods are updated to reflect the new
PipelineMatchreturn type.
28-28: LGTM! Verify PipelinePlugin implementation.The
FallbackServiceclass now inherits fromPipelinePlugin, which is consistent with the transition to the new pipeline plugin architecture.To ensure correct implementation, run the following script:
✅ Verification successful
LGTM! PipelinePlugin implementation verified.
The
FallbackServiceclass correctly inherits fromPipelinePluginand includes the necessary__init__method, ensuring compliance with the new pipeline plugin architecture.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify PipelinePlugin implementation ast-grep --lang python --pattern 'class FallbackService(PipelinePlugin): $$$ def __init__(self, $$$): $$$ $$$ 'Length of output: 18926
24-24: LGTM! Verify usage of new imports.The change from
IntentMatchtoPipelineMatchandPipelinePluginis consistent with the PR objectives. This transition appears to be part of a larger update to the latest pipeline plugins signature.To ensure correct usage, run the following script:
✅ Verification successful
Usage of new imports verified.
The usage of
PipelineMatchandPipelinePlugininovos_core/intent_services/fallback_service.pyhas been verified and aligns with the intended changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify usage of PipelineMatch and PipelinePlugin rg --type python -e 'PipelineMatch|PipelinePlugin' ovos_core/intent_services/fallback_service.pyLength of output: 18926
Line range hint
168-179: LGTM! Verify _fallback_range implementation.The return type of the
_fallback_rangemethod has been updated fromOptional[IntentMatch]toOptional[PipelineMatch], which is consistent with the transition to the new pipeline plugin architecture.To ensure correct implementation, run the following script:
ovos_core/intent_services/converse_service.py (4)
10-10: LGTM: Import changes are consistent with code modificationsThe import changes on lines 10 and 14 correctly reflect the transition from
IntentMatchtoPipelineMatchand the reorganization ofConverseModeandConverseActivationModeimports. These changes align with the modifications in theconverse_with_skillsmethod.Also applies to: 14-14
Line range hint
315-344: LGTM: Successful transition from IntentMatch to PipelineMatchThe
converse_with_skillsmethod has been updated to usePipelineMatchinstead ofIntentMatch, which is consistent with the import changes. Thehandledparameter is now correctly set based on thestatevariable, which improves the clarity of the code.
17-17: LGTM: Class inheritance updated to PipelinePluginThe
ConverseServiceclass now inherits fromPipelinePlugin, which is consistent with the transition to the pipeline architecture. This change likely provides additional functionality or interfaces required for the new pipeline system.
340-344:⚠️ Potential issuePotential IndexError when accessing utterances[0]
The code assumes that
utterancesis not empty when accessingutterances[0]. This could lead to anIndexErrorifutterancesis an empty list. Consider adding a check to ensureutterancesis not empty before accessing the first element.Apply this diff to handle the case when
utterancesis empty:+ if not utterances: + LOG.warning("No utterances provided to converse_with_skills") + return None return PipelineMatch(handled=state != UtteranceState.RESPONSE, # handled == True -> emit "ovos.utterance.handled" match_data={}, skill_id=skill_id, - utterance=utterances[0]) + utterance=utterances[0] if utterances else "")Likely invalid or redundant comment.
ovos_core/intent_services/__init__.py (4)
15-33: LGTM! Improved imports for better modularity and type hinting.The changes to the imports reflect a shift towards a more modular and plugin-based architecture. The addition of
Unionfor type hinting and the new pipeline-related imports (OCPPipelineMatcher,PipelineMatch,IntentHandlerMatch) indicate an effort to improve the intent matching system. The replacement ofget_full_lang_codewithstandardize_lang_tagsuggests an update in language handling.
317-317: LGTM! Simplified padatious matcher assignmentThe direct assignment of
self._padatious_servicetopadatious_matcherimproves code readability by removing a redundant conditional assignment.
353-354: LGTM! Improved session validationThe conversion of
_validate_sessionto a static method is appropriate as it doesn't useself. The standardization of thelangparameter usingstandardize_lang_tagensures consistency in language handling throughout the class.
532-532: LGTM! Improved method consistency and language handlingThe changes to these methods improve consistency and language handling:
handle_register_vocab: The addition of thelangparameter ensures proper language-specific vocabulary registration.handle_add_context,handle_remove_context, andhandle_clear_context: Converting these to static methods is appropriate as they don't useself.These changes contribute to better code organization and maintainability.
Also applies to: 562-563, 585-586, 597-598
test/unittests/test_intent_service.py (2)
18-18: Update import to reflect module changesThe import statement for
ContextManagerhas been updated tofrom ovos_bus_client.session import IntentContextManager as ContextManager, which aligns with the module's refactoring. This change ensures that you're importing theIntentContextManagerfrom its new location.
23-23: ImportFakeBusfor improved testingYou've added
from ovos_utils.fakebus import FakeBusto replace the previous use ofMockEmitter. UsingFakeBusenhances your test environment by providing a more realistic simulation of the message bus.
ovos.utterance.handled is now emitted by the fallback pipeline, the skill class should not emit it or we get duplicate events noticed in end2end tests from OpenVoiceOS/ovos-core#571
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_core/intent_services/__init__.py(12 hunks)
🧰 Additional context used
🪛 Ruff
ovos_core/intent_services/__init__.py
31-31: ovos_utils.log.deprecated imported but unused
(F401)
256-256: Ambiguous variable name: l
(E741)
🔇 Additional comments (5)
ovos_core/intent_services/__init__.py (5)
15-33: LGTM: Import changes align with pipeline updates
The import changes correctly support the transition to the latest pipeline plugins signature.
🧰 Tools
🪛 Ruff
31-31: ovos_utils.log.deprecated imported but unused
(F401)
346-379: LGTM: Improved type handling
The changes correctly handle both PipelineMatch and IntentHandlerMatch types with proper type annotations and runtime checks.
323-324: LGTM: Appropriate static method conversions
The conversion of methods to static is correct as they don't require instance state.
Also applies to: 529-530, 552-553, 564-565
499-499: LGTM: Method signature update
The updated parameters for register_vocabulary align with the latest pipeline plugins signature.
256-256:
Rename variable for better readability
The single letter variable l can be confused with number 1 or capital I. Use a more descriptive name like lang_code.
- valid_langs = [standardize_lang_tag(l) for l in valid_langs]
+ valid_langs = [standardize_lang_tag(lang_code) for lang_code in valid_langs]Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff
256-256: Ambiguous variable name: l
(E741)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
mycroft/skills/intent_services/__init__.py (1)
10-14: Enhance the warning message for missing Padatious.The warning message could be more informative to help users understand the implications and fallback behavior.
- LOG.warning("padatious not installed") + LOG.warning("Padatious not installed, falling back to Padacioso pipeline")🧰 Tools
🪛 Ruff
10-10:
ovos_padatious.opm.PadatiousMatcherimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
14-14:
padacioso.opm.PadaciosoPipelineimported but unused(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
mycroft/skills/intent_services/__init__.py(1 hunks)mycroft/skills/intent_services/commonqa_service.py(1 hunks)mycroft/skills/intent_services/padatious_service.py(1 hunks)ovos_core/intent_services/adapt_service.py(1 hunks)ovos_core/intent_services/commonqa_service.py(1 hunks)ovos_core/intent_services/ocp_service.py(1 hunks)ovos_core/intent_services/padacioso_service.py(1 hunks)ovos_core/intent_services/padatious_service.py(1 hunks)requirements/skills-essential.txt(1 hunks)requirements/skills-internet.txt(1 hunks)requirements/skills-media.txt(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- ovos_core/intent_services/adapt_service.py
- ovos_core/intent_services/commonqa_service.py
- ovos_core/intent_services/ocp_service.py
- ovos_core/intent_services/padacioso_service.py
- ovos_core/intent_services/padatious_service.py
🚧 Files skipped from review as they are similar to previous changes (3)
- requirements/skills-essential.txt
- requirements/skills-internet.txt
- requirements/skills-media.txt
🧰 Additional context used
🪛 Ruff
mycroft/skills/intent_services/__init__.py
1-1: ovos_core.intent_services.fallback_service.FallbackService imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: ovos_core.intent_services.converse_service.ConverseService imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
3-3: ovos_adapt.opm.AdaptPipeline imported but unused
(F401)
4-4: padacioso.opm.PadaciosoPipeline imported but unused
(F401)
5-5: ovos_commonqa.opm.CommonQAService imported but unused
(F401)
6-6: ovos_plugin_manager.templates.pipeline.IntentMatch imported but unused
(F401)
7-7: ovos_workshop.intents.Intent imported but unused
(F401)
7-7: ovos_workshop.intents.IntentBuilder imported but unused
(F401)
7-7: ovos_workshop.intents.Intent imported but unused
(F401)
10-10: ovos_padatious.opm.PadatiousMatcher imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
14-14: padacioso.opm.PadaciosoPipeline imported but unused
(F401)
mycroft/skills/intent_services/commonqa_service.py
1-1: ovos_commonqa.opm.CommonQAService imported but unused
Remove unused import: ovos_commonqa.opm.CommonQAService
(F401)
mycroft/skills/intent_services/padatious_service.py
16-16: ovos_padatious.opm.PadatiousMatcher imported but unused
Remove unused import
(F401)
16-16: ovos_padatious.opm.PadatiousPipeline imported but unused
Remove unused import
(F401)
16-16: ovos_padatious.opm.PadatiousIntent imported but unused
Remove unused import
(F401)
🔇 Additional comments (2)
mycroft/skills/intent_services/__init__.py (2)
1-2: LGTM: Core service imports are properly organized.
The imports are correctly structured to use the dedicated modules from ovos_core.intent_services.
🧰 Tools
🪛 Ruff
1-1: ovos_core.intent_services.fallback_service.FallbackService imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: ovos_core.intent_services.converse_service.ConverseService imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
3-5: Verify coordination with companion PRs.
The pipeline service imports have been updated to use the latest plugin signatures. Please ensure these changes are synchronized with the companion PRs mentioned in the PR description, particularly:
- ovos-adapt-pipeline-plugin#12
- ovos-padatious-pipeline-plugin#15
- ovos-common-query-pipeline-plugin#11
✅ Verification successful
Pipeline service imports are correctly aligned with the codebase
The verification shows that these pipeline services are properly integrated in the codebase:
- The imports are used consistently across both
mycroft/skills/intent_servicesandovos_core/intent_services - The services are properly instantiated in the
IntentServiceclass - There are corresponding service-specific files (
adapt_service.py,commonqa_service.py, etc.) that handle the implementation details - The deprecation notices in the codebase confirm these are the new intended import paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the imported classes exist in the respective repos
# Description: Check if the new pipeline classes are defined in the companion repos
echo "Checking for AdaptPipeline in ovos-adapt-pipeline-plugin..."
rg -l "class AdaptPipeline"
echo "Checking for PadaciosoPipeline in padacioso..."
rg -l "class PadaciosoPipeline"
echo "Checking for CommonQAService in ovos-commonqa..."
rg -l "class CommonQAService"
Length of output: 434
Script:
#!/bin/bash
# Let's check the imports and their usage in the codebase
# Check if these imports are used and how
echo "Checking usage of pipeline services..."
rg -A 5 "AdaptService|PadaciosoService|CommonQAService"
# Check the import statements to understand the module structure
echo -e "\nChecking import statements..."
rg "^from.*import.*(Adapt|Padacioso|CommonQA)"
# Look for any configuration or setup related to these services
echo -e "\nChecking for service configuration..."
rg -l "pipeline.*service" --type yaml
Length of output: 7038
🧰 Tools
🪛 Ruff
3-3: ovos_adapt.opm.AdaptPipeline imported but unused
(F401)
4-4: padacioso.opm.PadaciosoPipeline imported but unused
(F401)
5-5: ovos_commonqa.opm.CommonQAService imported but unused
(F401)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ovos_core/intent_services/__init__.py (1)
500-645: Consider adding deprecation timeline documentation.The deprecated methods and properties are well marked with
@deprecateddecorators and version information. However, it would be helpful to add documentation about when these will be removed completely.Add a comment block at the start of the deprecated section:
########### # DEPRECATED STUFF # The following methods and properties are deprecated and will be removed in version 1.0.0 # Users should migrate to using the plugin-based pipeline system instead. ###########
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_core/intent_services/__init__.py(14 hunks)
🧰 Additional context used
🪛 Ruff
ovos_core/intent_services/__init__.py
146-146: Ambiguous variable name: l
(E741)
🔇 Additional comments (6)
ovos_core/intent_services/__init__.py (6)
15-16: LGTM! Type hints and imports are properly structured.
The addition of Union type hints and defaultdict import improves code clarity and type safety.
236-244: LGTM! Well-documented deactivation tracking implementation.
The new deactivation tracking mechanism properly handles skill state management during intent matching.
381-382: LGTM! Proper cleanup of deactivation tracking.
The code correctly cleans up the deactivation tracking dictionary when the session ends.
213-214: LGTM! Proper conversion to static methods.
The conversion of these methods to static methods is appropriate as they don't use instance state.
Also applies to: 397-398, 420-421, 432-433
146-146:
Replace ambiguous variable name l with descriptive name.
Using single-letter variable names like l can be confusing as it looks similar to 1 (one) or I (capital i).
-valid_langs = [standardize_lang_tag(l) for l in valid_langs]
+valid_langs = [standardize_lang_tag(lang_code) for lang_code in valid_langs]Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff
146-146: Ambiguous variable name: l
(E741)
246-284: 🛠️ Refactor suggestion
Consider adding error handling for match data access.
The code assumes match_type and match_data will always exist on IntentHandlerMatch objects. While this is likely true for internal implementations, it's safer to handle potential attribute errors.
- data.update(match.match_data)
+ if hasattr(match, 'match_data'):
+ data.update(match.match_data)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test/unittests/test_intent_service.py (3)
Line range hint
26-49: Remove unusedMockEmitterclassThe
MockEmitterclass is defined but not utilized anywhere in the code. Removing this unused class will clean up the codebase and improve maintainability.Apply this diff to remove the unused class:
- class MockEmitter(object): - def __init__(self): - self.reset() - - def emit(self, message): - self.types.append(message.msg_type) - self.results.append(message.data) - - def get_types(self): - return self.types - - def get_results(self): - return self.results - - def remove(self, msg_type, handler): - self.removed.append(msg_type) - - def on(self, msg_type, handler): - pass - - def reset(self): - self.removed = [] - self.types = [] - self.results = []
Line range hint
51-51: Remove unusedemitterattribute inContextManagerTestThe
ContextManagerTestclass declares anemitterattribute but does not use it within any of its methods. Removing this unused attribute will enhance code clarity.Apply this diff to remove the unused attribute:
- class ContextManagerTest(TestCase): - emitter = MockEmitter() + class ContextManagerTest(TestCase):
Line range hint
73-79: Ensure test isolation when modifying global stateIn the
test_no_lang_in_messagemethod, the global locale is modified usingsetup_locale("it-it")andsetup_locale("en-US"). Modifying global state within tests can lead to flaky tests if other tests depend on the locale setting. Consider resetting the locale after the test to ensure proper isolation.You can restore the original locale after the test completes:
@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.""" + original_locale = setup_locale() setup_locale("it-it") msg = Message('test msg', data={}) self.assertEqual(get_message_lang(msg), 'it-IT') setup_locale("en-US") self.assertEqual(get_message_lang(msg), 'en-US') + setup_locale(original_locale)Alternatively, use
setUpandtearDownmethods to manage locale settings:class TestLanguageExtraction(TestCase): + def setUp(self): + self.original_locale = setup_locale() + + def tearDown(self): + setup_locale(self.original_locale)This ensures that each test starts with a known state and cleans up after itself, preventing unintended side effects.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
ovos_core/intent_services/__init__.py (3)
81-83: Consider adding session cleanup for deactivationsThe new deactivation tracking mechanism is a good addition, but the
_deactivationsdictionary might grow if sessions are abandoned without proper cleanup.Consider adding a cleanup mechanism in the session management code or implementing a session timeout-based cleanup.
Also applies to: 236-244
Line range hint
350-363: Optimize blacklist checking logicThe current implementation checks blacklists after obtaining a match. Consider moving these checks into the match functions to avoid unnecessary processing.
This would prevent the pipeline from spending time on matches that will be discarded anyway.
500-645: Enhance deprecation messages with migration examplesWhile the deprecation notices are clear, consider adding specific examples of how to migrate to the new plugin-based approach in the deprecation messages.
For example:
- log_deprecation("direct access to self.adapt_service is deprecated, " - "pipelines are in the progress of being replaced with plugins", "1.0.0") + log_deprecation("direct access to self.adapt_service is deprecated. " + "Use the plugin system instead, e.g., 'from ovos_plugin_manager import get_intent_plugin'. " + "Pipelines are in the progress of being replaced with plugins", "1.0.0")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_core/intent_services/__init__.py(14 hunks)
🧰 Additional context used
🪛 Ruff
ovos_core/intent_services/__init__.py
146-146: Ambiguous variable name: l
(E741)
🔇 Additional comments (2)
ovos_core/intent_services/__init__.py (2)
Line range hint 397-433: LGTM! Good static method conversions
The conversion of context handling methods to static methods is appropriate as they don't use instance state. The added type hints improve code clarity.
146-146:
Rename ambiguous variable name
The variable name l is ambiguous and could be confused with the number '1' or capital 'I'.
-valid_langs = [standardize_lang_tag(l) for l in valid_langs]
+valid_langs = [standardize_lang_tag(lang_code) for lang_code in valid_langs]Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff
146-146: Ambiguous variable name: l
(E741)
update imports and requirements for latest pipeline plugins signature
updates typing and versions from #570 without dragging the move to dynamic plugins
companion PRS from new requirements versions:
the number of bus messages per interaction has been reduced and optimized, should improve latency by a few ms
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Dependencies
ovos-padatious,ovos-skill-application-launcher, and others to ensure compatibility with newer features.Tests
Documentation
Deprecations