-
Notifications
You must be signed in to change notification settings - Fork 48
feat(caching): implements caching v0.1 #210
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
Conversation
| @@ -0,0 +1,351 @@ | |||
| """Manager for cache execution flow and state.""" | |||
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.
Can we move all the caching uitls to a caching sub folder?
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.
addressed in #e9e00987b01ccf6352cb7b00a324d7ff1fa213e7
| ) | ||
|
|
||
|
|
||
| class CacheManager: |
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 functions here are verbose and long.
Is this realy necessary?
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.
addressed in #34e372e6b5b91730c884ca201bb2d4ee6601f412
src/askui/utils/cache_migration.py
Outdated
| """Raised when cache migration fails.""" | ||
|
|
||
|
|
||
| class CacheMigration: |
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.
I would remove the cache migration class.
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.
addressed in #e6603eb703cdb81fa21676fe08423b55c8c07d0b
| from askui.models.shared.settings import CacheFile | ||
|
|
||
|
|
||
| class CacheValidator(ABC): |
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.
Are we using the Validators already? Otherwise remove it.
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.
Yes we do. Per default, we use a composition (OR conjunction) of StepFailureCountValidator (invalidates if same step fails too many times), TotalFailureRateValidator (invalidates if overall failure rate is too high), and StaleCacheValidator (invalidates if cache is too hold and has at least one failure).
| ) | ||
| from askui.models.shared.tools import ToolCollection | ||
| from askui.utils.placeholder_handler import PlaceholderHandler | ||
| from askui.utils.placeholder_identifier import identify_placeholders | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class CacheWriter: |
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.
We need to decouple here logic,
I would prefere that we introduce Domain Objects and delegate the placeholder replacement to them.
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.
addressed in #21abeae0bdb843cd47d16f40f13555322f20d7d7
…gent, to make the flow simpler
…o avoid confusion with CacheManager class
…rts (`CachedExecutionManager` i/o `Agent`)
…VisionAgent API for reporters
…s to improve readability of the code
…ill be done automatically when reading v0.0 cache files
…ParameterHandler more consistently and renames `placeholder` to `caching_parameter`.
21abeae to
9d94a7b
Compare
…anager to Agent, to fix FalseNegative cache invalidation
BREAKING CHANGE: CachingSettings follow a new (simpler) format now
This PR implements the latest caching mechanism v0.1 with the following features