fix:better_stop #643
fix:better_stop #643
Conversation
WalkthroughThis pull request encompasses modifications across multiple GitHub Actions workflow configurations and core intent service files. The changes primarily involve updating Python versions in workflow files, removing support for older Python versions (3.8 and below), and enhancing session management in intent services. The modifications aim to streamline testing infrastructure and improve intent handling by introducing more robust session tracking and management mechanisms. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
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 (
|
better state management, relates to OpenVoiceOS/ovos-core#643
better state management, relates to OpenVoiceOS/ovos-core#643
better state management, relates to OpenVoiceOS/ovos-core#643
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #643 +/- ##
==========================================
- Coverage 75.33% 71.01% -4.33%
==========================================
Files 15 15
Lines 3094 1639 -1455
==========================================
- Hits 2331 1164 -1167
+ Misses 763 475 -288
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
.github/workflows/pipaudit.yml (1)
Line range hint
17-18: Update GitHub Actions to latest versionsThe workflow uses outdated GitHub Actions versions that should be updated:
actions/checkout@v2should be updated tov4actions/setup-python@v1should be updated tov4🧰 Tools
🪛 actionlint (1.7.4)
17-17: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/build_tests.yml (1)
Line range hint
22-24: Update GitHub Actions to latest versionsThe workflow uses outdated GitHub Actions versions that should be updated:
actions/checkout@v2should be updated tov4actions/setup-python@v1should be updated tov4🧰 Tools
🪛 actionlint (1.7.4)
22-22: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/coverage.yml (1)
Line range hint
11-18: Fix Python version inconsistencyThe PYTHON environment variable is set to '3.9' but the workflow uses Python 3.10. This inconsistency should be fixed.
Apply this change:
runs-on: ubuntu-latest env: - PYTHON: '3.9' + PYTHON: '3.10'Additionally, update the outdated GitHub Actions:
- - uses: actions/checkout@master + - uses: actions/checkout@v4 - - name: Setup Python - uses: actions/setup-python@master + - name: Setup Python + uses: actions/setup-python@v4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/build_tests.yml(1 hunks).github/workflows/coverage.yml(1 hunks).github/workflows/pipaudit.yml(1 hunks).github/workflows/sync_tx.yml(1 hunks)ovos_core/intent_services/__init__.py(2 hunks)ovos_core/intent_services/converse_service.py(1 hunks)ovos_core/intent_services/stop_service.py(5 hunks)requirements/requirements.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: end2end_tests (3.9)
🔇 Additional comments (10)
ovos_core/intent_services/stop_service.py (5)
55-61: LGTM! Good improvement in skill filtering.The addition of session-based blacklist filtering prevents attempting to stop skills that are explicitly blacklisted, improving the robustness of the stop mechanism.
116-119: LGTM! Improved result handling.The explicit
stoppedflag makes the result handling more clear and maintainable.
162-162: LGTM! Enhanced logging and session management.Good improvements:
- Added logging for global stop events
- Added debug logging for skill stop attempts
- Properly updating and returning session state
Also applies to: 173-181
252-259: LGTM! Consistent improvements across stop handlers.The changes maintain symmetry with
match_stop_high, ensuring consistent logging and session handling in both high and low priority stop scenarios.Also applies to: 262-262
120-132: Track TTS origin for more accurate cleanup.The TODO comment indicates that speech origin is not currently tracked, which could lead to stopping TTS from unrelated skills.
Run this script to check if TTS tracking is implemented elsewhere:
ovos_core/intent_services/converse_service.py (1)
345-345: LGTM! Improved session state propagation.The addition of
updated_sessionto thePipelineMatchreturn value ensures that session state changes are properly propagated through the intent pipeline.ovos_core/intent_services/__init__.py (1)
279-279: LGTM! Completed session state propagation chain.The changes properly handle session state propagation:
- Uses updated session from match when available
- Ensures session state is serialized in reply context
Also applies to: 309-312
requirements/requirements.txt (1)
13-13: Verify compatibility with dependent packagesThe minimum version requirement for
ovos-plugin-managerhas been increased significantly from 0.5.6 to 0.8.0. While maintaining the same upper bound of <1.0.0, this change might affect compatibility with other packages.Let's verify the compatibility with other packages and check for any breaking changes:
✅ Verification successful
Version bump appears safe based on codebase analysis
The increase in minimum version requirement for
ovos-plugin-managerfrom 0.5.6 to 0.8.0 appears safe. The package is used extensively throughout the codebase but primarily for its template classes and utility functions, which typically maintain backward compatibility. The upper bound remains at <1.0.0, protecting against potential breaking changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for compatibility issues and breaking changes # Check if any other packages in requirements.txt depend on ovos-plugin-manager pip index dependencies ovos-plugin-manager==0.8.0 | grep -f <(cat requirements/requirements.txt | cut -d'>' -f1) # Check changelog/release notes for breaking changes gh api repos/OpenVoiceOS/ovos-plugin-manager/releases | jq -r '.[].body' | grep -i "breaking"Length of output: 358
Script:
#!/bin/bash # Search for imports and usage of ovos-plugin-manager rg -l "from ovos_plugin_manager|import ovos_plugin_manager" --type python # Search for any configuration files that might reference the package fd -e yaml -e toml -e ini -e cfg | xargs rg "ovos.plugin.manager|ovos_plugin_manager" # Check setup.py/pyproject.toml for dependencies fd "setup.py|pyproject.toml" | xargs catLength of output: 21553
Script:
#!/bin/bash # Search for any imports or usage of ovos-plugin-manager in Python files rg "from ovos_plugin_manager|import ovos_plugin_manager|ovos[_\.]plugin[_\.]manager" -t py # Also check for any potential version constraints in requirements files fd "requirements.txt" | xargs catLength of output: 4368
.github/workflows/pipaudit.yml (1)
14-14: LGTM: Python version matrix updateRemoving Python 3.7 and 3.8 from the test matrix aligns with the project's direction to modernize the Python version requirements.
.github/workflows/build_tests.yml (1)
19-19: Verify Python version matrix consistency across workflowsThe Python version matrix in this workflow (3.10, 3.11) is more restrictive compared to pipaudit.yml (3.9, 3.10, 3.11). This inconsistency might lead to confusion:
- build_tests.yml: Python 3.10, 3.11
- pipaudit.yml: Python 3.9, 3.10, 3.11
Please ensure this is intentional and update the matrices to be consistent across workflows.
Docstrings generation was requested by @JarbasAl. * #643 (comment) The following files were modified: * `ovos_core/intent_services/__init__.py` * `ovos_core/intent_services/converse_service.py` * `ovos_core/intent_services/stop_service.py`
|
Note We have generated docstrings for this pull request, at #644 |
Docstrings generation was requested by @JarbasAl. * #643 (comment) The following files were modified: * `ovos_core/intent_services/__init__.py` * `ovos_core/intent_services/converse_service.py` * `ovos_core/intent_services/stop_service.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
no need for 2 handlers, decrease complexity the only difference is an emitted event that is not listened too anywhere
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ovos_core/intent_services/converse_service.py (2)
359-359: Consider defensive copying of the session object.While adding the session to
PipelineMatchis a good improvement for session state tracking, consider creating a deep copy of the session object to prevent any unintended modifications after this point.- updated_session=session, + updated_session=session.copy(),
Line range hint
316-359: Good session management pattern implementation.The addition of session state tracking in the pipeline match provides a clean way to propagate session changes through the conversation pipeline. This architectural pattern will help with:
- Debugging conversation flow
- State management across the system
- Tracing skill interactions
Consider documenting this pattern in the project's architectural documentation to establish it as a best practice.
test/end2end/session/test_stop.py (1)
144-153: Enhanced cleanup process for stop operation.The changes introduce a more comprehensive cleanup sequence that:
- Properly terminates ongoing skill activities
- Forces timeout of conversations
- Stops any ongoing speech
- Updates skill usage timestamps
This enhancement ensures a more thorough cleanup when stopping skills.
Consider documenting these cleanup steps in the core documentation to help skill developers understand the complete stop sequence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ovos_core/intent_services/__init__.py(2 hunks)ovos_core/intent_services/converse_service.py(2 hunks)ovos_core/intent_services/stop_service.py(7 hunks)test/end2end/session/test_stop.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ovos_core/intent_services/init.py
- ovos_core/intent_services/stop_service.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: end2end_tests (3.9)
🔇 Additional comments (6)
ovos_core/intent_services/converse_service.py (1)
316-338: Well-documented method signature and behavior!The enhanced docstring provides comprehensive documentation of the method's purpose, parameters, return values, and implementation details. This improvement in documentation clarity will help developers better understand and use the method.
test/end2end/session/test_stop.py (5)
70-75: LGTM! Protocol simplification in stop response handling.The changes streamline the stop protocol by focusing on response messages rather than tracking both request and response messages. This simplification maintains effective test coverage while reducing redundant checks.
226-234: LGTM! Improved test documentation and consistent protocol.The changes maintain consistency with the simplified stop protocol while adding helpful comments that explain the purpose of each framework's response message.
313-316: LGTM! Consistent cleanup process across implementations.The changes ensure that both old and new stop implementations follow the same thorough cleanup process, maintaining consistency in the codebase.
334-336: LGTM! Enhanced response validation.The addition of explicit checks for both message type and result field improves test robustness by ensuring the stop operation reports success correctly.
357-377: LGTM! Comprehensive failure scenario coverage.The changes improve test coverage by:
- Validating speak messages during failed stop attempts
- Ensuring proper fallback behavior through the pipeline
- Verifying the complete message sequence in failure scenarios
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/end2end/session/test_sched.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: end2end_tests (3.9)
- GitHub Check: mycroft_tests (3.9)
🔇 Additional comments (1)
test/end2end/session/test_sched.py (1)
113-114: Verify context propagation in core functionality.The changes to how context is obtained suggest potential issues with context propagation in the core system. Please verify:
- Whether the context is properly maintained throughout the event chain in the core system
- If there are similar context handling issues in other event-triggering scenarios
- If this fix should be accompanied by changes to the core event scheduling system
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/end2end/skill-fake-fm/__init__.py (1)
Line range hint
18-35: Consider documenting the confidence score thresholdsThe score calculation logic (base 30 + 35 for "fake" + 20/-30 for media type + index) seems to be part of the test infrastructure. Consider adding comments explaining the significance of these thresholds and how they relate to the stop behavior being tested.
+ # Base confidence score for FakeFM skill score = 30 + # Boost confidence if "fake" keyword is present if "fake" in phrase: score += 35 + # Adjust confidence based on media type matching if media_type == MediaType.RADIO: score += 20 else: score -= 30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/end2end/session/test_ocp.py(2 hunks)test/end2end/skill-fake-fm/__init__.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: end2end_tests (3.9)
- GitHub Check: license_tests
🔇 Additional comments (4)
test/end2end/session/test_ocp.py (2)
148-148: LGTM! Clear documentation of expected behavior.The added comment "nothing to play" effectively documents the purpose of the "speak" message in the test sequence.
353-356: LGTM! Verify the expected number of query responses.The test now expects five query responses instead of one, which better reflects the actual behavior of the media search pipeline.
Run this script to verify the expected number of query responses:
✅ Verification successful
Verified: Five query responses is the correct expectation
The codebase consistently expects exactly 5 "ovos.common_play.query.response" messages across all test scenarios (generic search, radio, explicit skill search) in multiple test files. This is a well-established pattern in the OpenVoiceOS Common Play pipeline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the number of query responses in the codebase # Look for patterns where multiple query responses are emitted ast-grep --pattern 'emit($_, "ovos.common_play.query.response")'Length of output: 66
Script:
#!/bin/bash # Search for query response message pattern in various forms echo "=== Searching for query response message ===" rg "ovos\.common_play\.query\.response" -A 3 -B 3 echo -e "\n=== Searching for emit patterns in test files ===" fd "test.*\.py$" --exec rg "emit.*response" -A 2 -B 2 {} echo -e "\n=== Looking for message definitions ===" rg "common_play.*query.*response" -A 2 -B 2Length of output: 19073
test/end2end/skill-fake-fm/__init__.py (2)
18-18: Verify the impact of reduced confidence score on stop behaviorThe initial score reduction from 50 to 30 will result in lower match confidence scores for all search results. Given that this PR is focused on improving stop behavior, we should verify if this change affects the skill's stop responsiveness.
Let's check for related test cases:
Line range hint
18-35: Verify test coverage for lower confidence thresholdsSince this is a test skill and the confidence scores have been lowered, we should ensure that existing test cases still provide adequate coverage for edge cases.
Let's check for existing test coverage:
related OpenVoiceOS/OVOS-workshop#323
Summary by CodeRabbit
Chores
ovos-plugin-managerdependency to version 0.8.0.ovos-workshopdependency to version 3.3.4.Improvements
FakeFMSkillto enhance match confidence evaluation.