Add ConfigManager for unified configuration management#231
Add ConfigManager for unified configuration management#231AnkushMalaker wants to merge 1 commit intofix/open-memory-forkfrom
Conversation
- Introduced a new `config_manager.py` module to handle reading and writing configurations from `config.yml` and `.env` files, ensuring backward compatibility. - Refactored `ChronicleSetup` in `backends/advanced/init.py` to utilize `ConfigManager` for loading and updating configurations, simplifying the setup process. - Removed redundant methods for loading and saving `config.yml` directly in `ChronicleSetup`, as these are now managed by `ConfigManager`. - Enhanced user feedback during configuration updates, including success messages for changes made to configuration files.
|
@coderabbitai review |
WalkthroughThe PR introduces a centralized Changes
Sequence Diagram(s)sequenceDiagram
participant Init as ChronicleSetup<br/>(init.py)
participant CM as ConfigManager
participant YAML as config.yml
participant ENV as .env
participant OS as os.environ
Init->>CM: __init__(service_path)
CM->>CM: _find_repo_root()
CM->>CM: _detect_service_path()
CM->>YAML: Load current config
YAML-->>CM: config data
Note over Init,OS: Configuration Update Phase
Init->>CM: update_config_defaults({...})
CM->>YAML: _save_config_yml()
CM->>Init: Reload config_yml_data
Init->>CM: update_memory_config({...})
CM->>YAML: _save_config_yml()
CM->>ENV: _update_env_file(MEMORY_PROVIDER)
ENV-->>CM: Backup created
CM->>OS: Sync environment variable
CM->>Init: Reload config_yml_data
Note over Init,OS: Configuration Ready
Init->>CM: get_full_config()
CM-->>Init: Complete config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backends/advanced/init.py (1)
275-277: Consider caching config after updates to reduce I/O.The pattern of calling
update_config_defaults()followed immediately byget_full_config()results in redundant file reads. While not critical for an interactive setup script, this could be optimized by having the update methods return the updated config or by caching the result.💡 Possible optimization
Option 1: Have
update_config_defaults()return the updated config:In config_manager.py:
def update_config_defaults(self, updates: Dict[str, str]) -> Dict[str, Any]: config = self._load_config_yml() if "defaults" not in config: config["defaults"] = {} config["defaults"].update(updates) self._save_config_yml(config) return config # Return updated configThen in init.py:
self.config_yml_data = self.config_manager.update_config_defaults({"llm": "openai-llm", "embedding": "openai-embed"})Option 2: Accept this minor inefficiency since the setup script is not performance-critical.
Also applies to: 286-287, 296-297
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backends/advanced/init.pyconfig_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Black formatter with 100-character line length for Python code
Use isort for Python import organization
ALL imports must be at the top of the file after the docstring - never import modules in the middle of functions or files
Group imports in Python files: standard library, third-party, then local imports
Use lazy imports sparingly and only when absolutely necessary for circular import issues in Python
Always raise errors in Python, never silently ignore - use explicit error handling with proper exceptions rather than silent failures
Avoid defensivehasattr()checks in Python - research and understand input/response or class structure instead
Files:
config_manager.pybackends/advanced/init.py
🧠 Learnings (1)
📚 Learning: 2025-12-08T23:52:34.959Z
Learnt from: AnkushMalaker
Repo: chronicler-ai/chronicle PR: 178
File: backends/advanced/src/advanced_omi_backend/services/memory/providers/mycelia.py:218-223
Timestamp: 2025-12-08T23:52:34.959Z
Learning: In Python code (chronicle project), prefer logging.exception() inside except blocks to automatically log the full stack trace. When re-raising exceptions, always chain with 'raise ... from e' to preserve the original context; use 'raise ... from None' only if you explicitly want to suppress the context. This improves debuggability across Python files.
Applied to files:
config_manager.pybackends/advanced/init.py
🧬 Code graph analysis (1)
backends/advanced/init.py (1)
config_manager.py (3)
get_full_config(309-316)update_config_defaults(291-307)update_memory_config(260-279)
🪛 Ruff (0.14.10)
config_manager.py
109-109: Do not catch blind exception: Exception
(BLE001)
110-110: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
130-130: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
178-178: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
219-222: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
backends/advanced/init.py (1)
532-533: Clear documentation of ConfigManager's automatic save behavior.The updated logging message accurately reflects that ConfigManager handles saving both config.yml and .env automatically during updates. This helps users understand the new behavior.
config_manager.py (5)
1-34: Well-structured module with clear documentation.The module docstring provides clear usage examples and principles. Import organization follows coding guidelines with standard library imports first, followed by third-party imports.
40-64: Initialization logic is sound.The constructor properly handles both explicit path parameters and auto-detection, with appropriate fallbacks and debug logging.
66-98: Path detection methods are robust.Both
_find_repo_rootand_detect_service_pathhandle edge cases appropriately with fallbacks and logging. The known services list in_detect_service_pathis clear and maintainable.
181-325: Public API is well-designed and consistent.The public methods provide a clean interface for configuration management:
- Clear separation between getters and setters
- Proper synchronization between config.yml and .env (e.g., line 278-279 in
update_memory_config)- Consistent naming and return types
328-347: Singleton pattern implementation is functional.The
get_config_manager()factory function correctly handles the global singleton instance. Note that providing aservice_pathargument will create a new instance each time (line 345), which allows flexibility for different services but means the singleton is not strictly enforced.
| # Initialize ConfigManager | ||
| repo_root = Path.cwd().parent.parent # backends/advanced -> repo root | ||
| if str(repo_root) not in sys.path: | ||
| sys.path.insert(0, str(repo_root)) | ||
|
|
||
| from config_manager import ConfigManager | ||
|
|
||
| self.config_manager = ConfigManager(service_path="backends/advanced") |
There was a problem hiding this comment.
Move ConfigManager import to module level.
The import statement at line 42 is inside the __init__ method, which violates coding guidelines. As per guidelines: "ALL imports must be at the top of the file after the docstring - never import modules in the middle of functions or files."
The sys.path manipulation (lines 39-40) can remain in __init__, but move the import to the top of the file after other imports.
🔎 Proposed fix
Move the sys.path manipulation and import logic earlier or restructure to ensure the import happens at module level:
At the top of the file, after line 23, add:
from rich.text import Text
+
+# Add repo root to path for ConfigManager import
+_repo_root = Path.cwd().parent.parent
+if str(_repo_root) not in sys.path:
+ sys.path.insert(0, str(_repo_root))
+
+from config_manager import ConfigManagerThen in the __init__ method, replace lines 37-44 with:
- # Initialize ConfigManager
- repo_root = Path.cwd().parent.parent # backends/advanced -> repo root
- if str(repo_root) not in sys.path:
- sys.path.insert(0, str(repo_root))
-
- from config_manager import ConfigManager
-
self.config_manager = ConfigManager(service_path="backends/advanced")Based on coding guidelines.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
backends/advanced/init.py lines 37-44: the ConfigManager import is inside
__init__, violating the rule that all imports must be at module level; move the
import of ConfigManager to the top of the file (after the docstring and other
imports) so the module-level import reads from config_manager import
ConfigManager, and keep the sys.path manipulation (if needed) inside __init__ or
adjust so that module-level import succeeds (e.g., ensure repo_root is on
sys.path before the import by relocating any required path insertion to module
scope ahead of the import or refactor to avoid runtime path hacks).
| def _load_config_yml(self) -> Dict[str, Any]: | ||
| """Load config.yml file.""" | ||
| if not self.config_yml_path.exists(): | ||
| logger.warning(f"config.yml not found at {self.config_yml_path}") | ||
| return {} | ||
|
|
||
| try: | ||
| with open(self.config_yml_path, 'r') as f: | ||
| return yaml.safe_load(f) or {} | ||
| except Exception as e: | ||
| logger.error(f"Failed to load config.yml: {e}") | ||
| return {} |
There was a problem hiding this comment.
Improve exception handling in _load_config_yml.
Multiple issues with error handling:
- Line 110: Use
logging.exception()instead oflogging.error()to automatically log the full stack trace. - Line 111: Returning an empty dict silently swallows errors, which violates the coding guideline to "always raise errors, never silently ignore." If config.yml fails to load due to syntax errors or corruption, callers won't know there's a problem.
🔎 Proposed fix
def _load_config_yml(self) -> Dict[str, Any]:
"""Load config.yml file."""
if not self.config_yml_path.exists():
logger.warning(f"config.yml not found at {self.config_yml_path}")
return {}
try:
with open(self.config_yml_path, 'r') as f:
return yaml.safe_load(f) or {}
except Exception as e:
- logger.error(f"Failed to load config.yml: {e}")
- return {}
+ logger.exception(f"Failed to load config.yml: {e}")
+ raise RuntimeError(f"Failed to load config.yml from {self.config_yml_path}") from eBased on learnings and coding guidelines.
🧰 Tools
🪛 Ruff (0.14.10)
109-109: Do not catch blind exception: Exception
(BLE001)
110-110: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In config_manager.py around lines 100 to 111, the except block currently calls
logger.error and returns an empty dict which swallows real failures; change it
to use logger.exception("Failed to load config.yml at {self.config_yml_path}")
to record the full stack trace and then re-raise the exception (or raise a
specific ConfigLoadError) instead of returning {} so callers are informed of
parse/load failures; keep the existing warning/empty-return behavior only for
the "file not found" branch and still normalize yaml.safe_load(f) None to {}
before returning in the success path.
| def _save_config_yml(self, config: Dict[str, Any]): | ||
| """Save config.yml file with backup.""" | ||
| try: | ||
| # Create backup | ||
| if self.config_yml_path.exists(): | ||
| timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") | ||
| backup_path = self.config_yml_path.parent / f"config.yml.backup.{timestamp}" | ||
| shutil.copy2(self.config_yml_path, backup_path) | ||
| logger.info(f"Backed up config.yml to {backup_path.name}") | ||
|
|
||
| # Write updated config | ||
| with open(self.config_yml_path, 'w') as f: | ||
| yaml.dump(config, f, default_flow_style=False, sort_keys=False) | ||
|
|
||
| logger.info(f"Saved config.yml to {self.config_yml_path}") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Failed to save config.yml: {e}") | ||
| raise |
There was a problem hiding this comment.
Improve logging and exception chaining in _save_config_yml.
Two issues with error handling:
- Line 130: Use
logging.exception()instead oflogging.error()to automatically capture the full stack trace. - Line 131: When re-raising, chain with
raise ... from eto preserve the original exception context for better debuggability.
🔎 Proposed fix
except Exception as e:
- logger.error(f"Failed to save config.yml: {e}")
- raise
+ logger.exception(f"Failed to save config.yml: {e}")
+ raise RuntimeError(f"Failed to save config.yml to {self.config_yml_path}") from eBased on learnings.
🧰 Tools
🪛 Ruff (0.14.10)
130-130: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In config_manager.py around lines 113 to 131, the exception handling in
_save_config_yml uses logger.error and a bare raise which loses context; replace
logger.error(...) with logger.exception(...) so the stack trace is logged, and
when re-raising, use "raise ... from e" (i.e., raise the same or a new exception
from the caught exception) to preserve exception chaining and original context
for debugging.
| def _update_env_file(self, key: str, value: str): | ||
| """Update a single key in .env file.""" | ||
| if self.env_path is None: | ||
| logger.debug("No service path set, skipping .env update") | ||
| return | ||
|
|
||
| if not self.env_path.exists(): | ||
| logger.warning(f".env file not found at {self.env_path}") | ||
| return | ||
|
|
||
| try: | ||
| # Read current .env | ||
| with open(self.env_path, 'r') as f: | ||
| lines = f.readlines() | ||
|
|
||
| # Update or add line | ||
| key_found = False | ||
| updated_lines = [] | ||
|
|
||
| for line in lines: | ||
| if line.strip().startswith(f"{key}="): | ||
| updated_lines.append(f"{key}={value}\n") | ||
| key_found = True | ||
| else: | ||
| updated_lines.append(line) | ||
|
|
||
| # If key wasn't found, add it | ||
| if not key_found: | ||
| updated_lines.append(f"\n# Auto-updated by ConfigManager\n{key}={value}\n") | ||
|
|
||
| # Create backup | ||
| backup_path = f"{self.env_path}.bak" | ||
| shutil.copy2(self.env_path, backup_path) | ||
| logger.debug(f"Backed up .env to {backup_path}") | ||
|
|
||
| # Write updated file | ||
| with open(self.env_path, 'w') as f: | ||
| f.writelines(updated_lines) | ||
|
|
||
| # Update environment variable for current process | ||
| os.environ[key] = value | ||
|
|
||
| logger.info(f"Updated {key}={value} in .env file") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Failed to update .env file: {e}") | ||
| raise |
There was a problem hiding this comment.
Improve exception handling in _update_env_file.
Same exception handling issues as in other methods:
- Line 178: Use
logging.exception()instead oflogging.error(). - Line 179: Chain the exception with
raise ... from eto preserve context.
🔎 Proposed fix
except Exception as e:
- logger.error(f"Failed to update .env file: {e}")
- raise
+ logger.exception(f"Failed to update .env file: {e}")
+ raise RuntimeError(f"Failed to update .env file at {self.env_path}") from eBased on learnings.
🧰 Tools
🪛 Ruff (0.14.10)
178-178: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
config_manager.py around lines 133 to 179: the except block at lines ~178-179
uses logger.error and a bare raise which loses traceback chaining; replace
logger.error(...) with logger.exception(...) to log the stack trace and then
re-raise the exception using "raise from e" (i.e., raise <same exception> from
e) so the original exception context is preserved.
1 participant
config_manager.pymodule to handle reading and writing configurations fromconfig.ymland.envfiles, ensuring backward compatibility.ChronicleSetupinbackends/advanced/init.pyto utilizeConfigManagerfor loading and updating configurations, simplifying the setup process.config.ymldirectly inChronicleSetup, as these are now managed byConfigManager.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.