Added a possibility to select which speaker device to use #96
Conversation
WalkthroughThe pull request introduces a new function Changes
Sequence DiagramsequenceDiagram
participant Config as YAML Config
participant ConfigFunc as configure_io()
participant Sounddevice as sounddevice
participant Glados as Glados.start()
Config->>ConfigFunc: Read configuration file
ConfigFunc-->>Config: Parse YAML
ConfigFunc->>Sounddevice: Set default speaker device
Sounddevice-->>ConfigFunc: Device configured
ConfigFunc-->>Glados: Return configuration
Glados->>Glados: Continue initialization
Possibly related PRs
Poem
Finishing Touches
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: 1
🔭 Outside diff range comments (1)
glados.py (1)
Confirmed: Audio device handling needs improvement
The codebase lacks robust error handling for audio device operations:
- No error handling when setting
sd.default.device- Missing device availability checks before playback
- No recovery mechanism for device disconnection
- Basic stream monitoring without proper error recovery
🔗 Analysis chain
Line range hint
401-401: Add robust speaker device handling.The speaker device is used in multiple places but lacks handling for device changes or failures during runtime. Consider:
- Device disconnection during playback
- Device becoming unavailable between calls
- Device hot-plugging scenarios
Let's verify the current error handling:
Consider implementing a robust audio device manager that:
- Monitors device availability
- Handles device changes gracefully
- Falls back to default device when necessary
- Provides device status callbacks
Would you like me to propose a design for this enhancement?
Also applies to: 154-154, 90-90
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling around sounddevice.play calls rg -A 2 "sd\.play"Length of output: 412
Script:
#!/bin/bash # Search for exception handling and device management rg -B 2 -A 4 "try:.*sd\." # Search for sounddevice device configuration rg -B 2 -A 4 "sd\.(get|set|default|query|check)"Length of output: 627
🧰 Tools
🪛 Ruff (0.8.2)
87-88: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
88-88: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
🧹 Nitpick comments (1)
glados.py (1)
622-625: Optimize YAML file reading and add error handling.The current implementation reads the YAML file twice:
- Once in
configure_io- Again in
GladosConfig.from_yamlConsider refactoring to read the file once and share the parsed data.
Apply this diff to improve efficiency:
def start() -> None: """Set up the LLM server and start GlaDOS.""" - configure_io("glados_config.yml") - glados_config = GladosConfig.from_yaml("glados_config.yml") + try: + # Read YAML file once + with open("glados_config.yml", "r", encoding="utf-8") as file: + config_data = yaml.safe_load(file) + + # Configure IO using the parsed data + if "IO" in config_data: + configure_io_from_dict(config_data["IO"]) + + # Create Glados config using the parsed data + glados_config = GladosConfig.from_dict(config_data.get("Glados", {})) + + # Start Glados + glados = Glados.from_config(glados_config) + glados.start_listen_event_loop() + except Exception as e: + logger.error(f"Failed to start GlaDOS: {e}") + sys.exit(1)This requires adding two new methods:
configure_io_from_dictto configure IO from parsed dataGladosConfig.from_dictto create config from parsed dataWould you like me to generate the implementation for these methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
glados.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
glados.py
87-88: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
88-88: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
| def configure_io(path: str, key: str = "IO"): | ||
| data = None | ||
|
|
||
| try: | ||
| # First attempt with UTF-8 | ||
| with open(path, "r", encoding="utf-8") as file: | ||
| data = yaml.safe_load(file).get(key, None) | ||
| except UnicodeDecodeError: | ||
| # Fall back to utf-8-sig if UTF-8 fails (handles BOM) | ||
| with open(path, "r", encoding="utf-8-sig") as file: | ||
| data = yaml.safe_load(file).get(key, None) | ||
|
|
||
| if data is not None: | ||
| if "speaker_device" in data.keys(): | ||
| logger.debug(f"Setting speaker device to {data['speaker_device']}") | ||
| sd.default.device = data["speaker_device"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation.
The function needs improvements in several areas:
- Add error handling for file not found and invalid YAML
- Validate the speaker device ID before setting it
- Add type hints for parameters
Apply this diff to improve the function:
-def configure_io(path: str, key: str = "IO"):
+def configure_io(path: str, key: str = "IO") -> None:
+ """Configure IO devices from YAML configuration file.
+
+ Args:
+ path: Path to the YAML configuration file
+ key: Key in the YAML file containing IO configuration (default: "IO")
+
+ Raises:
+ FileNotFoundError: If the configuration file is not found
+ yaml.YAMLError: If the configuration file is invalid
+ ValueError: If the speaker device ID is invalid
+ """
data = None
try:
# First attempt with UTF-8
with open(path, "r", encoding="utf-8") as file:
data = yaml.safe_load(file).get(key, None)
except UnicodeDecodeError:
# Fall back to utf-8-sig if UTF-8 fails (handles BOM)
with open(path, "r", encoding="utf-8-sig") as file:
data = yaml.safe_load(file).get(key, None)
+ except FileNotFoundError:
+ logger.error(f"Configuration file not found: {path}")
+ raise
+ except yaml.YAMLError as e:
+ logger.error(f"Invalid YAML in configuration file: {e}")
+ raise
- if data is not None:
- if "speaker_device" in data.keys():
- logger.debug(f"Setting speaker device to {data['speaker_device']}")
- sd.default.device = data["speaker_device"]
+ if data is not None and "speaker_device" in data:
+ speaker_device = data["speaker_device"]
+ try:
+ # Query available devices to validate the speaker device ID
+ devices = sd.query_devices()
+ if not any(d["index"] == speaker_device for d in devices):
+ raise ValueError(f"Invalid speaker device ID: {speaker_device}")
+
+ logger.debug(f"Setting speaker device to {speaker_device}")
+ sd.default.device = speaker_device
+ except sd.PortAudioError as e:
+ logger.error(f"Error accessing audio devices: {e}")
+ raise ValueError(f"Error accessing audio devices: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def configure_io(path: str, key: str = "IO"): | |
| data = None | |
| try: | |
| # First attempt with UTF-8 | |
| with open(path, "r", encoding="utf-8") as file: | |
| data = yaml.safe_load(file).get(key, None) | |
| except UnicodeDecodeError: | |
| # Fall back to utf-8-sig if UTF-8 fails (handles BOM) | |
| with open(path, "r", encoding="utf-8-sig") as file: | |
| data = yaml.safe_load(file).get(key, None) | |
| if data is not None: | |
| if "speaker_device" in data.keys(): | |
| logger.debug(f"Setting speaker device to {data['speaker_device']}") | |
| sd.default.device = data["speaker_device"] | |
| def configure_io(path: str, key: str = "IO") -> None: | |
| """Configure IO devices from YAML configuration file. | |
| Args: | |
| path: Path to the YAML configuration file | |
| key: Key in the YAML file containing IO configuration (default: "IO") | |
| Raises: | |
| FileNotFoundError: If the configuration file is not found | |
| yaml.YAMLError: If the configuration file is invalid | |
| ValueError: If the speaker device ID is invalid | |
| """ | |
| data = None | |
| try: | |
| # First attempt with UTF-8 | |
| with open(path, "r", encoding="utf-8") as file: | |
| data = yaml.safe_load(file).get(key, None) | |
| except UnicodeDecodeError: | |
| # Fall back to utf-8-sig if UTF-8 fails (handles BOM) | |
| with open(path, "r", encoding="utf-8-sig") as file: | |
| data = yaml.safe_load(file).get(key, None) | |
| except FileNotFoundError: | |
| logger.error(f"Configuration file not found: {path}") | |
| raise | |
| except yaml.YAMLError as e: | |
| logger.error(f"Invalid YAML in configuration file: {e}") | |
| raise | |
| if data is not None and "speaker_device" in data: | |
| speaker_device = data["speaker_device"] | |
| try: | |
| # Query available devices to validate the speaker device ID | |
| devices = sd.query_devices() | |
| if not any(d["index"] == speaker_device for d in devices): | |
| raise ValueError(f"Invalid speaker device ID: {speaker_device}") | |
| logger.debug(f"Setting speaker device to {speaker_device}") | |
| sd.default.device = speaker_device | |
| except sd.PortAudioError as e: | |
| logger.error(f"Error accessing audio devices: {e}") | |
| raise ValueError(f"Error accessing audio devices: {e}") from e |
🧰 Tools
🪛 Ruff (0.8.2)
87-88: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
88-88: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
|
Hmmm, could you add in the ability to select the recording source too? |
This change allows the user to select which device to use for the calls to
sounddevice.play. It can be set as an extra option in theglados_config.ymlfile like so:To list the available devices and their IDs, just run
sounddevice.query_devices()Summary by CodeRabbit
New Features
Improvements