-
Notifications
You must be signed in to change notification settings - Fork 55
LCORE-703: rename conversation cache to conversation history #803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LCORE-703: rename conversation cache to conversation history #803
Conversation
WalkthroughThe PR renames Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)tests/unit/models/config/test_conversation_history.py (1)
tests/unit/cache/test_cache_factory.py (2)
⏰ 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). (4)
🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tests/unit/models/config/test_conversation_cache.py (2)
1-16: Rename in tests is consistent; consider aligning names/docs with new classImporting
ConversationHistoryConfigurationhere matches the model rename and keeps tests pointed at the right type. As a follow‑up, consider updating the module docstring (and potentially the file name) to referenceConversationHistoryConfigurationinstead ofConversationCacheConfigurationto avoid confusion.
19-52: Core validation tests still correctly exercise ConversationHistoryConfigurationThe tests for “no type”, “unknown type”, and “type without backend config” now construct
ConversationHistoryConfigurationand still match the validator behavior (Literal enforcement pluscheck_cache_configuration). Everything here looks consistent with the new class name. Note: the subtest label"SQLite cache"for the PostgreSQL branch (line 48) is slightly misleading; you may want to rename it to"PostgreSQL cache"when convenient.src/configuration.py (1)
140-146: Type annotation on conversation_cache_configuration matches underlying modelAnnotating
conversation_cache_configurationasConversationHistoryConfigurationwhile continuing to returnself._configuration.conversation_cachekeeps things type‑safe and consistent withConfigurationinmodels.config. If the long‑term goal is to shift external terminology fully to “conversation history”, you might eventually consider introducing aconversation_history_configurationalias and deprecating the old property name, but that’s not required for this refactor.src/cache/cache_factory.py (1)
3-49: Factory now correctly typed against ConversationHistoryConfigurationUpdating the import and signature to use
ConversationHistoryConfigurationkeepsCacheFactory.conversation_cachein sync with the model layer and withAppConfig.conversation_cache. The control flow and validation semantics are unchanged and remain consistent with theConversationHistoryConfigurationvalidator.Minor nit: the invalid cache type error message concatenation (lines 46‑48) is slightly awkward/misalquoted (
"'{constants.CACHE_TYPE_MEMORY} or {constants.CACHE_TYPE_NOOP}'"); consider tightening that string when you next touch this area.src/models/config.py (1)
514-552: ConversationHistoryConfiguration definition is coherent with existing cache semanticsThe
ConversationHistoryConfigurationmodel cleanly encapsulates the cache backend selection (type) plus backend‑specific configs (memory/sqlite/postgres) and enforces consistent combinations viacheck_cache_configuration. This matches howCacheFactory.conversation_cacheand the unit tests exercise it, so the rename looks behaviorally neutral.Given the class name now uses “History” while the docstring and error text still say “Conversation cache configuration” / “Conversation cache type…”, you may eventually want to align the wording with the new terminology for clarity, but it’s not required for correctness.
docs/openapi.json (1)
2123-2177: ConversationHistoryConfiguration schema matches the model; optional wording tweakThe new
ConversationHistoryConfigurationcomponent correctly exposestype,memory,sqlite, andpostgreswith the expected enums and references, mirroring the Pydantic model.The
descriptionstill reads “Conversation cache configuration.”; if the intent is to emphasize “history” across the public surface, consider updating this text in a follow‑up.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/config.pngis excluded by!**/*.pngdocs/config.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
docs/config.puml(3 hunks)docs/openapi.json(2 hunks)src/cache/cache_factory.py(2 hunks)src/configuration.py(2 hunks)src/models/config.py(2 hunks)tests/unit/cache/test_cache_factory.py(10 hunks)tests/unit/models/config/test_conversation_cache.py(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/models/config.py (2)
src/cache/cache_factory.py (1)
conversation_cache(20-49)src/configuration.py (1)
conversation_cache(162-170)
src/configuration.py (1)
src/models/config.py (1)
ConversationHistoryConfiguration(514-552)
src/cache/cache_factory.py (2)
src/models/config.py (2)
config(140-146)ConversationHistoryConfiguration(514-552)src/configuration.py (1)
conversation_cache(162-170)
tests/unit/models/config/test_conversation_cache.py (1)
src/models/config.py (2)
ConversationHistoryConfiguration(514-552)InMemoryCacheConfig(81-84)
tests/unit/cache/test_cache_factory.py (2)
src/models/config.py (3)
ConversationHistoryConfiguration(514-552)InMemoryCacheConfig(81-84)SQLiteDatabaseConfiguration(75-78)src/cache/cache_factory.py (2)
CacheFactory(16-49)conversation_cache(20-49)
⏰ 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). (4)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: e2e_tests (ci)
- GitHub Check: e2e_tests (azure)
🔇 Additional comments (6)
tests/unit/models/config/test_conversation_cache.py (2)
55-125: Backend-config vs type interaction tests remain valid after renameThe “no type but configured” and “multiple configurations” tests now use
ConversationHistoryConfigurationbut keep the same expectations as the model validator (requiringtypewhen any backend config is present and forbidding mixed backends). The rename here is purely mechanical and preserves behavior.
127-205: Per-backend happy-path and misconfiguration tests correctly target ConversationHistoryConfigurationAll backend‑specific tests (memory/sqlite/postgres) now instantiate
ConversationHistoryConfigurationand still:
- Assert correct wiring for valid configs, and
- Expect
ValidationErrorfor invalid or missing/incorrect sub‑configs.The changes are consistent and behaviorally neutral.
src/configuration.py (1)
10-24: Import update to ConversationHistoryConfiguration is correctSwitching the import to
ConversationHistoryConfigurationaligns this module with the renamed config model and the type ofConfiguration.conversation_cache.src/models/config.py (1)
596-613: Configuration.conversation_cache now correctly references ConversationHistoryConfigurationSetting
conversation_cache: ConversationHistoryConfiguration = Field(default_factory=ConversationHistoryConfiguration)keeps configuration construction simple and ensures a well‑typed default object is always present. This is consistent with howAppConfigandCacheFactoryconsume it and with the updated OpenAPI schema.No functional issues spotted here.
docs/openapi.json (1)
1875-1949: OpenAPI Configuration now points conversation_cache to ConversationHistoryConfigurationUpdating the
conversation_cacheproperty to$ref#/components/schemas/ConversationHistoryConfigurationkeeps the spec aligned with the renamed model while preserving the field name for backward compatibility. This wiring looks correct.docs/config.puml (1)
41-41: Terminology mismatch is intentional; rename only if part of broader cache→history initiative.The field
conversation_cache(48+ references across source code, tests, configs, and documentation) intentionally distinguishes between the configuration object (ConversationHistoryConfiguration) and its cache implementation. However, the fieldconversation_cachereferences the typeConversationHistoryConfiguration, creating a terminology mismatch.This distinction is deliberate:
conversation_cache_configurationproperty returns ConversationHistoryConfiguration (the data structure)conversation_cacheproperty returns Cache (the implementation)CacheFactory.conversation_cache()method uses "cache" terminologyIf this PR is not part of a systematic cache→history rename, leave the naming as-is (it's consistent with factory methods and distinguishes configuration from implementation). If this PR is renaming cache to history throughout, you'll need to rename this field to
conversation_historyand update all 48+ references (configuration model, properties, endpoints, tests, example configs, and documentation).
Description
LCORE-703: rename conversation cache to conversation history
Type of change
Related Tickets & Documents
Summary by CodeRabbit
Release Notes
Documentation
Refactor
Tests