feat/pipeline_plugins_opm #527
Conversation
WalkthroughThe updates introduce modifications to workflow configurations and intent service modules within the project. Key changes include the addition of a new system dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PluginManager
participant IntentService
participant AdaptPipeline
User->>PluginManager: Request intent processing
PluginManager->>IntentService: Forward request
IntentService->>AdaptPipeline: Process intent
AdaptPipeline-->>IntentService: Return matched intent
IntentService-->>PluginManager: Send response
PluginManager-->>User: Deliver intent response
Possibly related PRs
Suggested labels
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 (
|
JarbasAl
added
refactor
JarbasAl
force-pushed
the
feat/pipeline_plugins_opm
branch
from
689289f
to
29d9b55
Compare
![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, codebase verification and nitpick comments (4)
.github/workflows/build_tests.yml (1)
35-38: Temporary installation steps approved.The steps to install
ovos-adapt-pipeline-pluginandovos-padatious-pipeline-pluginfrom GitHub are necessary for now. Remember to remove these steps once the plugins are released..github/workflows/license_tests.yml (1)
27-30: Temporary installation steps approved.The steps to install
ovos-adapt-pipeline-pluginandovos-padatious-pipeline-pluginfrom GitHub are necessary for now. Remember to remove these steps once the plugins are released..github/workflows/unit_tests.yml (2)
51-53: Temporary installation steps approved.The steps to install
ovos-adapt-pipeline-pluginfrom GitHub are necessary for now. Remember to remove these steps once the plugin is released.
87-87: Temporary installation step approved.The step to install
ovos-padatious-pipeline-pluginfrom GitHub is necessary for now. Remember to remove this step once the plugin is released.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- .github/workflows/build_tests.yml (1 hunks)
- .github/workflows/license_tests.yml (1 hunks)
- .github/workflows/unit_tests.yml (2 hunks)
- mycroft/skills/intent_services/adapt_service.py (1 hunks)
- ovos_core/intent_services/init.py (2 hunks)
- ovos_core/intent_services/adapt_service.py (1 hunks)
- ovos_core/intent_services/commonqa_service.py (4 hunks)
- ovos_core/intent_services/converse_service.py (4 hunks)
- ovos_core/intent_services/fallback_service.py (3 hunks)
- ovos_core/intent_services/ocp_service.py (9 hunks)
- ovos_core/intent_services/padacioso_service.py (1 hunks)
- ovos_core/intent_services/padatious_service.py (1 hunks)
- ovos_core/intent_services/stop_service.py (6 hunks)
- requirements/lgpl.txt (1 hunks)
- requirements/requirements.txt (1 hunks)
- test/unittests/common_query/ovos_tskill_fakewiki/init.py (1 hunks)
- test/unittests/skills/decorator_test_skill.py (1 hunks)
- test/unittests/skills/test_intent_service_interface.py (1 hunks)
- test/unittests/skills/test_mycroft_skill.py (1 hunks)
Files skipped from review due to trivial changes (3)
- test/unittests/common_query/ovos_tskill_fakewiki/init.py
- test/unittests/skills/decorator_test_skill.py
- test/unittests/skills/test_intent_service_interface.py
Additional context used
Ruff
ovos_core/intent_services/adapt_service.py
2-2:
ovos_adapt.opm.AdaptPipelineimported but unusedRemove unused import:
ovos_adapt.opm.AdaptPipeline(F401)
ovos_core/intent_services/padacioso_service.py
2-2:
padacioso.opm.PadaciosoPipelineimported but unusedRemove unused import
(F401)
2-2:
padacioso.opm.PadaciosoIntentimported but unusedRemove unused import
(F401)
ovos_core/intent_services/padatious_service.py
2-2:
ovos_padatious.opm.PadatiousMatcherimported but unusedRemove unused import
(F401)
2-2:
ovos_padatious.opm.PadatiousPipelineimported but unusedRemove unused import
(F401)
mycroft/skills/intent_services/adapt_service.py
16-16:
ovos_adapt.context.ContextManagerFrameimported but unusedRemove unused import:
ovos_adapt.context.ContextManagerFrame(F401)
17-17:
ovos_adapt.engine.IntentDeterminationEngineimported but unusedRemove unused import:
ovos_adapt.engine.IntentDeterminationEngine(F401)
ovos_core/intent_services/ocp_service.py
415-415: f-string without any placeholders
Remove extraneous
fprefix(F541)
442-442: f-string without any placeholders
Remove extraneous
fprefix(F541)
459-459: f-string without any placeholders
Remove extraneous
fprefix(F541)
470-470: f-string without any placeholders
Remove extraneous
fprefix(F541)
494-494: f-string without any placeholders
Remove extraneous
fprefix(F541)
1094-1094: f-string without any placeholders
Remove extraneous
fprefix(F541)
Additional comments not posted (26)
requirements/lgpl.txt (1)
2-2: LGTM!The version constraint for
fann2remains unchanged, which is acceptable.requirements/requirements.txt (3)
11-11: Verify compatibility with the new version ofovos-plugin-manager.Ensure that the codebase is compatible with the alpha version
0.0.26a33ofovos-plugin-manager.
6-7: Verify compatibility with the new version ofpadacioso.Ensure that the codebase is compatible with the alpha version
0.2.2a1ofpadacioso.
7-7: Verify compatibility withovos-adapt-parser.Ensure that the codebase is compatible with
ovos-adapt-parser..github/workflows/build_tests.yml (1)
33-33: Verify the necessity ofpython3-fann2.Ensure that the addition of
python3-fann2is required for the new features or dependencies.ovos_core/intent_services/fallback_service.py (5)
Line range hint
161-195: Ensure the return type hint is appropriate.The return type hint
Optional[IntentMatch]is appropriate given the method's logic. The method returns anIntentMatchobject orNonebased on the fallback handling.
198-202: Ensure the return type hint is appropriate.The return type hint
Optional[IntentMatch]is appropriate given the method's logic. The method returns the result of_fallback_range, which matches the expected return type.
203-207: Ensure the return type hint is appropriate.The return type hint
Optional[IntentMatch]is appropriate given the method's logic. The method returns the result of_fallback_range, which matches the expected return type.
Line range hint
208-212: Ensure the return type hint is appropriate.The return type hint
Optional[IntentMatch]is appropriate given the method's logic. The method returns the result of_fallback_range, which matches the expected return type.
32-32: IntegratePipelinePlugincorrectly.The class
FallbackServicenow inherits fromPipelinePlugin. Ensure thePipelinePluginmethods and properties are correctly utilized within this class.ovos_core/intent_services/stop_service.py (4)
Line range hint
117-160: Ensure the return type hint is appropriate.The return type hint
Optional[IntentMatch]is appropriate given the method's logic. The method returns anIntentMatchobject orNonebased on the stop handling.
Line range hint
167-194: Ensure the return type hint is appropriate.The return type hint
Optional[IntentMatch]is appropriate given the method's logic. The method returns the result ofmatch_stop_low, which matches the expected return type.
Line range hint
195-236: Ensure the return type hint is appropriate.The return type hint
Optional[IntentMatch]is appropriate given the method's logic. The method returns anIntentMatchobject orNonebased on the stop handling.
15-15: IntegratePipelinePlugincorrectly.The class
StopServicenow inherits fromPipelinePlugin. Ensure thePipelinePluginmethods and properties are correctly utilized within this class.Verification successful
No direct usage of
PipelinePluginmethods or properties found inStopService.The
StopServiceclass inherits fromPipelinePlugin, but no methods or properties ofPipelinePluginare directly used within the class.
ovos_core/intent_services/stop_service.py, lines 15-282Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `PipelinePlugin` methods and properties within `StopService`. # Test: Search for `PipelinePlugin` methods and properties usage. Expect: Proper utilization within `StopService`. rg --type python -A 5 'class StopService' | rg 'PipelinePlugin'Length of output: 94
Script:
#!/bin/bash # Find the file containing the class StopService file=$(fd -e py 'class StopService' -x rg -l 'class StopService' {}) # Verify the usage of `PipelinePlugin` methods and properties within `StopService`. rg 'PipelinePlugin' $fileLength of output: 1353
Script:
#!/bin/bash # Extract the StopService class and its methods from the file rg -A 50 'class StopService' ovos_core/intent_services/stop_service.py > stop_service_extracted.py # Search for PipelinePlugin methods and properties within the extracted class rg 'PipelinePlugin' stop_service_extracted.pyLength of output: 153
Script:
#!/bin/bash # Extract the StopService class and its methods from the file with more lines to ensure the entire class is captured rg -A 100 'class StopService' ovos_core/intent_services/stop_service.py > stop_service_extracted.py # Search for PipelinePlugin methods and properties within the extracted class rg 'PipelinePlugin' stop_service_extracted.pyLength of output: 154
Script:
#!/bin/bash # Identify the start and end of the StopService class definition start_line=$(rg -n 'class StopService' ovos_core/intent_services/stop_service.py | cut -d: -f1) end_line=$(tail -n +$((start_line + 1)) ovos_core/intent_services/stop_service.py | rg -n '^class|^def' | head -n 1 | cut -d: -f1) if [ -z "$end_line" ]; then end_line=$(wc -l < ovos_core/intent_services/stop_service.py) else end_line=$((start_line + end_line - 1)) fi # Extract the StopService class definition sed -n "${start_line},${end_line}p" ovos_core/intent_services/stop_service.py > stop_service_extracted.py # Search for PipelinePlugin methods and properties within the extracted class rg 'PipelinePlugin' stop_service_extracted.pyLength of output: 401
ovos_core/intent_services/commonqa_service.py (3)
35-39: Ensure theconfigparameter is correctly utilized.The
configparameter is correctly utilized in the initialization logic, allowing for more flexible configuration management within the service.
Line range hint
80-122: Ensure the return type hint is appropriate.The return type hint
Optional[IntentMatch]is appropriate given the method's logic. The method returns anIntentMatchobject orNonebased on the query handling.
34-34: IntegratePipelinePluginandOVOSAbstractApplicationcorrectly.The class
CommonQAServicenow inherits from bothPipelinePluginandOVOSAbstractApplication. Ensure the methods and properties from both base classes are correctly utilized within this class.ovos_core/intent_services/converse_service.py (1)
Line range hint
316-345:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
converse_with_skillsmatch the new return type and usage ofIntentMatch.Verification successful
Verification successful.
The function
converse_with_skillsis used correctly in the codebase, and the return type and usage ofIntentMatchare consistent.
ovos_core/intent_services/converse_service.pyovos_core/intent_services/__init__.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `converse_with_skills` match the new return type and usage of `IntentMatch`. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python -A 5 $'converse_with_skills'Length of output: 75
Script:
#!/bin/bash # Description: Verify all function calls to `converse_with_skills` match the new return type and usage of `IntentMatch`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'converse_with_skills'Length of output: 1601
test/unittests/skills/test_mycroft_skill.py (1)
24-24: LGTM!The import change for
IntentBuilderreflects a reorganization or update in the underlying architecture of the project.ovos_core/intent_services/__init__.py (1)
34-34: LGTM!The import change for
IntentMatchenhances maintainability and consistency across the codebase by moving towards a more centralized or standardized definition.ovos_core/intent_services/ocp_service.py (6)
115-123: Constructor changes look good!The constructor now properly initializes both parent classes
PipelinePluginandOVOSAbstractApplication.
Line range hint
354-393:
Return type annotation is appropriate.The
match_highmethod's return type is correctly defined asOptional[IntentMatch].
Line range hint
395-423:
Return type annotation is appropriate.The
match_mediummethod's return type is correctly defined asOptional[IntentMatch].Tools
Ruff
415-415: f-string without any placeholders
Remove extraneous
fprefix(F541)
Line range hint
424-448:
Return type annotation is appropriate.The
match_fallbackmethod's return type is correctly defined asOptional[IntentMatch].Tools
Ruff
442-442: f-string without any placeholders
Remove extraneous
fprefix(F541)
459-459: f-string without any placeholders
Remove extraneous
fprefix(F541)
470-470: f-string without any placeholders
Remove extraneous
fprefix(F541)
Line range hint
451-504:
Return type annotation is appropriate.The
_process_play_querymethod's return type is correctly defined asOptional[IntentMatch].Tools
Ruff
442-442: f-string without any placeholders
Remove extraneous
fprefix(F541)
459-459: f-string without any placeholders
Remove extraneous
fprefix(F541)
470-470: f-string without any placeholders
Remove extraneous
fprefix(F541)
Line range hint
1074-1098:
Return type annotation is appropriate.The
match_legacymethod's return type is correctly defined asOptional[IntentMatch].Tools
Ruff
1094-1094: f-string without any placeholders
Remove extraneous
fprefix(F541)
JarbasAl
force-pushed
the
feat/pipeline_plugins_opm
branch
2 times, most recently
from
5315d9f
to
eb785e3
Compare
subclass pipelines from the OPM placeholder base class move to maintained ovos-adapt-parser package
JarbasAl
force-pushed
the
feat/pipeline_plugins_opm
branch
from
eb785e3
to
e5cceb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (16)
.github/workflows/build_tests.yml (1)
34-34: Consider moving build tools installation back to its original stepThe installation of build tools (
python -m pip install build wheel) has been moved from the "Install Build Tools" step to the "Install System Dependencies" step. This change makes the workflow less organized and potentially harder to maintain.Consider moving this line back to its original position under the "Install Build Tools" step:
- sudo apt install python3-dev swig libssl-dev libfann-dev portaudio19-dev libpulse-dev python3-fann2 - python -m pip install build wheel + sudo apt install python3-dev swig libssl-dev libfann-dev portaudio19-dev libpulse-dev python3-fann2 # Under the "Install Build Tools" step + python -m pip install build wheelThis will maintain a clearer separation of concerns in the workflow steps.
test/unittests/skills/test_mycroft_skill.py (1)
24-24: Update to OVOS-specific Adapt libraryThe import statement for
IntentBuilderhas been changed fromadapt.intenttoovos_adapt.intent. This indicates a transition to using an OVOS-specific version of the Adapt library.This change may have implications for how intents are built and processed throughout the tests and the entire system. Ensure that:
- All uses of
IntentBuilderin this file and related files are compatible with the OVOS version.- The
ovos_adaptlibrary is properly installed and configured in the project environment.- Any differences in behavior between the original Adapt and OVOS Adapt are documented and accounted for in the tests.
Consider adding a comment explaining the reason for this change to help future maintainers understand the context.
ovos_core/intent_services/__init__.py (2)
Line range hint
516-524: Ensure efficient handling of normalized utterancesThe change to use
padacioso_servicealigns with the earlier modifications. The additional handling for normalized utterances can improve intent matching accuracy. However, ensure that this doesn't introduce unnecessary complexity or performance overhead, especially in cases where the normalized utterance is identical to the original.Consider optimizing the normalized utterance handling:
def handle_get_padatious(self, message): utterance = message.data["utterance"] norm = message.data.get('norm_utt', utterance) - intent = self.padacioso_service.calc_intent(utterance) - if not intent and norm != utterance: - intent = self.padacioso_service.calc_intent(norm) + intent = self.padacioso_service.calc_intent(utterance) + if not intent and norm != utterance: + intent = self.padacioso_service.calc_intent(norm) if intent: intent = intent.__dict__ self.bus.emit(message.reply("intent.service.padatious.reply", {"intent": intent}))This change ensures that we only process the normalized utterance if it's different from the original and the original didn't produce a match, potentially saving unnecessary processing.
Line range hint
1-605: Overall assessment: Improved modularity with potential for optimizationThe changes in this file significantly improve the modularity of the intent parsing system by leveraging external packages. This approach can lead to better maintainability and easier updates in the future. However, there are a few key points to consider:
- Verify the compatibility and stability of the new external packages (
ovos_adapt.opm,ovos_padatious.opm, andpadacioso.opm).- Ensure that the fallback mechanism from
PadatiousServicetoPadaciosoServiceis robust and doesn't introduce unexpected behavior.- Assess the performance impact of the new pipeline configuration, particularly with the addition of
PadaciosoService.- Optimize the handling of normalized utterances in the
handle_get_padatiousmethod to avoid unnecessary processing.It's recommended to thoroughly test these changes, particularly focusing on intent matching accuracy and performance across various scenarios. Consider implementing comprehensive unit tests and conducting real-world testing to ensure the system's stability and effectiveness.
To further improve the system's modularity and maintainability, consider:
- Implementing a plugin system for intent parsers, allowing for easier addition or removal of parsing services.
- Creating an abstract base class for intent parsing services to ensure consistent interfaces across different implementations.
- Implementing a configuration-driven approach for defining the intent matching pipeline, allowing for easier customization without code changes.
ovos_core/intent_services/stop_service.py (9)
Line range hint
45-52: Add type hint to the__init__method for consistencyWhile type hints are used in other methods, the
__init__method lacks a type hint for thebusparameter. Adding it improves code readability and consistency.Consider adding the type hint as follows:
def __init__(self, bus): + def __init__(self, bus: MessageBusClient):Ensure that
MessageBusClientis imported:+from ovos_bus_client import MessageBusClient
Line range hint
55-68: Handle potential exceptions inload_resource_filesmethodThe
load_resource_filesmethod performs file I/O operations without exception handling. This could lead to unhandled exceptions if files or directories are missing or unreadable.Consider adding exception handling to improve robustness:
def load_resource_files(self): base = f"{dirname(__file__)}/locale" if not os.path.exists(base): LOG.warning(f"Locale directory not found: {base}") return for lang in os.listdir(base): lang2 = lang.split("-")[0].lower() self._voc_cache[lang2] = {} for f in os.listdir(f"{base}/{lang}"): file_path = f"{base}/{lang}/{f}" try: with open(file_path, encoding="utf-8") as fi: lines = [expand_options(l) for l in fi.read().split("\n") if l.strip() and not l.startswith("#")] n = f.split(".", 1)[0] self._voc_cache[lang2][n] = flatten_list(lines) except Exception as e: LOG.error(f"Error loading vocabulary file {file_path}: {e}")
Line range hint
98-129: Ensure all skills acknowledge the stop request or handle timeouts appropriatelyIn the
_collect_stop_skillsmethod, if some skills fail to respond to thestop.ping, theEventmay wait until the timeout is reached. This could delay stop actions unnecessarily.Consider implementing a maximum wait time per skill and handling unresponsive skills gracefully to improve responsiveness.
Line range hint
132-137: Check ifactive_skillsis empty before proceedingBefore proceeding with skill pings, it's good practice to check if
active_skillsis not empty to avoid unnecessary processing.Add a condition to return early if there are no active skills:
if not active_skills: return want_stopThis check is already present; ensure it remains after any refactoring.
Line range hint
140-155: Handle missing or unexpected responses instop_skillmethodIn the
stop_skillmethod, ifwait_for_responsereturnsNone, accessingresult.datacould raise anAttributeError.Modify the method to handle
Noneresponses safely:def stop_skill(self, skill_id: str, message: Message): stop_msg = message.reply(f"{skill_id}.stop") result = self.bus.wait_for_response(stop_msg, f"{skill_id}.stop.response") if result: if 'error' in result.data: error_msg = result.data['error'] LOG.error(f"{skill_id}: {error_msg}") return False else: return result.data.get('result', False) else: LOG.warning(f"No response from {skill_id} to stop request.") return False
149-152: Correct alignment inIntentMatchconstructionThe parameters in the
IntentMatchobject are misaligned with the opening parenthesis, which can affect readability.Adjust the indentation for clarity:
return IntentMatch(intent_service='Stop', - intent_type=True, - intent_data={"conf": conf}, - skill_id=None, - utterance=utterance) + intent_type=True, + intent_data={"conf": conf}, + skill_id=None, + utterance=utterance)
163-166: Ensure consistent parameter alignment inIntentMatchSimilar to the previous comment, adjust the indentation of the parameters for better readability.
return IntentMatch(intent_service='Stop', - intent_type=True, - intent_data={"conf": conf}, - skill_id=skill_id, - utterance=utterance) + intent_type=True, + intent_data={"conf": conf}, + skill_id=skill_id, + utterance=utterance)
Line range hint
189-198: Simplify conditional logic inmatch_stop_mediummethodThe conditional checks for
is_stopandis_global_stopcan be streamlined for better readability.Refactor the conditions:
is_stop = self.voc_match(utterance, 'stop', exact=False, lang=lang) is_global_stop = self.voc_match(utterance, 'global_stop', exact=False, lang=lang) or \ (is_stop and not self.get_active_skills(message)) if not is_stop and not is_global_stop: return None return self.match_stop_low(utterances, lang, message)
Line range hint
243-267: Optimize regular expression matching invoc_matchmethodCompiling regular expressions inside the loop can be inefficient. Pre-compiling the regex patterns can improve performance.
Modify the method to pre-compile patterns:
def voc_match(self, utt: str, voc_filename: str, lang: str, exact: bool = False): lang = lang.split("-")[0].lower() if lang not in self._voc_cache: return False _vocs = self._voc_cache[lang].get(voc_filename) or [] if utt and _vocs: if exact: return any(i.strip() == utt for i in _vocs) else: patterns = [re.compile(r'.*\b' + re.escape(i) + r'\b.*') for i in _vocs] return any(pattern.match(utt) for pattern in patterns) return FalseAlternatively, store the compiled patterns in a cache to avoid recompiling them on every call.
ovos_core/intent_services/ocp_service.py (3)
37-37: Consider the Order of Base Classes in Multiple InheritanceThe class
OCPPipelineMatchernow inherits from bothPipelinePluginandOVOSAbstractApplication. The order of base classes in multiple inheritance affects the Method Resolution Order (MRO). Ensure that the inheritance order aligns with the desired behavior, especially if there are overlapping methods or attributes. Reverse the order if necessary.
Line range hint
631-633: Avoid Using BareexceptClausesUsing bare
exceptstatements can catch unintended exceptions, potentially masking bugs and making debugging difficult. It's better practice to catch specific exceptions or at least useexcept Exceptionto avoid catching system-exiting exceptions likeKeyboardInterruptorSystemExit.Apply this diff to catch specific exceptions:
try: # classifier code -except: +except Exception as e: LOG.exception(f"OCP classifier exception: {query}", exc_info=e)Similarly, in the
is_ocp_querymethod:try: # binary classifier code -except: +except Exception as e: LOG.exception("OCP binary classifier failure", exc_info=e)Also applies to: 655-657
Line range hint
754-754: Accessing Protected Member_transformerIn
OCPFeaturizer.extract_entities, you're accessingocp_keywords._transformer, which is a protected member (prefixed with an underscore). Accessing protected members from outside the class is generally discouraged as it may lead to fragile code that's harder to maintain.Consider modifying the
KeywordFeaturesVectorizerclass to provide a public method or property to accesswordlistor perform the extraction without accessing the protected member. This would enhance maintainability and adhere to encapsulation principles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- .github/workflows/build_tests.yml (1 hunks)
- .github/workflows/license_tests.yml (1 hunks)
- mycroft/skills/intent_services/adapt_service.py (1 hunks)
- ovos_core/intent_services/init.py (2 hunks)
- ovos_core/intent_services/adapt_service.py (1 hunks)
- ovos_core/intent_services/commonqa_service.py (2 hunks)
- ovos_core/intent_services/converse_service.py (1 hunks)
- ovos_core/intent_services/fallback_service.py (2 hunks)
- ovos_core/intent_services/ocp_service.py (2 hunks)
- ovos_core/intent_services/padacioso_service.py (1 hunks)
- ovos_core/intent_services/padatious_service.py (1 hunks)
- ovos_core/intent_services/stop_service.py (5 hunks)
- requirements/lgpl.txt (1 hunks)
- requirements/requirements.txt (1 hunks)
- setup.py (2 hunks)
- test/unittests/common_query/ovos_tskill_fakewiki/init.py (1 hunks)
- test/unittests/skills/decorator_test_skill.py (1 hunks)
- test/unittests/skills/test_intent_service.py (1 hunks)
- test/unittests/skills/test_intent_service_interface.py (1 hunks)
- test/unittests/skills/test_mycroft_skill.py (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- requirements/requirements.txt
- test/unittests/common_query/ovos_tskill_fakewiki/init.py
🧰 Additional context used
🪛 Ruff
mycroft/skills/intent_services/adapt_service.py
16-16:
ovos_adapt.context.ContextManagerFrameimported but unusedRemove unused import:
ovos_adapt.context.ContextManagerFrame(F401)
17-17:
ovos_adapt.engine.IntentDeterminationEngineimported but unusedRemove unused import:
ovos_adapt.engine.IntentDeterminationEngine(F401)
18-18:
ovos_workshop.intents.Intentimported but unusedRemove unused import:
ovos_workshop.intents.Intent(F401)
19-19:
ovos_adapt.opm.ContextManagerimported but unusedRemove unused import
(F401)
19-19:
ovos_adapt.opm.AdaptPipelineimported but unusedRemove unused import
(F401)
ovos_core/intent_services/adapt_service.py
2-2:
ovos_adapt.opm.AdaptPipelineimported but unusedRemove unused import:
ovos_adapt.opm.AdaptPipeline(F401)
ovos_core/intent_services/padacioso_service.py
2-2:
padacioso.opm.PadaciosoPipelineimported but unusedRemove unused import
(F401)
2-2:
padacioso.opm.PadaciosoIntentimported but unusedRemove unused import
(F401)
ovos_core/intent_services/padatious_service.py
2-2:
ovos_padatious.opm.PadatiousMatcherimported but unusedRemove unused import
(F401)
2-2:
ovos_padatious.opm.PadatiousPipelineimported but unusedRemove unused import
(F401)
🔇 Additional comments (33)
requirements/lgpl.txt (2)
1-1: LGTM: Version constraint forovos_padatiousis appropriate.The version constraint
>=0.1.0,<1.0.0forovos_padatiousis well-defined and follows the suggestion from the previous review. This ensures compatibility while allowing for minor updates.
1-2: Verify the impact of dependency changes.The changes in this file align with the PR objectives of transitioning to maintained packages. However, it's important to ensure that these changes don't introduce compatibility issues in the project.
Let's verify the usage of the removed and added dependencies:
Please review the output of this script to ensure that:
- There are no remaining imports of
padatiousorpadaos.ovos_padatiousis being imported and used in the codebase.If any inconsistencies are found, additional code changes may be necessary to fully implement this transition.
✅ Verification successful
Action Required: Update Verification Scripts
Please run the following updated shell script to verify the dependency changes:
Dependency Changes Verified Successfully
All removed dependencies (
padatiousandpadaos) are no longer used in the codebase. The new dependencyovos_padatiousis correctly imported and utilized in the following files:
ovos_core/intent_services/padatious_service.pyovos_core/intent_services/__init__.py🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of removed dependencies and confirm usage of new dependencies. echo "Checking for any remaining usage of 'padatious':" rg --type python 'import\s+padatious|from\s+padatious' echo "Checking for any remaining usage of 'padaos':" rg --type python 'import\s+padaos|from\s+padaos' echo "Confirming usage of 'ovos_padatious':" rg --type python 'import\s+ovos_padatious|from\s+ovos_padatious'Length of output: 578
Script:
#!/bin/bash # Description: Check for any remaining usage of removed dependencies and confirm usage of new dependencies. echo "Checking for any remaining usage of 'padatious':" rg 'import\s+padatious|from\s+padatious' echo "Checking for any remaining usage of 'padaos':" rg 'import\s+padaos|from\s+padaos' echo "Confirming usage of 'ovos_padatious':" rg 'import\s+ovos_padatious|from\s+ovos_padatious'Length of output: 700
mycroft/skills/intent_services/adapt_service.py (2)
16-17:⚠️ Potential issueRemove unused imports
The imports of
ContextManagerFrameandIntentDeterminationEngineappear to be unused in this file. To maintain a clean codebase, consider removing these imports if they are not needed.Apply this diff to remove the unused imports:
-from ovos_adapt.context import ContextManagerFrame -from ovos_adapt.engine import IntentDeterminationEngine🧰 Tools
🪛 Ruff
16-16:
ovos_adapt.context.ContextManagerFrameimported but unusedRemove unused import:
ovos_adapt.context.ContextManagerFrame(F401)
17-17:
ovos_adapt.engine.IntentDeterminationEngineimported but unusedRemove unused import:
ovos_adapt.engine.IntentDeterminationEngine(F401)
18-18:⚠️ Potential issueRemove unused import and verify AdaptIntent usage
The
Intentimport fromovos_workshop.intentsappears to be unused. Consider removing it to keep the imports clean.The
AdaptIntentclass is defined but its usage is not visible in the provided code snippet. Please verify if this class is used elsewhere in the project.Apply this diff to remove the unused import:
-from ovos_workshop.intents import IntentBuilder, Intent +from ovos_workshop.intents import IntentBuilderRun the following script to check for usage of the AdaptIntent class:
If the script returns no results, consider removing the AdaptIntent class or adding a comment explaining its purpose if it's intended for future use.
Also applies to: 22-25
✅ Verification successful
Remove unused
IntentimportThe
Intentimport fromovos_workshop.intentsinadapt_service.pyis not used and can be safely removed to keep the imports clean.- from ovos_workshop.intents import IntentBuilder, Intent + from ovos_workshop.intents import IntentBuilder🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of AdaptIntent in the project # Test: Search for AdaptIntent usage echo "Searching for AdaptIntent usage:" rg --type python "AdaptIntent"Length of output: 467
Script:
#!/bin/bash # Description: Check for usage of AdaptIntent in Python files within the project # Test: Search for AdaptIntent usage in .py files echo "Searching for AdaptIntent usage in Python files:" rg "AdaptIntent" --glob "*.py"Length of output: 888
🧰 Tools
🪛 Ruff
18-18:
ovos_workshop.intents.Intentimported but unusedRemove unused import:
ovos_workshop.intents.Intent(F401)
test/unittests/skills/decorator_test_skill.py (2)
Line range hint
1-32: LGTM: Rest of the file is consistent and well-structured.The remaining parts of the file, including the usage of
OVOSSkill, decorators, and the implementation ofTestSkill, are consistent with the project's architecture and the purpose of this test file. No further changes are necessary.
15-15: Import statement updated to useovos_adapt.The import statement has been changed from
adapt.intenttoovos_adapt.intent. This change aligns with the PR objective of transitioning to maintained packages, specifically theovos-adapt-parser.To ensure consistency across the codebase, let's verify if all other occurrences of
adapt.intenthave been updated:✅ Verification successful
Import statements successfully updated to
ovos_adapt.intent.All instances of
from adapt.intent import IntentBuilderhave been replaced withfrom ovos_adapt.intent import IntentBuilderacross the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of 'adapt.intent' # Test: Search for 'adapt.intent'. Expect: No results, indicating all instances have been updated. rg --type python 'from adapt.intent import' # Test: Search for 'ovos_adapt.intent'. Expect: Results showing updated import statements. rg --type python 'from ovos_adapt.intent import'Length of output: 158
Script:
#!/bin/bash # Description: Check for any remaining instances of 'adapt.intent' and confirm 'ovos_adapt.intent' imports # Search for 'from adapt.intent import' in Python files fd --extension py --type f | xargs grep -H 'from adapt.intent import' # Search for 'from ovos_adapt.intent import' in Python files fd --extension py --type f | xargs grep -H 'from ovos_adapt.intent import'Length of output: 530
.github/workflows/build_tests.yml (1)
33-33: LGTM: Addition of python3-fann2 dependencyThe addition of
python3-fann2as a system dependency aligns with the PR objectives of transitioning to maintained packages. This change is appropriate and necessary for the new functionality being introduced..github/workflows/license_tests.yml (2)
42-43: Indentation correction looks goodThe indentation of the "Print report" step has been corrected, ensuring it's properly aligned under the
stepssection. This change improves the readability and correctness of the workflow file.
40-40: Verify the necessity of new package exclusionsThe exclusion list has been updated to include
ovos-adapt-parserandovos-padatious. This change aligns with the PR objectives, which mention transitioning to these maintained packages.To ensure these packages are indeed part of the project dependencies, run the following script:
setup.py (3)
60-66: LGTM: Well-structured plugin entry points.The
PLUGIN_ENTRY_POINTvariable is a well-structured list of entry points for various pipeline plugins. This addition aligns with the PR objectives of enhancing pipeline plugins within the OpenVoiceOS ecosystem. The naming convention is consistent, and the referenced service classes appear to be part of theovos_core.intent_servicesmodule.
98-98: LGTM: Proper integration of pipeline plugins.The addition of the 'opm.pipeline' entry point in the
setup()function correctly integrates thePLUGIN_ENTRY_POINTvariable. This change enables the registration of the defined plugins under the 'opm.pipeline' namespace, which is consistent with the PR objectives of enhancing pipeline functionality.
Line range hint
60-98: Summary: Successful integration of pipeline pluginsThe changes to
setup.pyeffectively integrate the new pipeline plugins into the package setup. The addition of thePLUGIN_ENTRY_POINTvariable and its inclusion in theentry_pointsdictionary of thesetup()function are well-implemented. These modifications align with the PR objectives of transitioning to maintained packages and enhancing pipeline plugins within the OpenVoiceOS ecosystem.The changes provide a clear and organized way to register the new plugins, which should facilitate easier management and extension of the pipeline functionality in the future.
test/unittests/skills/test_intent_service_interface.py (2)
Line range hint
1-186: No further changes required in this file.I've reviewed the rest of the file, and it appears that no additional changes are needed. The tests, particularly in the
KeywordIntentRegistrationTestclass, continue to useIntentBuilderin the same way as before. This suggests that the interface ofIntentBuilderfromovos_adapt.intentis compatible with the previous implementation.However, to ensure complete compatibility, please run the tests after making this change to verify that all test cases still pass successfully.
3-3: LGTM! Verify compatibility with the new IntentBuilder.The import statement has been updated to use
ovos_adapt.intentinstead ofadapt.intent, which aligns with the PR objectives of transitioning to maintained packages. This change looks good.To ensure that the new
IntentBuilderfromovos_adapt.intentis fully compatible with the existing tests, please run the following verification script:This script will help ensure that the new
IntentBuilderclass has the same interface and that all tests still pass with the updated import.test/unittests/skills/test_intent_service.py (2)
22-22: LGTM! Verify ContextManager functionality.The import statement has been updated to use
ovos_adapt.opminstead ofovos_core.intent_services.adapt_service. This change aligns with the PR objectives of transitioning to maintained packages.To ensure that this change doesn't affect the functionality of the tests, please run the following script:
#!/bin/bash # Description: Verify that ContextManager is correctly imported and used in the tests. # Test: Check if ContextManager is imported correctly rg --type python "from ovos_adapt.opm import ContextManager" test/unittests/skills/test_intent_service.py # Test: Verify usage of ContextManager in ContextManagerTest rg --type python -A 10 "class ContextManagerTest" test/unittests/skills/test_intent_service.py
22-22: Run test suite to ensure no regressions.While the import change doesn't require modifications to the existing tests, it's crucial to run the entire test suite to ensure that the new
ovos_adapt.opmpackage maintains the same API and functionality as the previousovos_core.intent_services.adapt_service.Please run the test suite and confirm that all tests pass:
test/unittests/skills/test_mycroft_skill.py (1)
Line range hint
1-1000: Verify test compatibility with OVOS AdaptWhile no other lines in the file have been explicitly changed, the modification of the
IntentBuilderimport could potentially affect the behavior of tests using this class.Please ensure that:
- All tests in this file run successfully with the new OVOS Adapt library.
- The behavior of
IntentBuilderin OVOS Adapt matches the expected behavior in these tests.- If there are any differences in behavior or new features in OVOS Adapt, consider adding new tests to cover these aspects.
To verify, run the following command from the project root:
If any tests fail, please review the output and make necessary adjustments to ensure compatibility with OVOS Adapt.
✅ Verification successful
All Tests Passed Successfully
The tests in
test/unittests/skills/test_mycroft_skill.pyran successfully with the new OVOS Adapt library. No issues were found related to theIntentBuilderimport changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Run the tests and capture the output python -m unittest test/unittests/skills/test_mycroft_skill.py > test_output.txt 2>&1 # Check for any failures or errors if grep -q "FAILED" test_output.txt || grep -q "ERROR" test_output.txt; then echo "Tests failed. Please review the output in test_output.txt" exit 1 else echo "All tests passed successfully." exit 0 fiLength of output: 209
ovos_core/intent_services/__init__.py (3)
60-62: Ensure proper fallback mechanism for PadatiousServiceThe conditional initialization of
PadatiousServicewith a fallback toPadaciosoServiceis a good approach for handling potential import issues or configuration preferences. However, make sure that thePadaciosoServiceis fully compatible with thePadatiousServiceinterface to avoid any runtime issues.To confirm the compatibility between
PadatiousServiceandPadaciosoService, run the following script:#!/bin/bash # Check for common methods between PadatiousService and PadaciosoService python3 -c " from ovos_padatious.opm import PadatiousPipeline from padacioso.opm import PadaciosoPipeline padatious_methods = set(dir(PadatiousPipeline)) padacioso_methods = set(dir(PadaciosoPipeline)) common_methods = padatious_methods.intersection(padacioso_methods) print('Common methods:', common_methods) missing_in_padacioso = padatious_methods - padacioso_methods if missing_in_padacioso: print('Methods in PadatiousService but not in PadaciosoService:', missing_in_padacioso) "
17-17: Verify compatibility with new external packagesThe changes in imports from local modules to external packages (
ovos_adapt.opm,ovos_padatious.opm, andpadacioso.opm) indicate a shift towards more modular architecture. This can improve maintainability and allow for easier updates. However, ensure that these external packages are properly maintained and fully compatible with the current system.To confirm the availability and version compatibility of these packages, run the following script:
Also applies to: 23-23
Line range hint
232-238: Verify performance impact of pipeline changesThe addition of
PadaciosoServiceto the pipeline and the conditional handling ofPadatiousServiceavailability improve the flexibility of the intent matching process. However, it's crucial to ensure that these changes don't negatively impact performance or accuracy.To assess the performance impact, consider implementing and running a benchmark test:
Also, consider adding unit tests to verify the accuracy of intent matching with the new pipeline configuration.
Also applies to: 246-247
ovos_core/intent_services/padatious_service.py (1)
2-2: Verify the necessity of the backward compatibility importsThe static analysis tools indicate that
PadatiousMatcherandPadatiousPipeline(aliased asPadatiousService) are imported but unused in this file. Since these imports are under the# backwards compat importssection, they might be intended for external use to maintain backward compatibility.Please verify if these imports are necessary. If they are not required, consider removing them to clean up the code.
To confirm whether these imports are accessed elsewhere in the codebase, you can run the following script:
✅ Verification successful
Verified: The backward compatibility imports (
PadatiousMatcherandPadatiousService) are used in multiple locations within the codebase.
test/unittests/skills/test_utterance_intents.pyovos_core/intent_services/__init__.pymycroft/skills/intent_services/padatious_service.pymycroft/skills/intent_services/__init__.py🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'PadatiousMatcher' and 'PadatiousService' are imported from 'padatious_service' elsewhere. # Test: Search for import statements that import these classes from 'padatious_service'. # Expect: Instances where these classes are imported from this module. rg --type python -A 2 $'from ovos_core.intent_services.padatious_service import (PadatiousMatcher|PadatiousService)'Length of output: 148
Script:
#!/bin/bash # Description: Check if 'PadatiousMatcher' and 'PadatiousService' are imported from 'padatious_service' elsewhere. # Find all Python files and search for the specific import statements. fd --extension py | xargs rg 'from ovos_core\.intent_services\.padatious_service import (PadatiousMatcher|PadatiousService)'Length of output: 813
🧰 Tools
🪛 Ruff
2-2:
ovos_padatious.opm.PadatiousMatcherimported but unusedRemove unused import
(F401)
2-2:
ovos_padatious.opm.PadatiousPipelineimported but unusedRemove unused import
(F401)
ovos_core/intent_services/padacioso_service.py (1)
2-2: Unused Imports Still PresentAs previously noted, the imports
PadaciosoPipelineandPadaciosoIntentare unused according to the static analysis tool and should be removed to clean up the code.🧰 Tools
🪛 Ruff
2-2:
padacioso.opm.PadaciosoPipelineimported but unusedRemove unused import
(F401)
2-2:
padacioso.opm.PadaciosoIntentimported but unusedRemove unused import
(F401)
ovos_core/intent_services/fallback_service.py (4)
17-17: Importing the 'time' module for timeout functionalityThe addition of
import timeis appropriate and necessary for implementing the time constraints in the_collect_fallback_skillsmethod.
23-23: Importing 'IntentMatch' and 'PipelinePlugin' for plugin integrationThe import of
IntentMatchandPipelinePluginfromovos_plugin_manager.templates.pipelineis correct and essential for integrating the fallback service into the plugin architecture.
31-31: Updating 'FallbackService' to inherit from 'PipelinePlugin'Modifying the
FallbackServiceclass to inherit fromPipelinePluginenhances modularity and aligns it with the plugin-based architecture, facilitating better integration and extensibility.
40-40: Initializing the superclass with the fallback configurationCalling
super().__init__(self.fallback_config)correctly initializes the superclass with the fallback configuration, ensuring that thePipelinePluginbase class is properly set up with the necessary settings.ovos_core/intent_services/stop_service.py (2)
10-10: Import ofIntentMatchandPipelinePluginis appropriateThe addition of the import statement for
IntentMatchandPipelinePluginaligns with the new pipeline architecture and is necessary for the class modifications below.
17-17: ClassStopServicenow correctly inherits fromPipelinePluginUpdating the
StopServiceclass to inherit fromPipelinePluginintegrates it into the pipeline framework, enhancing modularity and consistency with other pipeline plugins.ovos_core/intent_services/commonqa_service.py (3)
9-9: ImportingConfigurationis appropriateThe import statement
from ovos_config.config import Configurationis necessary for accessing configuration settings later in the code, such as in the__init__method.
11-11: ImportingIntentMatchandPipelinePluginis necessaryThe import statement
from ovos_plugin_manager.templates.pipeline import IntentMatch, PipelinePluginis required for the class definition and functionality.IntentMatchis used for intent matching, andPipelinePluginis now a parent class ofCommonQAService.
33-38: 🛠️ Refactor suggestionConsider using
super()for proper initialization in multiple inheritanceThe class
CommonQAServicenow inherits from bothPipelinePluginandOVOSAbstractApplication. In the__init__method, you're explicitly calling the__init__methods of both parent classes:def __init__(self, bus, config=None): OVOSAbstractApplication.__init__( self, bus=bus, skill_id="common_query.openvoiceos", resources_dir=f"{dirname(__file__)}") PipelinePlugin.__init__(self, config)Explicitly invoking parent
__init__methods can lead to issues in multiple inheritance scenarios due to the method resolution order (MRO). To ensure proper initialization and maintainability, consider usingsuper().__init__()which respects the MRO and properly initializes all parent classes.Here's a suggested refactor:
class CommonQAService(PipelinePlugin, OVOSAbstractApplication): def __init__(self, bus, config=None): - OVOSAbstractApplication.__init__( - self, bus=bus, skill_id="common_query.openvoiceos", - resources_dir=f"{dirname(__file__)}") - PipelinePlugin.__init__(self, config) + super().__init__(bus=bus, skill_id="common_query.openvoiceos", + resources_dir=f"{dirname(__file__)}", config=config)This change utilizes
super().__init__()to handle the initialization, which is the recommended practice in Python for multiple inheritance. It assumes that bothOVOSAbstractApplicationandPipelinePluginproperly implement cooperative multiple inheritance by usingsuper()in their own__init__methods.Please verify that the parent classes are compatible with this approach. If they are not designed to work with
super(), and explicit calls are necessary, ensure that the initialization order is correct and document the reasoning to aid future maintainers.ovos_core/intent_services/converse_service.py (2)
10-10: Imports are appropriate for the new class hierarchyThe addition of
IntentMatchandPipelinePluginimports is necessary for the updated class inheritance and intent matching functionalities.
Line range hint
161-168: Ensureshutdownmethod properly calls base classshutdownif necessaryIf
PipelinePlugindefines ashutdownmethod, consider callingsuper().shutdown()within theshutdownmethod to ensure proper resource cleanup.Run the following script to check if
PipelinePlugindefines ashutdownmethod:
ports usage of "excludes" needed for OpenVoiceOS/ovos-core#527 companion to OpenVoiceOS/ovos-adapt-pipeline-plugin#6
ports usage of "excludes" needed for OpenVoiceOS/ovos-core#527 companion to OpenVoiceOS/ovos-adapt-pipeline-plugin#6
* fix:finish extracting intent classes from adapt package ports usage of "excludes" needed for OpenVoiceOS/ovos-core#527 companion to OpenVoiceOS/ovos-adapt-pipeline-plugin#6 * fix test * simplify * add a test
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #527 +/- ##
==========================================
- Coverage 75.33% 72.93% -2.41%
==========================================
Files 15 15
Lines 3094 1633 -1461
==========================================
- Hits 2331 1191 -1140
+ Misses 763 442 -321
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
JarbasAl
requested review from
goldyfruit,
j1nx,
NeonDaniel,
builderjer and
a team
JarbasAl
force-pushed
the
feat/pipeline_plugins_opm
branch
from
e941a28
to
aed42b7
Compare
subclass pipelines from the OPM placeholder base class
directly integrated pipeline into padacioso
move to maintained ovos-adapt-parser and ovos-padatious forked packages and directly integrate pipeline plugins into those
extracts OCP code into it's own plugin ovos-ocp-pipeline-plugin to allow better versioning classifier models etc. same for ovos-common-query-pipeline-plugin
companion PRS:
moved tests:
Summary by CodeRabbit
Release Notes
New Features
PipelinePluginfor several services, improving modularity and functionality.PLUGIN_ENTRY_POINTin setup for better plugin registration.Bug Fixes
Refactor
Chores
requirements.txtandlgpl.txtto reflect new package versions.