-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add CachedViews #146
Open
iamkroot
wants to merge
14
commits into
beetbox:main
Choose a base branch
from
iamkroot:cache
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Add CachedViews #146
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
2696abc
views: Add CachedView
iamkroot 6dda361
views: Add ConfigHandleInvalidatedError and remove prints
iamkroot 9522958
tests: Add unit tests for cached views
iamkroot b891b3f
core: Remove some debug prints
iamkroot c4a2050
core: Simplify _recursive_invalidate for cached views
iamkroot 37491e8
core: Code cleanup for cached views
iamkroot d6a3722
core: Extract common code to CachedViewMixin
iamkroot 44cc666
cache: Also unset any handles on ancestors of a modified view
iamkroot a253275
Support get_handle on a CachedRootView
iamkroot 751537f
Move CachedViews to cache.py
iamkroot 17ed5f9
cache: Update tests
iamkroot 98e4982
core: Remove a misplaced type annotation
iamkroot da6c598
cache: Call template.get_default_value when view is missing
iamkroot b5444ef
cache: Simplify invalidation logic to match existing behaviour
iamkroot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
from typing import Dict, List | ||
|
||
from . import templates | ||
from .core import ROOT_NAME, Configuration, ConfigView, RootView, Subview | ||
|
||
|
||
class CachedHandle(object): | ||
"""Handle for a cached value computed by applying a template on the view. | ||
""" | ||
_INVALID = object() | ||
"""Sentinel object to denote that the cached value is out-of-date.""" | ||
|
||
def __init__(self, view: ConfigView, template=templates.REQUIRED) -> None: | ||
self.value = self._INVALID | ||
self.view = view | ||
self.template = template | ||
|
||
def get(self): | ||
"""Retreive the cached value from the handle. | ||
|
||
Will re-compute the value using `view.get(template)` if it has been | ||
invalidated. | ||
|
||
May raise a `NotFoundError` if the underlying view is missing. | ||
""" | ||
if self.value is self._INVALID: | ||
self.value = self.view.get(self.template) | ||
return self.value | ||
|
||
def _invalidate(self): | ||
"""Invalidate the cached value, will be repopulated on next `get()`. | ||
""" | ||
self.value = self._INVALID | ||
|
||
|
||
class CachedViewMixin: | ||
def __init__(self, *args, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
# keep track of all the handles from this view | ||
self.handles: List[CachedHandle] = [] | ||
# need to cache the subviews to be able to access their handles | ||
self.subviews: Dict[str, CachedConfigView] = {} | ||
|
||
def __getitem__(self, key) -> "CachedConfigView": | ||
try: | ||
return self.subviews[key] | ||
except KeyError: | ||
val = CachedConfigView(self, key) | ||
self.subviews[key] = val | ||
return val | ||
|
||
def __setitem__(self, key, value): | ||
subview: CachedConfigView = self[key] | ||
# invalidate the existing handles up and down the view tree | ||
subview._invalidate_descendants() | ||
self._invalidate_ancestors() | ||
|
||
return super().__setitem__(key, value) | ||
|
||
def _invalidate_ancestors(self): | ||
"""Invalidate the cached handles for all the views up the chain. | ||
|
||
This is to ensure that they aren't referring to stale values. | ||
""" | ||
parent = self | ||
while True: | ||
for handle in parent.handles: | ||
handle._invalidate() | ||
if parent.name == ROOT_NAME: | ||
break | ||
parent = parent.parent | ||
|
||
def _invalidate_descendants(self): | ||
"""Invalidate the handles for (sub)keys that were updated. | ||
""" | ||
for handle in self.handles: | ||
handle._invalidate() | ||
for subview in self.subviews.values(): | ||
subview._invalidate_descendants() | ||
|
||
def get_handle(self, template=templates.REQUIRED): | ||
"""Retreive a `CachedHandle` for the current view and template. | ||
""" | ||
handle = CachedHandle(self, template) | ||
self.handles.append(handle) | ||
return handle | ||
|
||
|
||
class CachedConfigView(CachedViewMixin, Subview): | ||
pass | ||
|
||
|
||
class CachedRootView(CachedViewMixin, RootView): | ||
pass | ||
|
||
|
||
class CachedConfiguration(CachedViewMixin, Configuration): | ||
pass |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import unittest | ||
|
||
import confuse | ||
from confuse.cache import CachedConfigView, CachedHandle, CachedRootView | ||
from confuse.templates import Sequence | ||
|
||
|
||
class CachedViewTest(unittest.TestCase): | ||
def setUp(self) -> None: | ||
self.config = CachedRootView([confuse.ConfigSource.of( | ||
{"a": ["b", "c"], | ||
"x": {"y": [1, 2], "w": "z", "p": {"q": 3}}})]) | ||
return super().setUp() | ||
|
||
def test_basic(self): | ||
view: CachedConfigView = self.config['x']['y'] | ||
handle: CachedHandle = view.get_handle(Sequence(int)) | ||
self.assertEqual(handle.get(), [1, 2]) | ||
|
||
def test_update(self): | ||
view: CachedConfigView = self.config['x']['y'] | ||
handle: CachedHandle = view.get_handle(Sequence(int)) | ||
self.config['x']['y'] = [4, 5] | ||
self.assertEqual(handle.get(), [4, 5]) | ||
|
||
def test_subview_update(self): | ||
view: CachedConfigView = self.config['x']['y'] | ||
handle: CachedHandle = view.get_handle(Sequence(int)) | ||
self.config['x'] = {'y': [4, 5]} | ||
self.assertEqual(handle.get(), [4, 5]) | ||
|
||
def test_missing(self): | ||
view: CachedConfigView = self.config['x']['y'] | ||
handle: CachedHandle = view.get_handle(Sequence(int)) | ||
|
||
self.config['x'] = {'p': [4, 5]} | ||
# new dict doesn't have a 'y' key, but according to the view-theory, | ||
# it will get the value from the older view that has been shadowed. | ||
self.assertEqual(handle.get(), [1, 2]) | ||
|
||
def test_missing2(self): | ||
view: CachedConfigView = self.config['x']['w'] | ||
handle = view.get_handle(str) | ||
self.assertEqual(handle.get(), 'z') | ||
|
||
self.config['x'] = {'y': [4, 5]} | ||
self.assertEqual(handle.get(), 'z') | ||
|
||
def test_list_update(self): | ||
view: CachedConfigView = self.config['a'][1] | ||
handle = view.get_handle(str) | ||
self.assertEqual(handle.get(), 'c') | ||
self.config['a'][1] = 'd' | ||
self.assertEqual(handle.get(), 'd') | ||
|
||
def test_root_update(self): | ||
root = self.config | ||
handle = self.config.get_handle({'a': Sequence(str)}) | ||
self.assertDictEqual(handle.get(), {'a': ['b', 'c']}) | ||
root['a'] = ['c', 'd'] | ||
self.assertDictEqual(handle.get(), {'a': ['c', 'd']}) | ||
|
||
def test_parent_invalidation(self): | ||
view: CachedConfigView = self.config['x']['p'] | ||
handle = view.get_handle(dict) | ||
self.assertEqual(handle.get(), {'q': 3}) | ||
self.config['x']['p']['q'] = 4 | ||
self.assertEqual(handle.get(), {'q': 4}) |
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.
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 don't know if this is a good idea, but just to try it out here: would it simplify things at all to keep the list of handles only on the
RootView
? That is, any time any data changes anywhere in the entire configuration, we'd invalidate everything. I know this is possibly a bit more inefficient, but it would centralize the tracking of handles and avoid the need for all views to keep track of their subviews for invalidation purposes.The reason I'm slightly nervous (perhaps unfoundedly) about the latter thing is that we have never before required that subviews be unique… that is, doing
config['foo']
twice in "normal" Confuse could give you back two different objects (equivalent objects, but different ones). These objects were short-lived and disposable; they generally did not stick around. Now we're saying that they must stick around, and in fact, that doingconfig['foo']
twice always gives you back exactly the same memoized view object. I'm just a tad worried that this means that, if there were some other way of constructing a view object onto the same "place" in the configuration hierarchy (such as via iteration?), it would not enjoy the same invalidation benefits. So this seems to place a few more restrictions on the way that views can be created, stored, and used.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 should mention that I could perhaps try prototyping this idea if it seems plausible to you!