LCORE-298: configuration for conversation cache #563
Conversation
|
Warning Rate limit exceeded@tisnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 11 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 (4)
WalkthroughAdds conversation cache configuration models and constants (memory/sqlite/postgres) to the configuration schema, plus a new AppConfig.conversation_cache property that proxies the loaded Configuration and raises if the configuration is not loaded. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Loader as ConfigLoader
participant Cfg as Configuration
participant Cache as ConversationCache
App->>Loader: load()
Loader-->>Cfg: instantiate Configuration
activate Cfg
Cfg->>Cache: build ConversationCache
activate Cache
Note over Cache: model_validator (after) validates backend selection
Cache-->>Cfg: validated or error
deactivate Cache
Cfg-->>Loader: loaded Configuration
deactivate Cfg
Loader-->>App: return config (success/error)
sequenceDiagram
autonumber
participant Client as Consumer
participant AppCfg as AppConfig
participant Cfg as Configuration
participant Cache as ConversationCache
Client->>AppCfg: conversation_cache
alt configuration not loaded
AppCfg-->>Client: raise LogicError
else loaded
AppCfg->>Cfg: get conversation_cache
Cfg-->>AppCfg: ConversationCache
AppCfg-->>Client: ConversationCache
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
tisnik
force-pushed
the
lcore-298-configuration-for-conversation-cache
branch
from
338b802 to
42b8d38
Compare
![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)
src/constants.py (1)
62-65: Add a supported set and a default knob for cache config.Centralize allowed backends and expose a sane default for in‑memory cache size to help validators, docs, and UIs.
# cache constants CACHE_TYPE_MEMORY = "memory" CACHE_TYPE_SQLITE = "sqlite" CACHE_TYPE_POSTGRES = "postgres" + +# supported cache backends +SUPPORTED_CONVERSATION_CACHE_TYPES = frozenset( + {CACHE_TYPE_MEMORY, CACHE_TYPE_SQLITE, CACHE_TYPE_POSTGRES} +) + +# defaults +DEFAULT_IN_MEMORY_CACHE_MAX_ENTRIES = 10_000src/models/config.py (2)
79-83: Provide a default and clarify units for max_entries.Make memory cache easier to enable without extra YAML and align with constants policy.
-class InMemoryCacheConfig(ConfigurationBase): +class InMemoryCacheConfig(ConfigurationBase): """In-memory cache configuration.""" - max_entries: PositiveInt + # Maximum number of conversation entries retained in memory (LRU policy assumed by impl) + max_entries: PositiveInt = Field( + default=constants.DEFAULT_IN_MEMORY_CACHE_MAX_ENTRIES + )If you don’t want a default, at least document expected magnitude (hundreds, thousands).
489-515: Constrain type and reject contradictory sub-configs.
- Narrow typing of
typeto prevent typos at compile-time.- Fail fast if
typeis set but other backend configs are also provided, or if sub-configs are provided without atype.class ConversationCache(ConfigurationBase): """Conversation cache configuration.""" - type: Optional[str] = None + type: Literal["memory", "sqlite", "postgres"] | None = None memory: Optional[InMemoryCacheConfig] = None sqlite: Optional[SQLiteDatabaseConfiguration] = None postgres: Optional[PostgreSQLDatabaseConfiguration] = None @model_validator(mode="after") def check_cache_configuration(self) -> Self: """Check conversation cache configuration.""" - if self.type is None: - return self + # If any backend config is provided, type must be explicitly selected + if self.type is None: + if any([self.memory, self.sqlite, self.postgres]): + raise ValueError( + "Conversation cache type must be set when backend configuration is provided" + ) + return self match self.type: case constants.CACHE_TYPE_MEMORY: if self.memory is None: raise ValueError("Memory cache is selected, but not configured") + if any([self.sqlite, self.postgres]): + raise ValueError("Only memory cache config must be provided") case constants.CACHE_TYPE_SQLITE: if self.sqlite is None: raise ValueError("SQLite cache is selected, but not configured") + if any([self.memory, self.postgres]): + raise ValueError("Only SQLite cache config must be provided") case constants.CACHE_TYPE_POSTGRES: if self.postgres is None: raise ValueError("PostgreSQL cache is selected, but not configured") + if any([self.memory, self.sqlite]): + raise ValueError("Only PostgreSQL cache config must be provided") case _: raise ValueError("Invalid conversation cache type selected") return selfOptional: replace
Literal[...]withEnumfor parity with other enums in this module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/configuration.py(2 hunks)src/constants.py(1 hunks)src/models/config.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/configuration.pysrc/constants.pysrc/models/config.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/configuration.pysrc/constants.pysrc/models/config.py
src/{models/config.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields
Files:
src/configuration.pysrc/models/config.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/configuration.pysrc/models/config.py
src/constants.py
📄 CodeRabbit inference engine (CLAUDE.md)
Keep shared constants in a central src/constants.py with descriptive comments
Files:
src/constants.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/config.py
🧬 Code graph analysis (2)
src/configuration.py (1)
src/models/config.py (1)
ConversationCache(489-514)
src/models/config.py (1)
src/configuration.py (1)
conversation_cache(129-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (2)
src/models/config.py (1)
532-533: LGTM: default_factory ensures a consistent ConversationCache object.No behavior concerns.
src/configuration.py (1)
22-23: LGTM — new accessor follows existing patterns and guards unloaded state. Search forconversations?_cachereturned matches only in src/configuration.py and src/models/config.py; no strayconversations_cachefound.
tisnik
force-pushed
the
lcore-298-configuration-for-conversation-cache
branch
from
42b8d38 to
3232b53
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/models/config.py (2)
79-84: InMemoryCacheConfig: solid; add Attributes section for clarityLGTM; add an Attributes block per guidelines.
class InMemoryCacheConfig(ConfigurationBase): - """In-memory cache configuration.""" + """In-memory cache configuration. + + Attributes: + max_entries: Maximum number of cached entries to retain. + """
489-530: ConversationCache validator is correct; add convenience accessors + tiny nits
- Add cache_type/config properties (mirrors DatabaseConfiguration) for ergonomic access.
- Swap any([...]) to any((...)) to avoid intermediate lists.
- Expand docstring with Attributes.
class ConversationCache(ConfigurationBase): - """Conversation cache configuration.""" + """Conversation cache configuration. + + Attributes: + type: Selected backend ("memory", "sqlite", or "postgres"). If None, cache is disabled. + memory: Configuration for in-memory backend. + sqlite: Configuration for SQLite backend. + postgres: Configuration for PostgreSQL backend. + """ @@ - if self.type is None: - if any([self.memory, self.sqlite, self.postgres]): + if self.type is None: + if any((self.memory, self.sqlite, self.postgres)): raise ValueError( "Conversation cache type must be set when backend configuration is provided" ) # no type selected + no configuration is expected and fully supported return self match self.type: case constants.CACHE_TYPE_MEMORY: if self.memory is None: raise ValueError("Memory cache is selected, but not configured") # no other DBs configuration allowed - if any([self.sqlite, self.postgres]): + if any((self.sqlite, self.postgres)): raise ValueError("Only memory cache config must be provided") case constants.CACHE_TYPE_SQLITE: if self.sqlite is None: raise ValueError("SQLite cache is selected, but not configured") # no other DBs configuration allowed - if any([self.memory, self.postgres]): + if any((self.memory, self.postgres)): raise ValueError("Only SQLite cache config must be provided") case constants.CACHE_TYPE_POSTGRES: if self.postgres is None: raise ValueError("PostgreSQL cache is selected, but not configured") # no other DBs configuration allowed - if any([self.memory, self.sqlite]): + if any((self.memory, self.sqlite)): raise ValueError("Only PostgreSQL cache config must be provided") case _: raise ValueError("Invalid conversation cache type selected") return self + + @property + def cache_type(self) -> Literal["memory", "sqlite", "postgres"]: + """Return the selected cache type.""" + if self.type is None: + raise ValueError("No conversation cache type configured") + return self.type + + @property + def config( + self, + ) -> InMemoryCacheConfig | SQLiteDatabaseConfiguration | PostgreSQLDatabaseConfiguration: + """Return the effective backend configuration for the selected type.""" + t = self.cache_type + if t == constants.CACHE_TYPE_MEMORY: + assert self.memory is not None + return self.memory + if t == constants.CACHE_TYPE_SQLITE: + assert self.sqlite is not None + return self.sqlite + if t == constants.CACHE_TYPE_POSTGRES: + assert self.postgres is not None + return self.postgres + raise ValueError("Invalid conversation cache type configured")Optional: consider renaming field type -> cache_type for consistency with DatabaseConfiguration.db_type. Non-breaking alternative is to alias and deprecate later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/configuration.py(2 hunks)src/constants.py(1 hunks)src/models/config.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/constants.py
- src/configuration.py
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/models/config.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/models/config.py
src/{models/config.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields
Files:
src/models/config.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/config.py
🧬 Code graph analysis (1)
src/models/config.py (1)
src/configuration.py (1)
conversation_cache(129-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (1)
src/models/config.py (1)
547-547: Approve: conversation_cache top-level field — integration verifiedCACHE_TYPE_* constants are present in src/constants.py and AppConfig.conversation_cache property exists in src/configuration.py; no action required.
tisnik
force-pushed
the
lcore-298-configuration-for-conversation-cache
branch
from
117363b to
fadf325
Compare
tisnik
force-pushed
the
lcore-298-configuration-for-conversation-cache
branch
from
fadf325 to
ae7886b
Compare
1 participant
Description
LCORE-298: configuration for conversation cache
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Documentation