perf: cache trained ML model in learning_svc across operation completions#3328
Open
perf: cache trained ML model in learning_svc across operation completions#3328
Conversation
Add TTL-based caching to avoid unnecessary model rebuilds. Model is only rebuilt when marked dirty or after cache TTL expires.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a TTL-based cache for the LearningService ML model so it isn’t rebuilt after every operation completion, improving runtime performance while allowing explicit invalidation.
Changes:
- Introduces
_model_dirty/_model_built_atand TTL logic to skip unnecessary model rebuilds. - Adds
invalidate_model_cache()to force a rebuild on next use. - Adds default config (
model_cache_ttl_seconds) and unit tests covering cache behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
app/service/learning_svc.py |
Implements dirty+TTL cache logic and explicit invalidation for model rebuilding. |
conf/default.yml |
Adds a configurable TTL (model_cache_ttl_seconds) for the model cache. |
tests/test_learning_svc_cache.py |
Adds tests to validate initial state, invalidation, and rebuild/skip behavior. |
Comments suppressed due to low confidence (1)
app/service/learning_svc.py:41
- The guard checks
executor.command, but the regex scan usesexecutor.test. Ifexecutor.testcan beNone/empty whileexecutor.commandis present,re.findall(...)can fail (expects a string/bytes). Either scanexecutor.command(if that’s the intended source) or update the guard to match the field being scanned.
if executor.command:
variables = frozenset(re.findall(self.re_variable, executor.test))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/service/learning_svc.py
Outdated
| return parsers | ||
|
|
||
| async def build_model(self): | ||
| cache_ttl = self.get_config('model_cache_ttl_seconds') or 3600 |
app/service/learning_svc.py
Outdated
Comment on lines
38
to
44
| for ability in await self.get_service('data_svc').locate('abilities'): | ||
| for executor in ability.executors: | ||
| if executor.command: | ||
| variables = frozenset(re.findall(self.re_variable, executor.test)) | ||
| if len(variables) > 1: # relationships require at least 2 variables | ||
| self.model.add(variables) | ||
| self.model = set(self.model) |
app/service/learning_svc.py
Outdated
Comment on lines
+34
to
+46
| cache_ttl = self.get_config('model_cache_ttl_seconds') or 3600 | ||
| if not self._model_dirty and (time.monotonic() - self._model_built_at) < cache_ttl: | ||
| self.log.debug('Skipping model rebuild - cache still valid (TTL=%ds)', cache_ttl) | ||
| return | ||
| for ability in await self.get_service('data_svc').locate('abilities'): | ||
| for executor in ability.executors: | ||
| if executor.command: | ||
| variables = frozenset(re.findall(self.re_variable, executor.test)) | ||
| if len(variables) > 1: # relationships require at least 2 variables | ||
| self.model.add(variables) | ||
| self.model = set(self.model) | ||
| self._model_dirty = False | ||
| self._model_built_at = time.monotonic() |
tests/test_learning_svc_cache.py
Outdated
Comment on lines
+31
to
+35
| mock_data_svc = AsyncMock() | ||
| svc.get_service = MagicMock(return_value=mock_data_svc) | ||
| svc.get_config = MagicMock(return_value=3600) | ||
| asyncio.run(svc.build_model()) | ||
| mock_data_svc.locate.assert_not_called() |
| svc.invalidate_model_cache() | ||
| assert svc._model_dirty is True | ||
|
|
||
| def test_skip_rebuild_when_cache_valid(self): |
- Coerce cache_ttl to int (clamped >= 1s) since get_config() may return string - Rebuild model into a fresh local set to remove stale entries from deleted abilities before assigning to self.model - Fix test: explicitly set mock_data_svc.locate = AsyncMock() before assert_not_called() to avoid false-positive child mock assertion - Add test for TTL-expiry rebuild (dirty=False but TTL elapsed) - Add test for string TTL coercion path
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Learning service retrained model from scratch after each operation. Added cache. With tests.