-
-
Notifications
You must be signed in to change notification settings - Fork 416
fix: Prevent exponential custom-option id growth on re-customization #6904
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
Changes from 8 commits
ef262b1
68956c3
cb93307
a375344
1ba0721
c97f714
7b3015e
2a4a4a9
2a4f8ef
08006b8
8480d33
8aeab01
b44cbcb
c69b057
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -37,6 +37,7 @@ | |||||||||||||||||
| import difflib | ||||||||||||||||||
| import inspect | ||||||||||||||||||
| import pickle | ||||||||||||||||||
| import threading | ||||||||||||||||||
| import traceback | ||||||||||||||||||
| import typing as t | ||||||||||||||||||
| from collections import defaultdict | ||||||||||||||||||
|
|
@@ -1213,6 +1214,12 @@ class Store: | |||||||||||||||||
| _weakrefs = {} | ||||||||||||||||||
| _options_context = False | ||||||||||||||||||
|
|
||||||||||||||||||
| # Monotonic high-water mark for custom-option ids and the lock guarding | ||||||||||||||||||
| # its allocation. Ids are never reused, so concurrent customizations | ||||||||||||||||||
| # receive disjoint id blocks instead of colliding on a recomputed offset. | ||||||||||||||||||
| _id_counter = 0 | ||||||||||||||||||
| _id_lock = threading.Lock() | ||||||||||||||||||
|
|
||||||||||||||||||
| # Backend option caches | ||||||||||||||||||
| _lookup_cache = {} | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -1735,8 +1742,8 @@ def create_custom_trees(cls, obj, options, backend=None): | |||||||||||||||||
| """ | ||||||||||||||||||
| clones, id_mapping = {}, [] | ||||||||||||||||||
| obj_ids = cls.get_object_ids(obj) | ||||||||||||||||||
| offset = cls.id_offset() | ||||||||||||||||||
| obj_ids = [None] if len(obj_ids) == 0 else obj_ids | ||||||||||||||||||
| offset = cls.reserve_ids(len(obj_ids)) | ||||||||||||||||||
|
|
||||||||||||||||||
| used_obj_types = [(opt.split(".")[0],) for opt in options] | ||||||||||||||||||
| backend = backend or Store.current_backend | ||||||||||||||||||
|
|
@@ -1754,18 +1761,23 @@ def create_custom_trees(cls, obj, options, backend=None): | |||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| custom_options = Store.custom_options(backend=backend) | ||||||||||||||||||
| for tree_id in obj_ids: | ||||||||||||||||||
| # Relocate the object's trees to a fresh contiguous block above the | ||||||||||||||||||
| # offset. Ids are opaque keys into Store._custom_options; the only | ||||||||||||||||||
| # requirements are that the new ids are disjoint from existing keys | ||||||||||||||||||
| # (guaranteed by the offset) and that distinct old ids map to distinct | ||||||||||||||||||
| # new ids. Indexing by position keeps the maximum id linear in the | ||||||||||||||||||
| # number of live customizations; deriving it from the old id's value | ||||||||||||||||||
| # (tree_id + offset + 1) doubles the maximum on every re-customization. | ||||||||||||||||||
| for new_id, tree_id in enumerate(obj_ids, start=offset): | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, removed. The rationale it carried -- why ids are relocated by position ( |
||||||||||||||||||
| if tree_id is not None and tree_id in custom_options: | ||||||||||||||||||
| original = custom_options[tree_id] | ||||||||||||||||||
| clone = OptionTree( | ||||||||||||||||||
| items=original.items(), groups=original.groups, backend=original.backend | ||||||||||||||||||
| ) | ||||||||||||||||||
| clones[tree_id + offset + 1] = clone | ||||||||||||||||||
| id_mapping.append((tree_id, tree_id + offset + 1)) | ||||||||||||||||||
| else: | ||||||||||||||||||
| clone = OptionTree(groups=available_options.groups, backend=backend) | ||||||||||||||||||
| clones[offset] = clone | ||||||||||||||||||
| id_mapping.append((tree_id, offset)) | ||||||||||||||||||
| clones[new_id] = clone | ||||||||||||||||||
| id_mapping.append((tree_id, new_id)) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Nodes needed to ensure allowed_keywords is respected | ||||||||||||||||||
| for obj_type, opts in used_options.items(): | ||||||||||||||||||
|
|
@@ -1871,6 +1883,24 @@ def id_offset(cls): | |||||||||||||||||
| # If no backends defined (e.g. plotting not imported) return zero | ||||||||||||||||||
| return max(max_ids) if max_ids else 0 | ||||||||||||||||||
|
|
||||||||||||||||||
| @classmethod | ||||||||||||||||||
| def reserve_ids(cls, n): | ||||||||||||||||||
| """Atomically reserve a contiguous block of ``n`` fresh custom-option | ||||||||||||||||||
| ids and return its first id. | ||||||||||||||||||
|
|
||||||||||||||||||
| Ids are drawn from a monotonic counter that never decreases, so a | ||||||||||||||||||
| block is never reused even as store entries are reclaimed, and | ||||||||||||||||||
| concurrent customizations receive disjoint blocks rather than | ||||||||||||||||||
| colliding on a recomputed offset. The counter is floored by | ||||||||||||||||||
| ``id_offset`` so it also clears any ids already present in the store | ||||||||||||||||||
| (e.g. restored by unpickling). | ||||||||||||||||||
|
|
||||||||||||||||||
| """ | ||||||||||||||||||
| with Store._id_lock: | ||||||||||||||||||
| base = max(Store._id_counter, cls.id_offset()) | ||||||||||||||||||
| Store._id_counter = base + n | ||||||||||||||||||
| return base | ||||||||||||||||||
|
|
||||||||||||||||||
| @classmethod | ||||||||||||||||||
| def update_backends(cls, id_mapping, custom_trees, backend=None): | ||||||||||||||||||
| """Given the id_mapping from previous ids to new ids and the new | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import itertools | ||
|
|
||
| import numpy as np | ||
|
|
||
| import holoviews as hv | ||
|
|
@@ -111,3 +113,97 @@ def test_holomap_options(self): | |
|
|
||
| def test_holomap_options_empty_no_exception(self): | ||
| hv.HoloMap({0: hv.Image(np.random.rand(10, 10))}).options() | ||
|
|
||
|
|
||
| @mpl_skip | ||
| class TestStoreOptionsCustomIdGrowth: | ||
| """ | ||
| Re-customizing a retained, already-customized object must not grow the | ||
| custom-option ids exponentially. The buggy behaviour minted a new id of | ||
| ``old_id + max(all_ids) + 2``, which doubles the maximum id on every call | ||
| when the object already holds the store's largest id. | ||
| """ | ||
|
|
||
| def setup_method(self): | ||
| hv.Store.current_backend = "matplotlib" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should change this back in teardown method. Though, I can see this is not done for the other ones in the file either. def setup_method(self):
self._backend = hv.Store.current_backend
hv.Store.set_current_backend("matplotlib")
def teardown_method(self):
hv.Store.current_backend = self._backend |
||
|
|
||
| @staticmethod | ||
| def _max_custom_id(): | ||
| keys = list(hv.Store._custom_options.get("matplotlib", {}).keys()) | ||
| return max(keys) if keys else 0 | ||
|
|
||
| def test_recustomization_id_growth_is_not_exponential(self): | ||
| el = hv.Curve([1, 2, 3]) | ||
| max_ids = [] | ||
| for _ in range(20): | ||
| el = el.opts(color="red") | ||
| max_ids.append(self._max_custom_id()) | ||
|
|
||
| # The object carries a single custom-option id, so a correct | ||
| # relocation grows the maximum by a small constant per call (linear). | ||
| # The exponential bug instead grows it by roughly the current maximum | ||
| # each call, so per-step growth quickly exceeds any small bound. | ||
| growth = [b - a for a, b in itertools.pairwise(max_ids)] | ||
| assert max(growth) <= 4, f"id growth per call is not bounded: {max_ids}" | ||
|
|
||
| def test_cross_backend_recustomization_keeps_children_distinct(self): | ||
| # Children customized in bokeh hold distinct ids that are absent from | ||
| # the matplotlib store. Re-customizing the overlay in matplotlib with a | ||
| # spec targeting the children must relocate each child id to a distinct | ||
| # new id; collapsing them onto a single id aliases the children and | ||
| # loses their distinct bokeh styles. | ||
| c1 = hv.Curve([1, 2, 3]).opts(color="red", backend="bokeh") | ||
| c2 = hv.Curve([3, 2, 1]).opts(color="green", backend="bokeh") | ||
| ov = (c1 * c2).opts({"Curve": {"linewidth": 3}}, backend="matplotlib") | ||
|
|
||
| first, second = ov.Curve.I, ov.Curve.II | ||
| assert first.id != second.id | ||
| assert hv.Store.lookup_options("bokeh", first, "style").kwargs["color"] == "red" | ||
| assert hv.Store.lookup_options("bokeh", second, "style").kwargs["color"] == "green" | ||
|
|
||
| def test_overlay_subset_update_keeps_ids_bounded(self): | ||
| # A dashboard overlay where only a subset of curves receives fresh data | ||
| # each restyle cycle (e.g. jittered streaming updates where some curves | ||
| # have no new message). Curves that skip an update are the same retained | ||
| # objects carrying their previous ids; restyling the overlay relocates | ||
| # them, and deriving the new id from the old value doubled them every | ||
| # cycle. No static/reference curve is required -- the retention comes | ||
| # purely from the partial updates. | ||
| names = list("abc") | ||
| curves = {k: hv.Curve([(0, 0)], label=k) for k in names} | ||
| base = self._max_custom_id() | ||
| max_ids = [] | ||
| for cycle in range(15): | ||
| k = names[cycle % len(names)] # only one curve gets fresh data | ||
| curves[k] = hv.Curve([(x, x + cycle) for x in range(cycle + 2)], label=k) | ||
| hv.Overlay(list(curves.values())).opts({"Curve": {"linewidth": 2}}) | ||
| max_ids.append(self._max_custom_id() - base) | ||
|
|
||
| # Polynomial growth after the fix; exponential (doubling per cycle) | ||
| # before it, so the final value blows far past any polynomial bound. | ||
| assert max_ids[-1] < 500, f"id growth is not polynomial: {max_ids}" | ||
|
|
||
| def test_concurrent_recustomization_is_thread_safe(self): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This runs ~7s or so, should it be marked as slow/skipped somehow?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this is to test the n_workers = 8
barrier = threading.Barrier(n_workers)
def worker(w):
barrier.wait()
items = []
for i in range(120):
start_id = hv.StoreOptions.reserve_ids(i)
items.extend(range(start_id, start_id + i))
return items
with ThreadPoolExecutor(max_workers=n_workers) as executor:
total_items = list(executor.map(worker, range(n_workers)))
a = sum(total_items, []) # noqa: RUF017
assert len(a) == len(set(a))With that being said, I cannot get this or the current implementation to fail when I change the diff --git a/holoviews/core/options.py b/holoviews/core/options.py
index 304bb6b0b..d2ea01098 100644
--- a/holoviews/core/options.py
+++ b/holoviews/core/options.py
@@ -37,11 +37,12 @@ from __future__ import annotations
import difflib
import inspect
import pickle
+import sysconfig
import threading
import traceback
import typing as t
from collections import defaultdict
-from contextlib import contextmanager
+from contextlib import contextmanager, nullcontext
import numpy as np
import param
@@ -56,6 +57,9 @@ if t.TYPE_CHECKING:
from ..util.settings import OutputSettings
+_PYTHON_FREETHREADING = bool(sysconfig.get_config_var("Py_GIL_DISABLED"))
+
+
def __getattr__(name):
if name == "Opts":
from ..util.warnings import deprecated
@@ -1218,7 +1222,7 @@ class Store:
# its allocation. Ids are never reused, so concurrent customizations
# receive disjoint id blocks instead of colliding on a recomputed offset.
_id_counter = 0
- _id_lock = threading.Lock()
+ _id_lock = threading.Lock() if _PYTHON_FREETHREADING else nullcontext()
# Backend option caches
_lookup_cache = {}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am using regular Python 3.11. I had a different fix initially (see 4th commit in the branch, a375344). This lead to exceptions about unknown IDs (sporadic, not always) in our dashboard (background thread, potentially multiple connected sessions). 7b3015e was the answer to this, but I have not tested it extensively without the lock.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call on the focused test -- I've added essentially your version as On whether the lock is needed off free-threading: I'd keep it unconditional rather than gate it on
base = max(Store._id_counter, cls.id_offset()) # load + call id_offset()
Store._id_counter = base + n # separate storeThe GIL only makes a single bytecode atomic. The reason it's hard to reproduce is timing, not absence: a normal To isolate the lock itself, So |
||
| # Customizing from multiple threads must not collide on relocated ids. | ||
| # Allocating ids from a recomputed max(store)+1 offset gives concurrent | ||
| # calls the same block, so one thread's freshly stored tree is reused | ||
| # by another and the id is gone by the time it is propagated, raising | ||
| # "New option id ... does not match any option trees". future.result() | ||
| # re-raises any such error from the worker threads, failing the test. | ||
| import threading | ||
| from concurrent.futures import ThreadPoolExecutor | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move these to top-level |
||
|
|
||
| n_workers = 8 | ||
| barrier = threading.Barrier(n_workers) | ||
|
|
||
| def worker(w): | ||
| barrier.wait() | ||
| for i in range(60): | ||
| el = hv.Curve([1, 2, 3], label=f"w{w}") | ||
| for _ in range(4): | ||
| el = el.opts(linewidth=(i % 5) + 1) | ||
|
|
||
| with ThreadPoolExecutor(max_workers=n_workers) as executor: | ||
| futures = [executor.submit(worker, w) for w in range(n_workers)] | ||
| for future in futures: | ||
| future.result() | ||
Uh oh!
There was an error while loading. Please reload this page.
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 think this comment is needed. Maybe it could be limited to one line.
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.
Agreed -- trimmed to a single line pointing at
reserve_ids, where the monotonic, disjoint allocation is documented. I kept a one-line breadcrumb rather than dropping it entirely so the two bare class attributes (_id_counter,_id_lock) still hint at where the logic lives.