feat:assistant_config #194
Conversation
deprecates RemoteConfig , backend-client and personal-server have been deprecated already adds AssistantConfig, meant for runtime changes made by OVOS, this ensure we don't accidentally corrupt or ignore user configs some plugins like IPGeo were using the RemoteConf for this purpose as an hack, some other components have been corrupting the user config / replacing user config with syntax errors
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces significant changes to the configuration management system in the OpenVoiceOS project. The modifications focus on updating configuration classes, removing deprecated remote configuration support, and introducing a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Configuration
participant AssistantConfig
participant DefaultConfig
participant SystemConfig
participant DistributionConfig
User->>Configuration: Initialize
Configuration->>AssistantConfig: Create runtime config
Configuration->>DefaultConfig: Load default settings
Configuration->>SystemConfig: Load system settings
Configuration->>DistributionConfig: Load distribution settings
Configuration->>User: Merged Configuration
Poem
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: 0
🧹 Nitpick comments (3)
ovos_config/locations.py (1)
127-127: Consider documenting the runtime configuration format.The new
ASSISTANT_CONFIGpath is introduced for plugins/skills to store runtime changes. Consider adding a docstring or comment explaining the expected format and structure of the runtime.conf file.ovos_config/models.py (1)
172-175: Consider adding docstring to AssistantConfig.The new
AssistantConfigclass is a key addition for runtime changes, but it lacks documentation explaining its purpose and usage.Consider adding a docstring:
class AssistantConfig(LocalConf): + """Configuration class for runtime changes made by plugins and skills. + This config is writable and changes here won't corrupt user configurations.""" def __init__(self): super().__init__(ASSISTANT_CONFIG)ovos_config/config.py (1)
386-396: Consider code style improvements.The function can be simplified using a ternary operator.
- if path is None: - conf = AssistantConfig() - else: - conf = LocalConf(path) + conf = AssistantConfig() if path is None else LocalConf(path)Also, consider updating the docstring to better reflect the AssistantConfig usage:
- """ updates user config file with the contents of provided dict - if a path is provided that location will be used instead of AssistantConfig""" + """Updates configuration with the contents of provided dict. + + Args: + config (dict): Configuration to merge + path (str, optional): If provided, this location will be used instead of AssistantConfig + bus: Optional message bus for broadcasting configuration changes + Returns: + LocalConf: The updated configuration object + """🧰 Tools
🪛 Ruff (0.8.2)
387-390: Use ternary operator
conf = AssistantConfig() if path is None else LocalConf(path)instead ofif-else-blockReplace
if-else-block withconf = AssistantConfig() if path is None else LocalConf(path)(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ovos_config/config.py(5 hunks)ovos_config/locations.py(4 hunks)ovos_config/models.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_config/config.py
387-390: Use ternary operator conf = AssistantConfig() if path is None else LocalConf(path) instead of if-else-block
Replace if-else-block with conf = AssistantConfig() if path is None else LocalConf(path)
(SIM108)
🔇 Additional comments (3)
ovos_config/locations.py (1)
90-94: LGTM! Clear deprecation warnings added.The deprecation warnings are well-implemented with clear messages and proper stacklevel.
Also applies to: 111-115
ovos_config/models.py (1)
189-193: LGTM! Consistent deprecation warnings.The deprecation warnings are consistently implemented across all renamed classes with clear messages.
Also applies to: 200-204, 210-214, 220-224, 230-234, 240-244
ovos_config/config.py (1)
37-37: LGTM! AssistantConfig properly integrated.The
AssistantConfigis correctly integrated into the configuration stack and reload mechanism.Also applies to: 141-141, 173-173
companion to OpenVoiceOS/ovos-config#194
companion to OpenVoiceOS/ovos-config#194
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ovos_config/config.py (2)
386-396: Simplify conditional logic using ternary operator.The code can be simplified using a ternary operator as suggested by the static analysis.
- if path is None: - conf = AssistantConfig() - else: - conf = LocalConf(path) + conf = AssistantConfig() if path is None else LocalConf(path)🧰 Tools
🪛 Ruff (0.8.2)
387-390: Use ternary operator
conf = AssistantConfig() if path is None else LocalConf(path)instead ofif-else-blockReplace
if-else-block withconf = AssistantConfig() if path is None else LocalConf(path)(SIM108)
Line range hint
1-396: Overall implementation aligns well with PR objectives, but RemoteConfig needs removal.The changes successfully introduce AssistantConfig and properly integrate it into the configuration management system. However, there's one critical issue:
- RemoteConfig is still present (line 36) despite being deprecated without replacement according to the pipeline warning.
The rest of the implementation looks solid, with proper deprecation warnings and configuration management integration.
🧰 Tools
🪛 GitHub Actions: Run UnitTests
[warning] 36-36: DeprecationWarning: deprecated without replacement, OVOS no longer supports remote config
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_config/config.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_config/config.py
387-390: Use ternary operator conf = AssistantConfig() if path is None else LocalConf(path) instead of if-else-block
Replace if-else-block with conf = AssistantConfig() if path is None else LocalConf(path)
(SIM108)
🪛 GitHub Actions: Run UnitTests
ovos_config/config.py
[warning] 36-36: DeprecationWarning: deprecated without replacement, OVOS no longer supports remote config
🔇 Additional comments (4)
ovos_config/config.py (4)
16-16: LGTM! Import changes align with the new configuration system.The addition of the warnings module and updated imports reflect the transition to the new configuration system with AssistantConfig.
Also applies to: 20-22
33-35: LGTM! Configuration class attributes updated to use new config types.The changes properly initialize the new configuration types and add AssistantConfig for runtime changes.
Also applies to: 37-37
141-141: LGTM! AssistantConfig properly integrated into configuration management.The changes correctly:
- Include AssistantConfig in reload operations
- Add AssistantConfig to the configuration stack
- Set up file watching for AssistantConfig changes
Also applies to: 173-173, 264-266
376-380: LGTM! Proper deprecation warning added.The deprecation warning is well-implemented with a clear message directing users to use Configuration() directly.
mark as breaking change
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ovos_config/__init__.py (1)
1-1: Address unused imports.The imports
ConfigurationandLocalConfare not used in this file.Either:
- Remove the unused imports:
-from ovos_config.config import Configuration, LocalConf
- Or add them to
__all__if they are meant to be exported:+__all__ = ['Configuration', 'LocalConf'] from ovos_config.config import Configuration, LocalConf🧰 Tools
🪛 Ruff (0.8.2)
1-1:
ovos_config.config.Configurationimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
1-1:
ovos_config.config.LocalConfimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ovos_config/__init__.py(1 hunks)ovos_config/__main__.py(4 hunks)ovos_config/config.py(9 hunks)ovos_config/mycroft.conf(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_config/__init__.py
1-1: ovos_config.config.Configuration imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
1-1: ovos_config.config.LocalConf imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
ovos_config/config.py
333-335: f-string used as docstring. Python will interpret this as a joined string, rather than a docstring.
(B021)
🔇 Additional comments (9)
ovos_config/__main__.py (4)
13-13: LGTM! Configuration paths updated correctly.The changes correctly update the imports and configuration paths to use the new AssistantConfig.
Also applies to: 18-19
109-109: LGTM! Command description updated.The command description has been updated to reflect the new configuration structure.
281-281: LGTM! CLI options updated.The CLI options have been updated to use
assistantinstead ofremote.Also applies to: 285-285
297-297: LGTM! Documentation updated.The joining pattern documentation has been updated to reflect the new configuration hierarchy.
ovos_config/config.py (3)
33-36: LGTM! Configuration class attributes updated.The Configuration class has been correctly updated to use the new config classes.
348-352: LGTM! Deprecation warnings implemented correctly.The deprecation warnings are properly implemented with appropriate warning messages and stacklevels.
Also applies to: 359-363
364-364: LGTM! Clear logging of configuration path changes.The log warning clearly indicates which configuration file is being updated.
ovos_config/mycroft.conf (2)
2-5: LGTM! Documentation updated for OVOS context.The configuration file documentation has been updated to use OpenVoiceOS terminology instead of Mycroft-specific terms.
566-569: LGTM! New session configuration added.The new session configuration section has been added with appropriate documentation.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
test/unittests/test_configuration.py (1)
Line range hint
165-165: Update test assertions for new configuration structureThe test assertions need to be updated to reflect the removal of remote configuration:
Apply this diff to update the assertions:
- "from_rem": False, + "from_rem": None,- "remote": False}) + "remote": None})Also applies to: 151-151, 157-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ovos_config/__main__.py(4 hunks)ovos_config/config.py(9 hunks)ovos_config/models.py(4 hunks)test/unittests/config_stack/remote.yaml(0 hunks)test/unittests/test_configuration.py(3 hunks)
💤 Files with no reviewable changes (1)
- test/unittests/config_stack/remote.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- ovos_config/models.py
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_config/config.py
22-22: ovos_config.models.MycroftDefaultConfig imported but unused
Remove unused import
(F401)
22-22: ovos_config.models.OvosDistributionConfig imported but unused
Remove unused import
(F401)
22-22: ovos_config.models.MycroftSystemConfig imported but unused
Remove unused import
(F401)
22-22: ovos_config.models.MycroftUserConfig imported but unused
Remove unused import
(F401)
22-22: ovos_config.models.RemoteConf imported but unused
Remove unused import
(F401)
333-335: f-string used as docstring. Python will interpret this as a joined string, rather than a docstring.
(B021)
🪛 GitHub Actions: Run UnitTests
test/unittests/test_configuration.py
[error] 195-195: Test failure in test_config_patches_filewatch: AssertionError: False is not true
[error] 305-305: Test failure in test_on_file_changes_not_called: Expected mock to be called once but was called 2 times
[warning] 192-192: DeprecationWarning: use 'update_assistant_config' instead of 'update_mycroft_config'
🔇 Additional comments (6)
test/unittests/test_configuration.py (1)
9-10: LGTM! Import cleanup.The removal of unused imports
sleepandMagicMockimproves code cleanliness.ovos_config/config.py (3)
33-36: LGTM! Configuration class updatesThe changes correctly implement the new configuration structure using the new config classes.
358-365: LGTM! Deprecation warning implementationThe deprecation warning is well implemented with a clear message and proper stacklevel.
332-343:⚠️ Potential issueFix docstring format in update_assistant_config
The function uses an f-string as a docstring, which Python will interpret as a joined string rather than a docstring.
Apply this diff to fix the docstring:
- f""" updates assistant config file with the contents of provided dict - path: {ASSISTANT_CONFIG} - """ + """ updates assistant config file with the contents of provided dict + path: {} + """.format(ASSISTANT_CONFIG)Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
333-335: f-string used as docstring. Python will interpret this as a joined string, rather than a docstring.
(B021)
ovos_config/__main__.py (2)
18-19: LGTM! Updated CONFIGS listThe CONFIGS list correctly reflects the new configuration structure with AssistantConfig replacing RemoteConfig.
297-297: Update configuration joining pattern documentationThe joining pattern documentation has been updated to reflect the new configuration structure.
Run this script to verify the joining pattern matches the implementation:
✅ Verification successful
Configuration joining pattern documentation is accurate
The documented pattern "user > assistant > system > default" correctly reflects the implementation. In the code, configs are loaded in reverse precedence order (default first, user last) because later configs override earlier ones during merging, which creates the documented precedence order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify configuration joining pattern in code # Expected pattern: user > assistant > system > default ast-grep --pattern $'class Configuration { $$$ load_all_configs($$$) { $$$ configs = [$$$] $$$ } $$$ }'Length of output: 124
Script:
#!/bin/bash # Search for configuration related files and the joining pattern echo "=== Searching for configuration files ===" rg -l "Configuration" --type py echo -e "\n=== Searching for joining pattern mentions ===" rg "joining pattern|config.*join" -A 3 -B 3 echo -e "\n=== Searching for configuration loading implementation ===" rg "load_all_configs|load.*config" -A 5 -B 5 --type pyLength of output: 20346
Script:
#!/bin/bash # Search for the load_all_configs implementation with more context echo "=== Searching for load_all_configs implementation ===" rg "def load_all_configs" -A 20 -B 5 ovos_config/config.py echo -e "\n=== Searching for configuration stack loading ===" rg "configs = \[" -A 10 -B 10 ovos_config/config.pyLength of output: 3126
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
deprecates RemoteConfig , backend-client and personal-server have been deprecated already
adds AssistantConfig, meant for runtime changes made by OVOS, this ensure we don't accidentally corrupt or ignore user configs
some plugins like IPGeo were using the RemoteConf for this purpose as an hack, some other components have been corrupting the user config / replacing user config with syntax errors
Summary by CodeRabbit
Release Notes
Configuration Management
AssistantConfigfor runtime configurationUser Experience
Testing
Documentation