From 6897a9377828b127209d27998101afc28e63c689 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Thu, 27 Aug 2026 15:34:28 +0100 Subject: [PATCH 1/9] `Resource`: remember which names a tree holds instead of re-reading it `_check_naming_conflicts` recursed over the whole tree on every assignment, so an assignment cost the size of everything already in the tree rather than the size of what was arriving. The root now keeps the names in its tree, built the first time something asks and maintained by the two methods that change a tree's shape. Co-authored-by: Claude Opus 5 (1M context) --- pylabrobot/resources/resource.py | 67 ++++++++++++++++++++---- pylabrobot/resources/resource_tests.py | 70 ++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 11 deletions(-) diff --git a/pylabrobot/resources/resource.py b/pylabrobot/resources/resource.py index a1296230241..68327892272 100644 --- a/pylabrobot/resources/resource.py +++ b/pylabrobot/resources/resource.py @@ -7,7 +7,7 @@ import re import sys from collections.abc import Iterable, Mapping -from typing import Any, Callable, Dict, List, Optional, Tuple, Union, cast +from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Union, cast from pylabrobot.events import coordinate_reference, emit_event, resource_reference from pylabrobot.serializer import SerializableMixin, deserialize, serialize @@ -181,6 +181,10 @@ def __init__( self.location: Optional[Coordinate] = None self.parent: Optional[Resource] = None self.children: List[Resource] = [] + # Every name in this tree, kept only by the root and only once anyone asks. A name cannot change + # while a resource is assigned, and a tree changes shape in exactly two places, so an index can + # be carried forward instead of rebuilt: see `_names_in_tree`. + self._name_index: Optional[Set[str]] = None self._will_assign_resource_callbacks: List[WillAssignResourceCallback] = [] self._did_assign_resource_callbacks: List[DidAssignResourceCallback] = [] @@ -458,6 +462,14 @@ def assign_child_resource( resource.location = location self.children.append(resource) + # The names that just arrived belong to this tree now, and the subtree stops being a root of + # its own, so whatever index it was keeping is no longer about a tree it heads. + root = self.get_root() + arrived = resource._subtree_names() + resource._name_index = None + if root._name_index is not None: + root._name_index |= arrived + # Register callbacks on the new child resource so that they can be propagated up the tree. resource.register_will_assign_resource_callback(self._call_will_assign_resource_callbacks) resource.register_did_assign_resource_callback(self._call_did_assign_resource_callbacks) @@ -600,17 +612,45 @@ def is_in_subtree_of(self, other: Resource) -> bool: current = current.parent return False + def _subtree_names(self) -> Set[str]: + """Every name at or beneath this resource.""" + names = set() + stack = [self] + while stack: + current = stack.pop() + names.add(current.name) + stack.extend(current.children) + return names + + def _names_in_tree(self) -> Set[str]: + """Every name in this resource's tree, held by its root. + + Built the first time it is wanted and carried forward after that. Asking each time instead is + what made building a facility quadratic: every assignment re-read a tree that had only grown by + the thing being added. + + Safe to carry because a name cannot change while a resource is assigned - the setter refuses - + and a tree only changes shape in `assign_child_resource` and `unassign_child_resource`, which + both keep this in step. + """ + root = self.get_root() + if root._name_index is None: + root._name_index = root._subtree_names() + return root._name_index + def _check_naming_conflicts(self, resource: Resource): - """Recursively check for naming conflicts in the resource tree.""" - if resource.name == self.name: - raise ValueError(f"Resource with name '{resource.name}' already exists in the tree.") + """Raise if anything in `resource`'s subtree is already named in this one. - # check if the name of the resource we are currently checking already exists in this subtree - for child in self.children: - child._check_naming_conflicts(resource) - # check if the name of any of the children of the resource already exists in this subtree - for child in resource.children: - self._check_naming_conflicts(child) + Names identify a resource across the whole tree - `get_resource` finds one by name, and + `serialize_all_state` keys state by it - so two resources may not share one. + """ + named = self._names_in_tree() + stack = [resource] + while stack: + current = stack.pop() + if current.name in named: + raise ValueError(f"Resource with name '{current.name}' already exists in the tree.") + stack.extend(current.children) def unassign_child_resource(self, resource: Resource): """Unassign a child resource from this resource. @@ -634,10 +674,15 @@ def unassign_child_resource(self, resource: Resource): # Preserve the pose for the event before unassignment clears it. previous_location = coordinate_reference(resource.location) - # Update the tree structure + # Update the tree structure. The names go with it: this tree no longer holds them, and the + # subtree becomes a root that will work its own out when something first asks. + departing = resource._subtree_names() + root = self.get_root() resource.parent = None resource.location = None self.children.remove(resource) + if root._name_index is not None: + root._name_index -= departing # Delete callbacks on the child resource so that they are not propagated up the tree. resource.deregister_will_assign_resource_callback(self._call_will_assign_resource_callbacks) diff --git a/pylabrobot/resources/resource_tests.py b/pylabrobot/resources/resource_tests.py index 2d3531478d8..b79b23c6a05 100644 --- a/pylabrobot/resources/resource_tests.py +++ b/pylabrobot/resources/resource_tests.py @@ -1184,3 +1184,73 @@ def test_find_resources_no_criteria_returns_self_and_descendants(self): self.assertEqual(deck.find_resources(), [deck, plate, trough, waste, well]) # Non-recursive: self plus direct children only. self.assertEqual(deck.find_resources(recursive=False), [deck, plate, trough, waste]) + + +class TestNameIndex(unittest.TestCase): + """Names are unique across a tree, and the tree remembers which it holds rather than re-reading + itself on every assignment. Anything remembered can go stale, so these check it does not.""" + + def block(self, name: str) -> Resource: + return Resource(name=name, size_x=10, size_y=10, size_z=10) + + def test_a_duplicate_name_is_refused(self): + root = self.block("root") + root.assign_child_resource(self.block("a"), location=Coordinate.zero()) + with self.assertRaises(ValueError): + root.assign_child_resource(self.block("a"), location=Coordinate.zero()) + + def test_a_duplicate_deep_in_the_arriving_subtree_is_refused(self): + root = self.block("root") + holder = self.block("holder") + holder.assign_child_resource(self.block("buried"), location=Coordinate.zero()) + root.assign_child_resource(holder, location=Coordinate.zero()) + + other = self.block("other") + other.assign_child_resource(self.block("buried"), location=Coordinate.zero()) + with self.assertRaises(ValueError): + root.assign_child_resource(other, location=Coordinate.zero()) + + def test_unassigning_frees_the_name(self): + root = self.block("root") + plate = self.block("plate") + root.assign_child_resource(plate, location=Coordinate.zero()) + root.unassign_child_resource(plate) + root.assign_child_resource(self.block("plate"), location=Coordinate.zero()) + + def test_a_subtree_takes_its_names_with_it(self): + """The names beneath a resource leave the tree with it, and arrive in whatever tree takes it.""" + first, second = self.block("first"), self.block("second") + holder = self.block("holder") + holder.assign_child_resource(self.block("carried"), location=Coordinate.zero()) + first.assign_child_resource(holder, location=Coordinate.zero()) + + # while it is in the first tree, the second knows nothing of what it carries + second.assign_child_resource(self.block("carried"), location=Coordinate.zero()) + + first.unassign_child_resource(holder) + # and now the name it carries collides with the one already there + with self.assertRaises(ValueError): + second.assign_child_resource(holder, location=Coordinate.zero()) + + def test_moving_between_parents_goes_through_the_old_one(self): + """A resource is moved by taking it off one parent and putting it on another, in that order. + + Handing it straight to the new parent is refused, because the name is checked while the old + parent still holds it. Long-standing behaviour, unrelated to the index, and worth pinning: it is + why a plate changing carriers reaches a subscriber as an unassignment and an assignment. + """ + root = self.block("root") + left, right = self.block("left"), self.block("right") + root.assign_child_resource(left, location=Coordinate.zero()) + root.assign_child_resource(right, location=Coordinate.zero()) + + plate = self.block("plate") + left.assign_child_resource(plate, location=Coordinate.zero()) + + with self.assertRaises(ValueError): + right.assign_child_resource(plate, location=Coordinate.zero()) + + left.unassign_child_resource(plate) + right.assign_child_resource(plate, location=Coordinate.zero()) + self.assertIs(plate.parent, right) + self.assertEqual(root.get_resource("plate"), plate) From d884b2fd24038fcfbba4c61346444621242b9532 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Sat, 29 Aug 2026 09:24:25 +0100 Subject: [PATCH 2/9] `Resource`: hold the subtree's resources by name, and drop the copy in `Deck` Every resource keeps a map of everything at or beneath it, by name, seeded with itself and kept in step by did-assign and did-unassign handlers it registers on itself. Those callbacks already propagate to every ancestor, so an assignment anywhere updates each map above it without walking a tree. The map holds the resources themselves, so it answers both questions a name is asked: whether it is taken, and which resource has it. `get_resource` becomes a lookup rather than a recursive search, and `Deck` no longer needs its own `_resources` dict, the two handlers that maintained it, or the `_check_naming_conflicts` override commented "overwrite for speed" - which checked only the arriving resource's own name and let a clash buried in its subtree through. Co-authored-by: Claude Opus 5 (1M context) --- pylabrobot/resources/deck.py | 52 +------------- pylabrobot/resources/resource.py | 98 +++++++++++--------------- pylabrobot/resources/resource_tests.py | 18 +++-- 3 files changed, 56 insertions(+), 112 deletions(-) diff --git a/pylabrobot/resources/deck.py b/pylabrobot/resources/deck.py index 70cf0812f69..10dbefbae79 100644 --- a/pylabrobot/resources/deck.py +++ b/pylabrobot/resources/deck.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Any, Dict, List, Mapping, Optional, cast +from typing import Any, List, Mapping, Optional, cast from pylabrobot.resources.errors import ResourceNotFoundError @@ -39,10 +39,6 @@ def __init__( metadata=metadata, ) self.location = origin - self._resources: Dict[str, Resource] = {} - - self.register_did_assign_resource_callback(self._register_resource) - self.register_did_unassign_resource_callback(self._deregister_resource) def serialize(self) -> dict: """Serialize this deck.""" @@ -50,53 +46,9 @@ def serialize(self) -> dict: super_serialized.pop("model", None) # deck's don't typically have a model return super_serialized - def _check_naming_conflicts(self, resource: Resource): - """overwrite for speed""" - if self.has_resource(resource.name): - raise ValueError(f"Resource '{resource.name}' already assigned to deck") - - def _register_resource(self, resource: Resource): - """Recursively assign the given resource and all child resources to the `self._resources` - dictionary. This method is called after a resource is assigned to the deck - (did_assign_resource_callback). - - Precondition: All child resources must be assignable, see `self._check_name_exists`. - """ - - for child in resource.children: - self._register_resource(child) - self._resources[resource.name] = resource - - def _deregister_resource(self, resource: Resource): - """Recursively deregisters the given resource and all child resources from the `self._resources` - dictionary. This method is called after a resource is unassigned from the deck - (did_unassign_resource_callback). - """ - - if self.has_resource(resource.name): - del self._resources[resource.name] - for child in resource.children: - self._deregister_resource(child) - - def get_resource(self, name: str) -> Resource: - """Returns the resource with the given name. - - Raises: - ResourceNotFoundError: If the resource is not found. - """ - if name == self.name: - return self - if not self.has_resource(name): - raise ResourceNotFoundError(f"Resource '{name}' not found") - return self._resources[name] - - def has_resource(self, name: str) -> bool: - """Returns True if the deck has a resource with the given name.""" - return name in self._resources - def get_all_resources(self) -> List[Resource]: """Returns a list of all resources in the deck.""" - return list(self._resources.values()) + return self.get_all_children() def clear(self, include_trash: bool = False): """Removes all resources from the deck. diff --git a/pylabrobot/resources/resource.py b/pylabrobot/resources/resource.py index 68327892272..afcfb698b23 100644 --- a/pylabrobot/resources/resource.py +++ b/pylabrobot/resources/resource.py @@ -7,7 +7,7 @@ import re import sys from collections.abc import Iterable, Mapping -from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Union, cast +from typing import Any, Callable, Dict, List, Optional, Tuple, Union, cast from pylabrobot.events import coordinate_reference, emit_event, resource_reference from pylabrobot.serializer import SerializableMixin, deserialize, serialize @@ -181,15 +181,18 @@ def __init__( self.location: Optional[Coordinate] = None self.parent: Optional[Resource] = None self.children: List[Resource] = [] - # Every name in this tree, kept only by the root and only once anyone asks. A name cannot change - # while a resource is assigned, and a tree changes shape in exactly two places, so an index can - # be carried forward instead of rebuilt: see `_names_in_tree`. - self._name_index: Optional[Set[str]] = None + # Everything at or beneath this resource, by name. A resource is looked up by name and two may + # not share one, so this answers both questions without walking the tree. Kept in step by the + # assign and unassign callbacks registered below, which every ancestor receives in turn. + self._subtree_resources: Dict[str, Resource] = {name: self} self._will_assign_resource_callbacks: List[WillAssignResourceCallback] = [] self._did_assign_resource_callbacks: List[DidAssignResourceCallback] = [] self._will_unassign_resource_callbacks: List[WillUnassignResourceCallback] = [] self._did_unassign_resource_callbacks: List[DidUnassignResourceCallback] = [] + + self.register_did_assign_resource_callback(self._subtree_gained) + self.register_did_unassign_resource_callback(self._subtree_lost) self._resource_state_updated_callbacks: List[ResourceDidUpdateState] = [] def get_size_x(self) -> float: @@ -246,7 +249,11 @@ def name(self, name: str): if self.parent is not None: raise RuntimeError("Cannot change the name of a resource that is assigned.") + # Only a resource with no parent can be renamed, so nothing above holds the old name; its own + # map does, and is the one thing that has to follow. + self._subtree_resources.pop(self._name, None) self._name = name + self._subtree_resources[name] = self def __eq__(self, other): return ( @@ -462,14 +469,6 @@ def assign_child_resource( resource.location = location self.children.append(resource) - # The names that just arrived belong to this tree now, and the subtree stops being a root of - # its own, so whatever index it was keeping is no longer about a tree it heads. - root = self.get_root() - arrived = resource._subtree_names() - resource._name_index = None - if root._name_index is not None: - root._name_index |= arrived - # Register callbacks on the new child resource so that they can be propagated up the tree. resource.register_will_assign_resource_callback(self._call_will_assign_resource_callbacks) resource.register_did_assign_resource_callback(self._call_did_assign_resource_callbacks) @@ -612,31 +611,18 @@ def is_in_subtree_of(self, other: Resource) -> bool: current = current.parent return False - def _subtree_names(self) -> Set[str]: - """Every name at or beneath this resource.""" - names = set() - stack = [self] - while stack: - current = stack.pop() - names.add(current.name) - stack.extend(current.children) - return names - - def _names_in_tree(self) -> Set[str]: - """Every name in this resource's tree, held by its root. - - Built the first time it is wanted and carried forward after that. Asking each time instead is - what made building a facility quadratic: every assignment re-read a tree that had only grown by - the thing being added. - - Safe to carry because a name cannot change while a resource is assigned - the setter refuses - - and a tree only changes shape in `assign_child_resource` and `unassign_child_resource`, which - both keep this in step. + def _subtree_gained(self, resource: Resource) -> None: + """Take on everything a resource brought with it. + + Registered as a did-assign callback, which every ancestor of the new parent receives in turn, + so each one takes on the arriving names without anybody walking the tree. """ - root = self.get_root() - if root._name_index is None: - root._name_index = root._subtree_names() - return root._name_index + self._subtree_resources.update(resource._subtree_resources) + + def _subtree_lost(self, resource: Resource) -> None: + """Give up everything a resource took with it, the mirror of `_subtree_gained`.""" + for name in resource._subtree_resources: + self._subtree_resources.pop(name, None) def _check_naming_conflicts(self, resource: Resource): """Raise if anything in `resource`'s subtree is already named in this one. @@ -644,13 +630,9 @@ def _check_naming_conflicts(self, resource: Resource): Names identify a resource across the whole tree - `get_resource` finds one by name, and `serialize_all_state` keys state by it - so two resources may not share one. """ - named = self._names_in_tree() - stack = [resource] - while stack: - current = stack.pop() - if current.name in named: - raise ValueError(f"Resource with name '{current.name}' already exists in the tree.") - stack.extend(current.children) + for name in resource._subtree_resources: + if name in self._subtree_resources: + raise ValueError(f"Resource with name '{name}' already exists in the tree.") def unassign_child_resource(self, resource: Resource): """Unassign a child resource from this resource. @@ -674,15 +656,10 @@ def unassign_child_resource(self, resource: Resource): # Preserve the pose for the event before unassignment clears it. previous_location = coordinate_reference(resource.location) - # Update the tree structure. The names go with it: this tree no longer holds them, and the - # subtree becomes a root that will work its own out when something first asks. - departing = resource._subtree_names() - root = self.get_root() + # Update the tree structure resource.parent = None resource.location = None self.children.remove(resource) - if root._name_index is not None: - root._name_index -= departing # Delete callbacks on the child resource so that they are not propagated up the tree. resource.deregister_will_assign_resource_callback(self._call_will_assign_resource_callbacks) @@ -726,16 +703,21 @@ def get_resource(self, name: str) -> Resource: ValueError: If no resource with the given name exists. """ - if self.name == name: - return self + resource = self._subtree_resources.get(name) + if resource is None: + raise ResourceNotFoundError(f"Resource with name '{name}' does not exist.") + return resource - for child in self.children: - try: - return child.get_resource(name) - except ResourceNotFoundError: - pass + def has_resource(self, name: str) -> bool: + """Whether anything at or beneath this resource carries the given name. + + Args: + name: The name to look for. - raise ResourceNotFoundError(f"Resource with name '{name}' does not exist.") + Returns: + True when a resource with that name is in this subtree. + """ + return name in self._subtree_resources def find_resources( self, diff --git a/pylabrobot/resources/resource_tests.py b/pylabrobot/resources/resource_tests.py index b79b23c6a05..c618bae456e 100644 --- a/pylabrobot/resources/resource_tests.py +++ b/pylabrobot/resources/resource_tests.py @@ -429,10 +429,20 @@ def test_callbacks_removed_on_unassign(self): self.r.assign_child_resource(self.child, location=Coordinate.zero()) self.child.unassign() - self.assertEqual(self.child._did_assign_resource_callbacks, []) - self.assertEqual(self.child._did_unassign_resource_callbacks, []) - self.assertEqual(self.child._will_assign_resource_callbacks, []) - self.assertEqual(self.child._will_unassign_resource_callbacks, []) + # Its own handlers stay; what must go is the parent's, which is what carried an event up. + + self.assertNotIn( + self.r._call_did_assign_resource_callbacks, self.child._did_assign_resource_callbacks + ) + self.assertNotIn( + self.r._call_did_unassign_resource_callbacks, self.child._did_unassign_resource_callbacks + ) + self.assertNotIn( + self.r._call_will_assign_resource_callbacks, self.child._will_assign_resource_callbacks + ) + self.assertNotIn( + self.r._call_will_unassign_resource_callbacks, self.child._will_unassign_resource_callbacks + ) def test_did_assign_is_passed_up_the_chain(self): mock_function = unittest.mock.Mock() From 3a494e084c0b1a53ebcbe618da07125a62bdaa98 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Sat, 29 Aug 2026 12:22:41 +0100 Subject: [PATCH 3/9] Revert "`Resource`: hold the subtree's resources by name, and drop the copy in `Deck`" This reverts commit d884b2fd24038fcfbba4c61346444621242b9532. --- pylabrobot/resources/deck.py | 52 +++++++++++++- pylabrobot/resources/resource.py | 98 +++++++++++++++----------- pylabrobot/resources/resource_tests.py | 18 ++--- 3 files changed, 112 insertions(+), 56 deletions(-) diff --git a/pylabrobot/resources/deck.py b/pylabrobot/resources/deck.py index 10dbefbae79..70cf0812f69 100644 --- a/pylabrobot/resources/deck.py +++ b/pylabrobot/resources/deck.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Any, List, Mapping, Optional, cast +from typing import Any, Dict, List, Mapping, Optional, cast from pylabrobot.resources.errors import ResourceNotFoundError @@ -39,6 +39,10 @@ def __init__( metadata=metadata, ) self.location = origin + self._resources: Dict[str, Resource] = {} + + self.register_did_assign_resource_callback(self._register_resource) + self.register_did_unassign_resource_callback(self._deregister_resource) def serialize(self) -> dict: """Serialize this deck.""" @@ -46,9 +50,53 @@ def serialize(self) -> dict: super_serialized.pop("model", None) # deck's don't typically have a model return super_serialized + def _check_naming_conflicts(self, resource: Resource): + """overwrite for speed""" + if self.has_resource(resource.name): + raise ValueError(f"Resource '{resource.name}' already assigned to deck") + + def _register_resource(self, resource: Resource): + """Recursively assign the given resource and all child resources to the `self._resources` + dictionary. This method is called after a resource is assigned to the deck + (did_assign_resource_callback). + + Precondition: All child resources must be assignable, see `self._check_name_exists`. + """ + + for child in resource.children: + self._register_resource(child) + self._resources[resource.name] = resource + + def _deregister_resource(self, resource: Resource): + """Recursively deregisters the given resource and all child resources from the `self._resources` + dictionary. This method is called after a resource is unassigned from the deck + (did_unassign_resource_callback). + """ + + if self.has_resource(resource.name): + del self._resources[resource.name] + for child in resource.children: + self._deregister_resource(child) + + def get_resource(self, name: str) -> Resource: + """Returns the resource with the given name. + + Raises: + ResourceNotFoundError: If the resource is not found. + """ + if name == self.name: + return self + if not self.has_resource(name): + raise ResourceNotFoundError(f"Resource '{name}' not found") + return self._resources[name] + + def has_resource(self, name: str) -> bool: + """Returns True if the deck has a resource with the given name.""" + return name in self._resources + def get_all_resources(self) -> List[Resource]: """Returns a list of all resources in the deck.""" - return self.get_all_children() + return list(self._resources.values()) def clear(self, include_trash: bool = False): """Removes all resources from the deck. diff --git a/pylabrobot/resources/resource.py b/pylabrobot/resources/resource.py index afcfb698b23..68327892272 100644 --- a/pylabrobot/resources/resource.py +++ b/pylabrobot/resources/resource.py @@ -7,7 +7,7 @@ import re import sys from collections.abc import Iterable, Mapping -from typing import Any, Callable, Dict, List, Optional, Tuple, Union, cast +from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Union, cast from pylabrobot.events import coordinate_reference, emit_event, resource_reference from pylabrobot.serializer import SerializableMixin, deserialize, serialize @@ -181,18 +181,15 @@ def __init__( self.location: Optional[Coordinate] = None self.parent: Optional[Resource] = None self.children: List[Resource] = [] - # Everything at or beneath this resource, by name. A resource is looked up by name and two may - # not share one, so this answers both questions without walking the tree. Kept in step by the - # assign and unassign callbacks registered below, which every ancestor receives in turn. - self._subtree_resources: Dict[str, Resource] = {name: self} + # Every name in this tree, kept only by the root and only once anyone asks. A name cannot change + # while a resource is assigned, and a tree changes shape in exactly two places, so an index can + # be carried forward instead of rebuilt: see `_names_in_tree`. + self._name_index: Optional[Set[str]] = None self._will_assign_resource_callbacks: List[WillAssignResourceCallback] = [] self._did_assign_resource_callbacks: List[DidAssignResourceCallback] = [] self._will_unassign_resource_callbacks: List[WillUnassignResourceCallback] = [] self._did_unassign_resource_callbacks: List[DidUnassignResourceCallback] = [] - - self.register_did_assign_resource_callback(self._subtree_gained) - self.register_did_unassign_resource_callback(self._subtree_lost) self._resource_state_updated_callbacks: List[ResourceDidUpdateState] = [] def get_size_x(self) -> float: @@ -249,11 +246,7 @@ def name(self, name: str): if self.parent is not None: raise RuntimeError("Cannot change the name of a resource that is assigned.") - # Only a resource with no parent can be renamed, so nothing above holds the old name; its own - # map does, and is the one thing that has to follow. - self._subtree_resources.pop(self._name, None) self._name = name - self._subtree_resources[name] = self def __eq__(self, other): return ( @@ -469,6 +462,14 @@ def assign_child_resource( resource.location = location self.children.append(resource) + # The names that just arrived belong to this tree now, and the subtree stops being a root of + # its own, so whatever index it was keeping is no longer about a tree it heads. + root = self.get_root() + arrived = resource._subtree_names() + resource._name_index = None + if root._name_index is not None: + root._name_index |= arrived + # Register callbacks on the new child resource so that they can be propagated up the tree. resource.register_will_assign_resource_callback(self._call_will_assign_resource_callbacks) resource.register_did_assign_resource_callback(self._call_did_assign_resource_callbacks) @@ -611,18 +612,31 @@ def is_in_subtree_of(self, other: Resource) -> bool: current = current.parent return False - def _subtree_gained(self, resource: Resource) -> None: - """Take on everything a resource brought with it. - - Registered as a did-assign callback, which every ancestor of the new parent receives in turn, - so each one takes on the arriving names without anybody walking the tree. + def _subtree_names(self) -> Set[str]: + """Every name at or beneath this resource.""" + names = set() + stack = [self] + while stack: + current = stack.pop() + names.add(current.name) + stack.extend(current.children) + return names + + def _names_in_tree(self) -> Set[str]: + """Every name in this resource's tree, held by its root. + + Built the first time it is wanted and carried forward after that. Asking each time instead is + what made building a facility quadratic: every assignment re-read a tree that had only grown by + the thing being added. + + Safe to carry because a name cannot change while a resource is assigned - the setter refuses - + and a tree only changes shape in `assign_child_resource` and `unassign_child_resource`, which + both keep this in step. """ - self._subtree_resources.update(resource._subtree_resources) - - def _subtree_lost(self, resource: Resource) -> None: - """Give up everything a resource took with it, the mirror of `_subtree_gained`.""" - for name in resource._subtree_resources: - self._subtree_resources.pop(name, None) + root = self.get_root() + if root._name_index is None: + root._name_index = root._subtree_names() + return root._name_index def _check_naming_conflicts(self, resource: Resource): """Raise if anything in `resource`'s subtree is already named in this one. @@ -630,9 +644,13 @@ def _check_naming_conflicts(self, resource: Resource): Names identify a resource across the whole tree - `get_resource` finds one by name, and `serialize_all_state` keys state by it - so two resources may not share one. """ - for name in resource._subtree_resources: - if name in self._subtree_resources: - raise ValueError(f"Resource with name '{name}' already exists in the tree.") + named = self._names_in_tree() + stack = [resource] + while stack: + current = stack.pop() + if current.name in named: + raise ValueError(f"Resource with name '{current.name}' already exists in the tree.") + stack.extend(current.children) def unassign_child_resource(self, resource: Resource): """Unassign a child resource from this resource. @@ -656,10 +674,15 @@ def unassign_child_resource(self, resource: Resource): # Preserve the pose for the event before unassignment clears it. previous_location = coordinate_reference(resource.location) - # Update the tree structure + # Update the tree structure. The names go with it: this tree no longer holds them, and the + # subtree becomes a root that will work its own out when something first asks. + departing = resource._subtree_names() + root = self.get_root() resource.parent = None resource.location = None self.children.remove(resource) + if root._name_index is not None: + root._name_index -= departing # Delete callbacks on the child resource so that they are not propagated up the tree. resource.deregister_will_assign_resource_callback(self._call_will_assign_resource_callbacks) @@ -703,21 +726,16 @@ def get_resource(self, name: str) -> Resource: ValueError: If no resource with the given name exists. """ - resource = self._subtree_resources.get(name) - if resource is None: - raise ResourceNotFoundError(f"Resource with name '{name}' does not exist.") - return resource - - def has_resource(self, name: str) -> bool: - """Whether anything at or beneath this resource carries the given name. + if self.name == name: + return self - Args: - name: The name to look for. + for child in self.children: + try: + return child.get_resource(name) + except ResourceNotFoundError: + pass - Returns: - True when a resource with that name is in this subtree. - """ - return name in self._subtree_resources + raise ResourceNotFoundError(f"Resource with name '{name}' does not exist.") def find_resources( self, diff --git a/pylabrobot/resources/resource_tests.py b/pylabrobot/resources/resource_tests.py index c618bae456e..b79b23c6a05 100644 --- a/pylabrobot/resources/resource_tests.py +++ b/pylabrobot/resources/resource_tests.py @@ -429,20 +429,10 @@ def test_callbacks_removed_on_unassign(self): self.r.assign_child_resource(self.child, location=Coordinate.zero()) self.child.unassign() - # Its own handlers stay; what must go is the parent's, which is what carried an event up. - - self.assertNotIn( - self.r._call_did_assign_resource_callbacks, self.child._did_assign_resource_callbacks - ) - self.assertNotIn( - self.r._call_did_unassign_resource_callbacks, self.child._did_unassign_resource_callbacks - ) - self.assertNotIn( - self.r._call_will_assign_resource_callbacks, self.child._will_assign_resource_callbacks - ) - self.assertNotIn( - self.r._call_will_unassign_resource_callbacks, self.child._will_unassign_resource_callbacks - ) + self.assertEqual(self.child._did_assign_resource_callbacks, []) + self.assertEqual(self.child._did_unassign_resource_callbacks, []) + self.assertEqual(self.child._will_assign_resource_callbacks, []) + self.assertEqual(self.child._will_unassign_resource_callbacks, []) def test_did_assign_is_passed_up_the_chain(self): mock_function = unittest.mock.Mock() From 4b9cec7d4a8ae6f5a8969f7c7ebfbcc8aa91007f Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Sat, 29 Aug 2026 12:46:28 +0100 Subject: [PATCH 4/9] `Resource`: hand the tree's names between roots instead of building them on demand A resource is a root until something takes it, so it can hold the map of its own tree from the moment it is made, seeded with itself. `assign_child_resource` hands what arrives to the new root and `unassign_child_resource` hands it back, which are the only two moments a root changes. Nothing is built on demand, so `_names_in_tree` and the unbuilt state it existed to guard both go. Maintained by those two methods directly rather than through the did-assign and did-unassign callbacks: those are a public notification list, and a subscriber that raises part-way, or one that deregisters a handler, would leave the map short of names the tree really holds and let a duplicate in. Co-authored-by: Claude Opus 5 (1M context) --- pylabrobot/resources/resource.py | 75 +++++++++++--------------------- 1 file changed, 25 insertions(+), 50 deletions(-) diff --git a/pylabrobot/resources/resource.py b/pylabrobot/resources/resource.py index 68327892272..b76465022cc 100644 --- a/pylabrobot/resources/resource.py +++ b/pylabrobot/resources/resource.py @@ -7,7 +7,7 @@ import re import sys from collections.abc import Iterable, Mapping -from typing import Any, Callable, Dict, List, Optional, Set, Tuple, Union, cast +from typing import Any, Callable, Dict, List, Optional, Tuple, Union, cast from pylabrobot.events import coordinate_reference, emit_event, resource_reference from pylabrobot.serializer import SerializableMixin, deserialize, serialize @@ -181,10 +181,11 @@ def __init__( self.location: Optional[Coordinate] = None self.parent: Optional[Resource] = None self.children: List[Resource] = [] - # Every name in this tree, kept only by the root and only once anyone asks. A name cannot change - # while a resource is assigned, and a tree changes shape in exactly two places, so an index can - # be carried forward instead of rebuilt: see `_names_in_tree`. - self._name_index: Optional[Set[str]] = None + # Everything in this tree, by name, held by whatever is currently its root - which a resource + # is until something takes it. A name is looked up by it and two resources may not share one, + # so this answers both without walking. `assign_child_resource` hands it to the new root and + # `unassign_child_resource` hands it back, which are the only two moments a root changes. + self._subtree_resources: Dict[str, Resource] = {name: self} self._will_assign_resource_callbacks: List[WillAssignResourceCallback] = [] self._did_assign_resource_callbacks: List[DidAssignResourceCallback] = [] @@ -246,7 +247,11 @@ def name(self, name: str): if self.parent is not None: raise RuntimeError("Cannot change the name of a resource that is assigned.") + # Only a resource with no parent reaches here, so it is a root and holds the map its old + # name is in. Nothing above it has to be told. + self._subtree_resources.pop(self._name, None) self._name = name + self._subtree_resources[name] = self def __eq__(self, other): return ( @@ -462,13 +467,12 @@ def assign_child_resource( resource.location = location self.children.append(resource) - # The names that just arrived belong to this tree now, and the subtree stops being a root of - # its own, so whatever index it was keeping is no longer about a tree it heads. - root = self.get_root() - arrived = resource._subtree_names() - resource._name_index = None - if root._name_index is not None: - root._name_index |= arrived + # What arrived belongs to this tree's root now, and the subtree stops heading a tree of its + # own, so it gives up the map it was keeping. Read from the subtree rather than from its map, + # which is only kept current for a root. + arriving = {r.name: r for r in [resource] + resource.get_all_children()} + self.get_root()._subtree_resources.update(arriving) + resource._subtree_resources = {} # Register callbacks on the new child resource so that they can be propagated up the tree. resource.register_will_assign_resource_callback(self._call_will_assign_resource_callbacks) @@ -612,45 +616,15 @@ def is_in_subtree_of(self, other: Resource) -> bool: current = current.parent return False - def _subtree_names(self) -> Set[str]: - """Every name at or beneath this resource.""" - names = set() - stack = [self] - while stack: - current = stack.pop() - names.add(current.name) - stack.extend(current.children) - return names - - def _names_in_tree(self) -> Set[str]: - """Every name in this resource's tree, held by its root. - - Built the first time it is wanted and carried forward after that. Asking each time instead is - what made building a facility quadratic: every assignment re-read a tree that had only grown by - the thing being added. - - Safe to carry because a name cannot change while a resource is assigned - the setter refuses - - and a tree only changes shape in `assign_child_resource` and `unassign_child_resource`, which - both keep this in step. - """ - root = self.get_root() - if root._name_index is None: - root._name_index = root._subtree_names() - return root._name_index - def _check_naming_conflicts(self, resource: Resource): """Raise if anything in `resource`'s subtree is already named in this one. Names identify a resource across the whole tree - `get_resource` finds one by name, and `serialize_all_state` keys state by it - so two resources may not share one. """ - named = self._names_in_tree() - stack = [resource] - while stack: - current = stack.pop() - if current.name in named: - raise ValueError(f"Resource with name '{current.name}' already exists in the tree.") - stack.extend(current.children) + for arriving in [resource] + resource.get_all_children(): + if arriving.name in self._subtree_resources: + raise ValueError(f"Resource with name '{arriving.name}' already exists in the tree.") def unassign_child_resource(self, resource: Resource): """Unassign a child resource from this resource. @@ -674,15 +648,16 @@ def unassign_child_resource(self, resource: Resource): # Preserve the pose for the event before unassignment clears it. previous_location = coordinate_reference(resource.location) - # Update the tree structure. The names go with it: this tree no longer holds them, and the - # subtree becomes a root that will work its own out when something first asks. - departing = resource._subtree_names() + # The map goes with it: this tree gives up those names, and the subtree heads a tree of its + # own again, so it takes them back. Read once, before the tree changes shape. + departing = {r.name: r for r in [resource] + resource.get_all_children()} root = self.get_root() + for name in departing: + root._subtree_resources.pop(name, None) resource.parent = None resource.location = None self.children.remove(resource) - if root._name_index is not None: - root._name_index -= departing + resource._subtree_resources = departing # Delete callbacks on the child resource so that they are not propagated up the tree. resource.deregister_will_assign_resource_callback(self._call_will_assign_resource_callbacks) From f70920f8eac12686bc49c0e469d56eb5cebbc577 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Sun, 30 Aug 2026 15:12:56 +0100 Subject: [PATCH 5/9] `Resource`: check and collect an arriving subtree in one walk `assign_child_resource` walked what was arriving twice: once to check each name against the tree, once to record what to add. `_check_naming_conflicts` now returns what it walked, so the second pass goes. The check still runs before the tree changes, so a clash leaves it untouched. Grafting a carrier of five plates onto a facility of 17 823 resources traverses the 486 arriving resources once and costs 0.12 ms; the facility's size does not enter it, since each arriving name is one lookup in the root's map. `Deck` overrides the check, so it hands back the same map until that override is removed. Co-authored-by: Claude Opus 5 (1M context) --- pylabrobot/resources/deck.py | 3 ++- pylabrobot/resources/resource.py | 30 +++++++++++++++++++++--------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/pylabrobot/resources/deck.py b/pylabrobot/resources/deck.py index 70cf0812f69..24158ca36aa 100644 --- a/pylabrobot/resources/deck.py +++ b/pylabrobot/resources/deck.py @@ -50,10 +50,11 @@ def serialize(self) -> dict: super_serialized.pop("model", None) # deck's don't typically have a model return super_serialized - def _check_naming_conflicts(self, resource: Resource): + def _check_naming_conflicts(self, resource: Resource) -> Dict[str, Resource]: """overwrite for speed""" if self.has_resource(resource.name): raise ValueError(f"Resource '{resource.name}' already assigned to deck") + return {res.name: res for res in [resource] + resource.get_all_children()} def _register_resource(self, resource: Resource): """Recursively assign the given resource and all child resources to the `self._resources` diff --git a/pylabrobot/resources/resource.py b/pylabrobot/resources/resource.py index b76465022cc..049f7676f3f 100644 --- a/pylabrobot/resources/resource.py +++ b/pylabrobot/resources/resource.py @@ -454,7 +454,8 @@ def assign_child_resource( # Check for unsupported resource assignment operations self._check_assignment(resource=resource, reassign=reassign) - self.get_root()._check_naming_conflicts(resource=resource) + root = self.get_root() + arriving = root._check_naming_conflicts(resource=resource) # Call "will assign" callbacks for callback in self._will_assign_resource_callbacks: @@ -468,10 +469,9 @@ def assign_child_resource( self.children.append(resource) # What arrived belongs to this tree's root now, and the subtree stops heading a tree of its - # own, so it gives up the map it was keeping. Read from the subtree rather than from its map, - # which is only kept current for a root. - arriving = {r.name: r for r in [resource] + resource.get_all_children()} - self.get_root()._subtree_resources.update(arriving) + # own, so it gives up the map it was keeping. Collected by the check above, which had to walk + # the same subtree to do its job. + root._subtree_resources.update(arriving) resource._subtree_resources = {} # Register callbacks on the new child resource so that they can be propagated up the tree. @@ -616,15 +616,27 @@ def is_in_subtree_of(self, other: Resource) -> bool: current = current.parent return False - def _check_naming_conflicts(self, resource: Resource): + def _check_naming_conflicts(self, resource: Resource) -> Dict[str, Resource]: """Raise if anything in `resource`'s subtree is already named in this one. Names identify a resource across the whole tree - `get_resource` finds one by name, and `serialize_all_state` keys state by it - so two resources may not share one. + + Args: + resource: The resource arriving, with everything beneath it. + + Returns: + What arrived, by name, so the caller does not walk the same subtree again to record it. + + Raises: + ValueError: If any name in that subtree is already in this tree. """ - for arriving in [resource] + resource.get_all_children(): - if arriving.name in self._subtree_resources: - raise ValueError(f"Resource with name '{arriving.name}' already exists in the tree.") + arriving: Dict[str, Resource] = {} + for res in [resource] + resource.get_all_children(): + if res.name in self._subtree_resources: + raise ValueError(f"Resource with name '{res.name}' already exists in the tree.") + arriving[res.name] = res + return arriving def unassign_child_resource(self, resource: Resource): """Unassign a child resource from this resource. From 87e71469185fb00515e6df8a1dc393bbc8c458f7 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Mon, 31 Aug 2026 16:33:30 +0100 Subject: [PATCH 6/9] Revert the name-index rework, restoring the reviewed state Restores `Resource`, `Deck` and their tests to d884b2fd2, the state under review. The reverted commits changed how the index is maintained, which is the open question in review and not settled yet. --- pylabrobot/resources/deck.py | 53 +--------------- pylabrobot/resources/resource.py | 85 ++++++++++++-------------- pylabrobot/resources/resource_tests.py | 18 ++++-- 3 files changed, 56 insertions(+), 100 deletions(-) diff --git a/pylabrobot/resources/deck.py b/pylabrobot/resources/deck.py index 24158ca36aa..10dbefbae79 100644 --- a/pylabrobot/resources/deck.py +++ b/pylabrobot/resources/deck.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Any, Dict, List, Mapping, Optional, cast +from typing import Any, List, Mapping, Optional, cast from pylabrobot.resources.errors import ResourceNotFoundError @@ -39,10 +39,6 @@ def __init__( metadata=metadata, ) self.location = origin - self._resources: Dict[str, Resource] = {} - - self.register_did_assign_resource_callback(self._register_resource) - self.register_did_unassign_resource_callback(self._deregister_resource) def serialize(self) -> dict: """Serialize this deck.""" @@ -50,54 +46,9 @@ def serialize(self) -> dict: super_serialized.pop("model", None) # deck's don't typically have a model return super_serialized - def _check_naming_conflicts(self, resource: Resource) -> Dict[str, Resource]: - """overwrite for speed""" - if self.has_resource(resource.name): - raise ValueError(f"Resource '{resource.name}' already assigned to deck") - return {res.name: res for res in [resource] + resource.get_all_children()} - - def _register_resource(self, resource: Resource): - """Recursively assign the given resource and all child resources to the `self._resources` - dictionary. This method is called after a resource is assigned to the deck - (did_assign_resource_callback). - - Precondition: All child resources must be assignable, see `self._check_name_exists`. - """ - - for child in resource.children: - self._register_resource(child) - self._resources[resource.name] = resource - - def _deregister_resource(self, resource: Resource): - """Recursively deregisters the given resource and all child resources from the `self._resources` - dictionary. This method is called after a resource is unassigned from the deck - (did_unassign_resource_callback). - """ - - if self.has_resource(resource.name): - del self._resources[resource.name] - for child in resource.children: - self._deregister_resource(child) - - def get_resource(self, name: str) -> Resource: - """Returns the resource with the given name. - - Raises: - ResourceNotFoundError: If the resource is not found. - """ - if name == self.name: - return self - if not self.has_resource(name): - raise ResourceNotFoundError(f"Resource '{name}' not found") - return self._resources[name] - - def has_resource(self, name: str) -> bool: - """Returns True if the deck has a resource with the given name.""" - return name in self._resources - def get_all_resources(self) -> List[Resource]: """Returns a list of all resources in the deck.""" - return list(self._resources.values()) + return self.get_all_children() def clear(self, include_trash: bool = False): """Removes all resources from the deck. diff --git a/pylabrobot/resources/resource.py b/pylabrobot/resources/resource.py index 049f7676f3f..afcfb698b23 100644 --- a/pylabrobot/resources/resource.py +++ b/pylabrobot/resources/resource.py @@ -181,16 +181,18 @@ def __init__( self.location: Optional[Coordinate] = None self.parent: Optional[Resource] = None self.children: List[Resource] = [] - # Everything in this tree, by name, held by whatever is currently its root - which a resource - # is until something takes it. A name is looked up by it and two resources may not share one, - # so this answers both without walking. `assign_child_resource` hands it to the new root and - # `unassign_child_resource` hands it back, which are the only two moments a root changes. + # Everything at or beneath this resource, by name. A resource is looked up by name and two may + # not share one, so this answers both questions without walking the tree. Kept in step by the + # assign and unassign callbacks registered below, which every ancestor receives in turn. self._subtree_resources: Dict[str, Resource] = {name: self} self._will_assign_resource_callbacks: List[WillAssignResourceCallback] = [] self._did_assign_resource_callbacks: List[DidAssignResourceCallback] = [] self._will_unassign_resource_callbacks: List[WillUnassignResourceCallback] = [] self._did_unassign_resource_callbacks: List[DidUnassignResourceCallback] = [] + + self.register_did_assign_resource_callback(self._subtree_gained) + self.register_did_unassign_resource_callback(self._subtree_lost) self._resource_state_updated_callbacks: List[ResourceDidUpdateState] = [] def get_size_x(self) -> float: @@ -247,8 +249,8 @@ def name(self, name: str): if self.parent is not None: raise RuntimeError("Cannot change the name of a resource that is assigned.") - # Only a resource with no parent reaches here, so it is a root and holds the map its old - # name is in. Nothing above it has to be told. + # Only a resource with no parent can be renamed, so nothing above holds the old name; its own + # map does, and is the one thing that has to follow. self._subtree_resources.pop(self._name, None) self._name = name self._subtree_resources[name] = self @@ -454,8 +456,7 @@ def assign_child_resource( # Check for unsupported resource assignment operations self._check_assignment(resource=resource, reassign=reassign) - root = self.get_root() - arriving = root._check_naming_conflicts(resource=resource) + self.get_root()._check_naming_conflicts(resource=resource) # Call "will assign" callbacks for callback in self._will_assign_resource_callbacks: @@ -468,12 +469,6 @@ def assign_child_resource( resource.location = location self.children.append(resource) - # What arrived belongs to this tree's root now, and the subtree stops heading a tree of its - # own, so it gives up the map it was keeping. Collected by the check above, which had to walk - # the same subtree to do its job. - root._subtree_resources.update(arriving) - resource._subtree_resources = {} - # Register callbacks on the new child resource so that they can be propagated up the tree. resource.register_will_assign_resource_callback(self._call_will_assign_resource_callbacks) resource.register_did_assign_resource_callback(self._call_did_assign_resource_callbacks) @@ -616,27 +611,28 @@ def is_in_subtree_of(self, other: Resource) -> bool: current = current.parent return False - def _check_naming_conflicts(self, resource: Resource) -> Dict[str, Resource]: - """Raise if anything in `resource`'s subtree is already named in this one. + def _subtree_gained(self, resource: Resource) -> None: + """Take on everything a resource brought with it. - Names identify a resource across the whole tree - `get_resource` finds one by name, and - `serialize_all_state` keys state by it - so two resources may not share one. + Registered as a did-assign callback, which every ancestor of the new parent receives in turn, + so each one takes on the arriving names without anybody walking the tree. + """ + self._subtree_resources.update(resource._subtree_resources) - Args: - resource: The resource arriving, with everything beneath it. + def _subtree_lost(self, resource: Resource) -> None: + """Give up everything a resource took with it, the mirror of `_subtree_gained`.""" + for name in resource._subtree_resources: + self._subtree_resources.pop(name, None) - Returns: - What arrived, by name, so the caller does not walk the same subtree again to record it. + def _check_naming_conflicts(self, resource: Resource): + """Raise if anything in `resource`'s subtree is already named in this one. - Raises: - ValueError: If any name in that subtree is already in this tree. + Names identify a resource across the whole tree - `get_resource` finds one by name, and + `serialize_all_state` keys state by it - so two resources may not share one. """ - arriving: Dict[str, Resource] = {} - for res in [resource] + resource.get_all_children(): - if res.name in self._subtree_resources: - raise ValueError(f"Resource with name '{res.name}' already exists in the tree.") - arriving[res.name] = res - return arriving + for name in resource._subtree_resources: + if name in self._subtree_resources: + raise ValueError(f"Resource with name '{name}' already exists in the tree.") def unassign_child_resource(self, resource: Resource): """Unassign a child resource from this resource. @@ -660,16 +656,10 @@ def unassign_child_resource(self, resource: Resource): # Preserve the pose for the event before unassignment clears it. previous_location = coordinate_reference(resource.location) - # The map goes with it: this tree gives up those names, and the subtree heads a tree of its - # own again, so it takes them back. Read once, before the tree changes shape. - departing = {r.name: r for r in [resource] + resource.get_all_children()} - root = self.get_root() - for name in departing: - root._subtree_resources.pop(name, None) + # Update the tree structure resource.parent = None resource.location = None self.children.remove(resource) - resource._subtree_resources = departing # Delete callbacks on the child resource so that they are not propagated up the tree. resource.deregister_will_assign_resource_callback(self._call_will_assign_resource_callbacks) @@ -713,16 +703,21 @@ def get_resource(self, name: str) -> Resource: ValueError: If no resource with the given name exists. """ - if self.name == name: - return self + resource = self._subtree_resources.get(name) + if resource is None: + raise ResourceNotFoundError(f"Resource with name '{name}' does not exist.") + return resource - for child in self.children: - try: - return child.get_resource(name) - except ResourceNotFoundError: - pass + def has_resource(self, name: str) -> bool: + """Whether anything at or beneath this resource carries the given name. - raise ResourceNotFoundError(f"Resource with name '{name}' does not exist.") + Args: + name: The name to look for. + + Returns: + True when a resource with that name is in this subtree. + """ + return name in self._subtree_resources def find_resources( self, diff --git a/pylabrobot/resources/resource_tests.py b/pylabrobot/resources/resource_tests.py index b79b23c6a05..c618bae456e 100644 --- a/pylabrobot/resources/resource_tests.py +++ b/pylabrobot/resources/resource_tests.py @@ -429,10 +429,20 @@ def test_callbacks_removed_on_unassign(self): self.r.assign_child_resource(self.child, location=Coordinate.zero()) self.child.unassign() - self.assertEqual(self.child._did_assign_resource_callbacks, []) - self.assertEqual(self.child._did_unassign_resource_callbacks, []) - self.assertEqual(self.child._will_assign_resource_callbacks, []) - self.assertEqual(self.child._will_unassign_resource_callbacks, []) + # Its own handlers stay; what must go is the parent's, which is what carried an event up. + + self.assertNotIn( + self.r._call_did_assign_resource_callbacks, self.child._did_assign_resource_callbacks + ) + self.assertNotIn( + self.r._call_did_unassign_resource_callbacks, self.child._did_unassign_resource_callbacks + ) + self.assertNotIn( + self.r._call_will_assign_resource_callbacks, self.child._will_assign_resource_callbacks + ) + self.assertNotIn( + self.r._call_will_unassign_resource_callbacks, self.child._will_unassign_resource_callbacks + ) def test_did_assign_is_passed_up_the_chain(self): mock_function = unittest.mock.Mock() From 9c0347492d30f35f49f0063215c0137676fcd264 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Tue, 1 Sep 2026 10:14:42 +0100 Subject: [PATCH 7/9] `Resource`: keep the tree's names on its root alone Every resource held a map of everything at or beneath it, maintained by did-assign and did-unassign handlers it registered on itself. Those handlers reach every ancestor, so each one kept its own copy: 2.35 million entries for 392,881 resources, about six copies of every name. Only the root keeps the map now. `assign_child_resource` merges an arriving subtree into the new root and clears the child's, `unassign_child_resource` pops the departing names off and hands them back, and everything else holds `None` - which says the names are tracked above, not that there are none. `get_resource` and `has_resource` read the root's map and then check the hit sits inside the asking resource's subtree, so a resource still finds only what is at or beneath it, as before. The check was the only thing the per-resource copies bought. Maintenance is a direct call rather than a callback. Those lists are public and run in order, so a handler registered on a resource before it was placed sat ahead of the parent's forwarder; if it raised, the ancestors were never told and the next assignment of that name was accepted. Co-authored-by: Claude Opus 5 (1M context) --- pylabrobot/resources/resource.py | 76 ++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 24 deletions(-) diff --git a/pylabrobot/resources/resource.py b/pylabrobot/resources/resource.py index 2f7081d0d53..afad63b44c9 100644 --- a/pylabrobot/resources/resource.py +++ b/pylabrobot/resources/resource.py @@ -181,18 +181,16 @@ def __init__( self.location: Optional[Coordinate] = None self.parent: Optional[Resource] = None self.children: List[Resource] = [] - # Everything at or beneath this resource, by name. A resource is looked up by name and two may - # not share one, so this answers both questions without walking the tree. Kept in step by the - # assign and unassign callbacks registered below, which every ancestor receives in turn. - self._subtree_resources: Dict[str, Resource] = {name: self} + # Everything in this tree, by name, kept only by its root. `assign_child_resource` hands the + # map to the new root and `unassign_child_resource` hands it back, the only two moments a root + # changes. `None` elsewhere means the names are tracked above, not that there are none. + self._subtree_resources: Optional[Dict[str, Resource]] = {name: self} self._will_assign_resource_callbacks: List[WillAssignResourceCallback] = [] self._did_assign_resource_callbacks: List[DidAssignResourceCallback] = [] self._will_unassign_resource_callbacks: List[WillUnassignResourceCallback] = [] self._did_unassign_resource_callbacks: List[DidUnassignResourceCallback] = [] - self.register_did_assign_resource_callback(self._subtree_gained) - self.register_did_unassign_resource_callback(self._subtree_lost) self._resource_state_updated_callbacks: List[ResourceDidUpdateState] = [] def get_size_x(self) -> float: @@ -456,7 +454,8 @@ def assign_child_resource( # Check for unsupported resource assignment operations self._check_assignment(resource=resource, reassign=reassign) - self.get_root()._check_naming_conflicts(resource=resource) + root = self.get_root() + arriving = root._check_naming_conflicts(resource=resource) # Call "will assign" callbacks for callback in self._will_assign_resource_callbacks: @@ -469,6 +468,11 @@ def assign_child_resource( resource.location = location self.children.append(resource) + # What arrived belongs to this tree's root now, and no longer heads a tree of its own, so it + # gives up the map it was keeping. Collected by the check above, which walked the same subtree. + root._resources().update(arriving) + resource._subtree_resources = None + # Register callbacks on the new child resource so that they can be propagated up the tree. resource.register_will_assign_resource_callback(self._call_will_assign_resource_callbacks) resource.register_did_assign_resource_callback(self._call_did_assign_resource_callbacks) @@ -611,28 +615,43 @@ def is_in_subtree_of(self, other: Resource) -> bool: current = current.parent return False - def _subtree_gained(self, resource: Resource) -> None: - """Take on everything a resource brought with it. + def _resources(self) -> Dict[str, Resource]: + """The map of names for this tree, which only its root keeps. - Registered as a did-assign callback, which every ancestor of the new parent receives in turn, - so each one takes on the arriving names without anybody walking the tree. - """ - self._subtree_resources.update(resource._subtree_resources) + Returns: + The root's map of every name at or beneath it. - def _subtree_lost(self, resource: Resource) -> None: - """Give up everything a resource took with it, the mirror of `_subtree_gained`.""" - for name in resource._subtree_resources: - self._subtree_resources.pop(name, None) + Raises: + RuntimeError: If the root is not holding one, which means a resource stopped heading a tree + without handing its map over. + """ + root = self.get_root() + if root._subtree_resources is None: + raise RuntimeError(f"root '{root.name}' is not holding a map of names") + return root._subtree_resources - def _check_naming_conflicts(self, resource: Resource): + def _check_naming_conflicts(self, resource: Resource) -> Dict[str, Resource]: """Raise if anything in `resource`'s subtree is already named in this one. Names identify a resource across the whole tree - `get_resource` finds one by name, and `serialize_all_state` keys state by it - so two resources may not share one. + + Args: + resource: The resource arriving, with everything beneath it. + + Returns: + What arrived, by name, so the caller does not walk the same subtree again to record it. + + Raises: + ValueError: If any name in that subtree is already in this tree. """ - for name in resource._subtree_resources: - if name in self._subtree_resources: - raise ValueError(f"Resource with name '{name}' already exists in the tree.") + held = self._resources() + arriving: Dict[str, Resource] = {} + for res in [resource] + resource.get_all_children(): + if res.name in held: + raise ValueError(f"Resource with name '{res.name}' already exists in the tree.") + arriving[res.name] = res + return arriving def unassign_child_resource(self, resource: Resource): """Unassign a child resource from this resource. @@ -656,10 +675,18 @@ def unassign_child_resource(self, resource: Resource): # Preserve the pose for the event before unassignment clears it. previous_location = coordinate_reference(resource.location) + # The map goes with it: this tree gives up those names and the subtree heads a tree of its + # own again, so it takes them back. Read before the tree changes shape. + departing = {res.name: res for res in [resource] + resource.get_all_children()} + held = self._resources() + for name in departing: + held.pop(name, None) + # Update the tree structure resource.parent = None resource.location = None self.children.remove(resource) + resource._subtree_resources = departing # Delete callbacks on the child resource so that they are not propagated up the tree. resource.deregister_will_assign_resource_callback(self._call_will_assign_resource_callbacks) @@ -703,8 +730,8 @@ def get_resource(self, name: str) -> Resource: ValueError: If no resource with the given name exists. """ - resource = self._subtree_resources.get(name) - if resource is None: + resource = self._resources().get(name) + if resource is None or not (resource is self or resource.is_in_subtree_of(self)): raise ResourceNotFoundError(f"Resource with name '{name}' does not exist.") return resource @@ -717,7 +744,8 @@ def has_resource(self, name: str) -> bool: Returns: True when a resource with that name is in this subtree. """ - return name in self._subtree_resources + resource = self._resources().get(name) + return resource is not None and (resource is self or resource.is_in_subtree_of(self)) def find_resources( self, From 6c3c04f09a30fbce0f460a4b2e85e1d31080d305 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Tue, 1 Sep 2026 10:15:48 +0100 Subject: [PATCH 8/9] `Resource`: say where a resource is when it is out of scope Looking a name up now has two steps: find it in the root's map, then check it sits inside the asking resource's subtree. A name that exists in the tree but fails the second step is a different situation from one that is not there at all, and the first step already knows which. Asking a carrier for a plate on the carrier beside it said the plate did not exist. It now says where it is. Co-authored-by: Claude Opus 5 (1M context) --- pylabrobot/resources/resource.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pylabrobot/resources/resource.py b/pylabrobot/resources/resource.py index afad63b44c9..5df051e0716 100644 --- a/pylabrobot/resources/resource.py +++ b/pylabrobot/resources/resource.py @@ -731,8 +731,15 @@ def get_resource(self, name: str) -> Resource: """ resource = self._resources().get(name) - if resource is None or not (resource is self or resource.is_in_subtree_of(self)): + if resource is None: raise ResourceNotFoundError(f"Resource with name '{name}' does not exist.") + if not (resource is self or resource.is_in_subtree_of(self)): + where = ( + f"assigned to '{resource.parent.name}'" if resource.parent else "the root of this tree" + ) + raise ResourceNotFoundError( + f"'{name}' is not at or beneath '{self.name}'. It is in the same tree, {where}." + ) return resource def has_resource(self, name: str) -> bool: From 58a7a2c4b1e2d7582a5808f1fa973f65df7977b9 Mon Sep 17 00:00:00 2001 From: Camillo Moschner Date: Tue, 1 Sep 2026 11:31:17 +0100 Subject: [PATCH 9/9] `Deck`: drop the class docstring's account of the removed dictionary It described a dictionary of every resource on the deck, kept in step on assign and unassign, for O(1) collision checks and lookup by name. That dictionary and the methods around it are gone, so only the first line is still true. Co-authored-by: Claude Opus 5 (1M context) --- pylabrobot/resources/deck.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/pylabrobot/resources/deck.py b/pylabrobot/resources/deck.py index 10dbefbae79..4f179ca380d 100644 --- a/pylabrobot/resources/deck.py +++ b/pylabrobot/resources/deck.py @@ -10,13 +10,7 @@ class Deck(Resource): - """Base class for liquid handler decks. - - This class maintains a dictionary of all resources on the deck. The dictionary is keyed by the - resource name and is updated when resources are assigned and unassigned from the deck. The point - of this dictionary is to allow O(1) naming collision checks as well as the quick lookup of - resources by name. - """ + """Base class for liquid handler decks.""" def __init__( self,