-
Notifications
You must be signed in to change notification settings - Fork 4
Rework caching. #48
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
base: main
Are you sure you want to change the base?
Rework caching. #48
Conversation
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.
Pull request overview
This PR refactors the caching architecture to use a type-safe CacheParameter interface pattern instead of string arrays. The changes ensure that cache parameters are used correctly and consistently across the codebase through new ArchUnit tests that enforce architectural constraints.
Key Changes
- Introduced
CacheParameterinterface with concrete implementations (ClassifierCacheParameter,EmbeddingCacheParameter) to replace string array parameters - Moved
ClassifierCacheKeyandEmbeddingCacheKeyto dedicated subpackages (classifierandembedding) - Added comprehensive ArchUnit tests to enforce that CacheKey implementations use CacheParameter correctly and that CacheParameter implementations include all fields
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/cache/CacheParameter.java |
New interface defining the contract for cache parameters with a parameters() method |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/cache/classifier/ClassifierCacheParameter.java |
New record implementing CacheParameter for classifier caching, with backward compatibility for temperature |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/cache/embedding/EmbeddingCacheParameter.java |
New record implementing CacheParameter for embedding caching |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/cache/classifier/ClassifierCacheKey.java |
Moved to classifier subpackage and updated to use ClassifierCacheParameter in static factory method |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/cache/embedding/EmbeddingCacheKey.java |
Moved to embedding subpackage and updated to use EmbeddingCacheParameter in static factory method |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/cache/CacheKey.java |
Updated imports to reflect moved cache key classes |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/cache/CacheManager.java |
Simplified API to accept CacheParameter instead of string arrays; removed unused getCache(Path, boolean) method |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/classifier/ChatLanguageModelProvider.java |
Changed getCacheParameters() to return ClassifierCacheParameter instead of string array |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/classifier/SimpleClassifier.java |
Updated to use new cacheParameters() method and ClassifierCacheKey.of() with CacheParameter |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/classifier/ReasoningClassifier.java |
Updated to use new cacheParameters() method and ClassifierCacheKey.of() with CacheParameter |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/preprocessor/SummarizePreprocessor.java |
Updated to use new cacheParameters() method and ClassifierCacheKey.of() with CacheParameter |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/embeddingcreator/CachedEmbeddingCreator.java |
Updated to use EmbeddingCacheParameter throughout and in EmbeddingCacheKey.of() calls |
src/test/java/edu/kit/kastel/sdq/lissa/ratlr/ArchitectureTest.java |
Added two new ArchUnit rules to enforce CacheKey and CacheParameter implementation contracts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/java/edu/kit/kastel/sdq/lissa/ratlr/ArchitectureTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package edu.kit.kastel.sdq.lissa.ratlr.cache.embedding; | ||
|
|
||
| import edu.kit.kastel.sdq.lissa.ratlr.cache.CacheParameter; | ||
|
|
Copilot
AI
Dec 11, 2025
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.
The EmbeddingCacheParameter record lacks a class-level javadoc comment explaining its purpose and usage. Other CacheParameter implementations should follow similar documentation standards. Consider adding javadoc that explains what this parameter represents and how it's used in the caching system.
| /** | |
| * Represents a cache parameter for embedding operations, encapsulating the model name. | |
| * <p> | |
| * This parameter is used as a key in the caching system to uniquely identify | |
| * cache entries related to embeddings for a specific model. Other {@link CacheParameter} | |
| * implementations should follow similar documentation standards. | |
| */ |
src/main/java/edu/kit/kastel/sdq/lissa/ratlr/cache/classifier/ClassifierCacheParameter.java
Show resolved
Hide resolved
| .filter(m -> !m.getName() | ||
| .equals("parameters")) // parameters() is for cache naming, not | ||
| // for key construction |
Copilot
AI
Dec 11, 2025
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.
The comment explaining why the parameters() method is filtered out is split across multiple lines in an inconsistent way. The comment starts on line 170 but is part of the filter statement on line 169. This should be reformatted to have the complete comment on a single line or moved to a separate line above the filter statement for better readability.
| .filter(m -> !m.getName() | |
| .equals("parameters")) // parameters() is for cache naming, not | |
| // for key construction | |
| // parameters() is for cache naming, not for key construction | |
| .filter(m -> !m.getName().equals("parameters")) |
| public String parameters() { | ||
| if (temperature == 0.0) { | ||
| return String.join("_", modelName, String.valueOf(seed)); | ||
| } else { | ||
| return String.join("_", modelName, String.valueOf(seed), String.valueOf(temperature)); | ||
| } |
Copilot
AI
Dec 11, 2025
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.
The parameters() method only includes temperature in the output when it's not 0.0, but the method always accesses the temperature field (via the condition check). This satisfies the architectural rule that requires all fields to be accessed. However, consider whether this conditional inclusion could lead to cache collisions when temperature is explicitly set to 0.0 versus when it's the default value. The current implementation appears intentional for backward compatibility, but it's worth documenting this behavior more explicitly in the class javadoc.



Now it shall be ensured that CacheParameters are used correctly.