validate cache consistency on first access #815
Conversation
* validate on startup * update on any plugin file change Signed-off-by: Jeffrey Martin <[email protected]>
| if not os.path.exists(self._user_plugin_cache_filename): | ||
| update_user_file = True |
There was a problem hiding this comment.
it looks like this is asserted before it's known whether there are any changes present (i.e. before it's known if any data will be written). should the mkdir on L79 etc. be skipped if there won't be changes?
There was a problem hiding this comment.
If self._user_plugin_cache_filename does not exist it will be written to even if there are no changes required, this conditional detects a need to copy the file which will be a write. The mkdir is to ensure the full path the file will be placed in exists.
| if mod_time > user_time: | ||
| # rebuild all for now, this can be more made more selective later | ||
| return False |
| ) as cache_file: | ||
| json.dump(local_cache, cache_file, cls=PluginEncoder, indent=2) | ||
| logging.debug("rebuilding plugin cache") | ||
| with PluginCache._mutex: |
| sorted_keys = sorted(list(plugin_dict.keys())) | ||
| local_cache[plugin_type] = {i: plugin_dict[i] for i in sorted_keys} |
There was a problem hiding this comment.
is this sorting redundant, seeing as the data goes into a set?
There was a problem hiding this comment.
The sorting reduces churn in the package file when we commit updates. In testing of the original addition of the cache I found the order of plugins is not consistent unless sorted.
There was a problem hiding this comment.
ah, ok. this might be relying on non-guaranteed behavior in dict but i guess we can deal with that if it rears its head.
jmartin-tech
force-pushed
the
feature/cached-plugin-enum-updates
branch
2 times, most recently
from
3545818 to
767157e
Compare
* raises assertion when package plugin cache is not found * test for removed module file * test missing package plugin cache Signed-off-by: Jeffrey Martin <[email protected]>
jmartin-tech
force-pushed
the
feature/cached-plugin-enum-updates
branch
from
767157e to
a6db301
Compare
| @pytest.mark.skipif( | ||
| sys.platform == "win32", | ||
| reason="Windows Github executor does not raise the ValueError", | ||
| ) |
There was a problem hiding this comment.
I don't like this add, however the test passes in a Win10 miniconda env on 3.10 and even in github actions on 3.12, which suggests the failure is github runner specific. Will dig deeper at another time and skip the Windows test for now.
Verify cache on startup without
importingfiles unless rebuild is required.This improves development workflow to ensure the that in progress plugins are found on
clilaunch, initial load times in development environments will delay for full rebuild.Further enhancement to consider not addressed here:
user localplugins to tracked in upgrade plugin structure #384user localsupport)interactivemode (no issue yet needs discussion on value)Testing:
Monitor
garak.logduring the following:rebuilding plugin cacherebuilding plugin cacherebuilding plugin cacheTests to be added shortly.