refactor!:drop OCP - #115
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces significant changes to the audio handling components in the OVOS project. The modifications primarily focus on simplifying the Open Common Play (OCP) functionality by removing the Changes
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
breaking
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ovos_audio/audio.py (1)
353-353: Correct the spelling of “prefered_service”
Suggest renaming the parameter to “preferred_service” for clarity.- def play(self, tracks: Union[List[str], List[Tuple[str, str]]], - prefered_service: Optional[str], repeat: bool = False): + def play(self, tracks: Union[List[str], List[Tuple[str, str]]], + preferred_service: Optional[str], repeat: bool = False):ovos_audio/playback.py (1)
123-133: Add docstring to the newputmethod
The method is clear, but adding a docstring can improve maintainability and clarify usage, e.g.:+ def put(self, wav: str, + visemes: Optional[List[str]]=None, + listen: bool = False, + tts_id: Optional[str] = None, + message: Optional[Message] = None): + """ + Enqueue an audio file for future playback. + :param wav: Path to the WAV file for playback + :param visemes: Sequence of visemes for lip-sync (optional) + :param listen: Set to True if the system should listen subsequently + :param tts_id: Identifier for the TTS engine + :param message: Original message context + """ message = message or Message("") ...ovos_audio/service.py (1)
106-106:enable_old_audioserviceremains set to True
Since OCP is being dropped, consider defaulting this to False if legacy audio is truly optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ovos_audio/audio.py(6 hunks)ovos_audio/playback.py(2 hunks)ovos_audio/service.py(2 hunks)requirements/extras.txt(0 hunks)requirements/requirements.txt(1 hunks)
💤 Files with no reviewable changes (1)
- requirements/extras.txt
🔇 Additional comments (11)
requirements/requirements.txt (1)
2-4: LGTM: Unchanged dependencies maintain stability
The version constraints for other core dependencies remain unchanged, which helps maintain stability during this significant refactor.
ovos_audio/audio.py (7)
25-25: Consider removing the MediaState import if OCP is fully dropped
MediaState is still referenced at line 316 for INVALID_MEDIA, but if the broader goal is to remove all OCP features, ensure this import remains strictly necessary.
28-28: Confirmed usage of require_native_source
The decorator is used across multiple handlers (pause, resume, queue, etc.) and appears necessary.
39-39: Removal of disable_ocp parameter aligns well with PR objectives
Streamlines the constructor and removes unnecessary logic linked to OCP.
49-49: No functional change at line 49
No issues observed here; it appears to be spacing or a minor structural adjustment.
55-55: Minor reorganization of fields
Declaring volume_is_speaking here is fine; no issues or conflicts detected.
222-227: Improved error handling on stopping current track
Wrapping self.current.stop() in a try-except block and setting stopped = False upon failure avoids unhandled exceptions and unexpected state. Good defensive coding.
238-242: Robust approach to restoring volume
Using try-except for self.current.restore_volume() prevents crashes if volume restoration fails.
ovos_audio/playback.py (1)
4-4: New type annotations
Adding List to the imports is consistent and supports the added parameter typing.
ovos_audio/service.py (2)
57-57: Removal of disable_ocp parameter
This change removes outdated OCP logic, making the constructor more concise.
110-110: Constructor usage clarifies OCP removal
This instantiation without disable_ocp aligns with the simplified audio service.
The classic OCP audio backend (ovos_plugin_common_play / OCPAudioBackend) is removed from the legacy audio service — media playback moves to the OCP MediaProvider plugins / ovos-media (OVOS-OCP-1). Removes find_ocp(), self.ocp, and the OCP backend wiring; the per-backend ocp_start/stop/pause/resume/error lifecycle hooks and the generic stream-extractor/MediaState utilities are kept. `disable_ocp` is retained as a deprecated no-op argument on AudioService and PlaybackService for backward compatibility with existing callers (ovoscope harness, ovos-core launcher), and the OCP-specific unit tests are removed. Rebuilt on current dev (modern CI workflows); supersedes the stale branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6d96552 to
507e4b9
Compare
Beep boop! Here's the latest status of your PR checks. 🤖I've aggregated the results of the automated checks for this PR below. 🏷️ Release PreviewThe release candidate is looking strong. 💪 Caution Breaking change — this PR will bump the MAJOR version ( Current:
🚀 Release Channel Compatibility Predicted next version:
📋 Repo HealthYour repository is in great shape! ✅ ✅ All required files present. Latest Version: ✅ 📊 CoverageCalculating the test-to-code ratio. ➗ ✅ 98.2% total coverage Per-file coverage (9 files)
Full report: download the 🔒 Security (pip-audit)Scanning for any signs of suspicious activity. 🕵️♂️ ✅ No known vulnerabilities found (60 packages scanned). ⚖️ License CheckEnsuring no copyleft violations in this PR. ⬅️ ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔍 LintA quick update on the progress of your PR checks. 📈 ❌ ruff: issues found — see job log 🔨 Build TestsEverything is bolted down and ready to go. 🔩 ✅ All versions pass
🔗 Downstream DependentsA quick update on the progress of your PR checks. 📈 ovos-audio==1.2.3a1 Making code review just a little bit easier. 💆♂️ |
1 participant
completely remove OCP integration, leave only the legacy audio system around
Summary by CodeRabbit
Release Notes
New Features
puttoPlaybackThreadfor queuing audio playbackImprovements
Changes
disable_ocpparameter from audio service constructorsDependency Updates
ovos-utilspackage version requirementovos_plugin_common_play[extractors]dependency