Scaling: Resource: remember which names a tree holds instead of re-reading it - #1222

Merged
BioCam merged 14 commits into
mainfrom
v1-resource-name-index
Sep 2, 2026
Merged

Scaling: Resource: remember which names a tree holds instead of re-reading it#1222
BioCam merged 14 commits into
mainfrom
v1-resource-name-index

Conversation

@BioCam

Copy link
Copy Markdown
Collaborator

Problem

_check_naming_conflicts enforces tree-wide name uniqueness by recursing over the whole tree, and assign_child_resource calls it on get_root() for every resource assigned. An assignment costs the size of everything already in the tree rather than the size of what is arriving, so building n resources costs n² - measured between n^1.9 and n^2.0 - and every later assignment still walks all of it.

A loaded STARlet deck is around 2 000 resources. One plate move on it, an unassign and a re-assign, already costs 47 ms. Put several instruments in one tree and the cost grows with all of them, not with the plate:

resourcesinstrumentsbeforeafter
2 001147.0 ms0.0 ms
4 002293.1 ms0.0 ms
8 0044186.7 ms0.0 ms
16 0088379.3 ms0.0 ms

Changes

  • Resource._name_index holds the names in a tree, kept only by the root, built the first time _names_in_tree asks for it.
  • _subtree_names returns the names at or beneath a resource; assign_child_resource unions the arriving ones in, unassign_child_resource subtracts them.
  • _check_naming_conflicts walks the arriving subtree against that set instead of recursing over both trees. It reads the index off get_root(), where it was already called from, so it still covers the whole tree.

Behaviour: unchanged, and an assignment now costs the size of what is arriving. Carrying the index forward is safe because a name cannot change while a resource is assigned - the setter refuses - and a tree only changes shape in the two methods that maintain it.

Scope: cheaper, not narrower. The check still runs before the branch that detaches an already-attached resource, so a move within one tree is still refused and callers still unassign first. Excluding the arriving resource's own names would fix that, and is a follow-up since it changes behaviour the tests pin.

Tests

TestNameIndex (a duplicate name, one buried in the arriving subtree, unassigning freeing a name, a subtree carrying its names into whatever tree takes it, and the order a move has to happen in). Checked separately against the recursion it replaces over 4 000 generated tree shapes, and the index against a fresh walk after each step of 300 assign and unassign sequences. ruff format, ruff check --select I, ruff check and mypy pylabrobot --check-untyped-defs are clean; the full suite passes (2 450 passed, 2 skipped, 206 subtests).

`_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) <noreply@anthropic.com>
@BioCam
BioCam marked this pull request as ready for review August 27, 2026 15:38
@rickwierenga

Copy link
Copy Markdown
Member

let's remove the stuff from the Deck class in this case

Comment threadpylabrobot/resources/resource.py Outdated
Comment on lines +183 to +187
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo _name_index should be called _subtree_names and _subtree_names should be named _get_subtree_names or something

Comment threadpylabrobot/resources/resource.py Outdated
@rickwierenga

Copy link
Copy Markdown
Member

it would be clean to use the will/did assign child resource

  • will: check name as it currently does
  • did: update cache (so no _names_in_tree cache checking needed)

BioCamand others added 7 commits August 28, 2026 12:25
…n `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) <noreply@anthropic.com>
…hem 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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
Restores `Resource`, `Deck` and their tests to d884b2f, the state under
review. The reverted commits changed how the index is maintained, which is
the open question in review and not settled yet.
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

let's remove the stuff from the Deck class in this case

@rickwierenga - Done.
_resources, _register_resource, _deregister_resource, get_resource, has_resource and the _check_naming_conflicts override are all removed, so Deck no longer keeps its own copy of the tree.

get_all_resources is public and has callers, so I kept it and only changed the body:
it was reading the private dict, and now delegates to get_all_children(). Happy to deprecate it separately if you would rather it went too.

One behaviour change I think is worth flagging:
the old override compared only resource.name, so assigning a carrier that already held a resource named like one on the deck was accepted, and put a duplicate name in the tree.
The Resource version checks the whole arriving subtree, so that case now raises.

That behaviour is what prompted me to generate #1228 .
A name is meant to be an identifier: unique across the tree, and fixed for the life of the resource.
Neither was true.
It could be duplicated on assignment, as above, and it could still be reassigned after the resource existed, which left anything already named after it out of step.
#1228 makes name immutable after instantiation; this check closes the other half.

BioCamand others added 5 commits August 31, 2026 22:45
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

I have made a couple of changes to build a stronger root-based index ledger that fixes a naming bug and improves the performance (needed for scaling of the PLR resource model, including the upcoming change of Tip to be a resource).
So here is a (hopefully more) concise summary:

What main does today

The naming check runs on self.get_root(), so which implementation you get depends on what the root is -> i.e is inconsistent.
Deck overrides it with a single dict lookup, docstringed "overwrite for speed". Everything else uses Resource's recursive version, whose cost grows with the tree.

So main is fast in one shape: a Deck on the path, asked about itself.
That is the liquid handler shape we are removing in v1, and it is the shape Deck was built for.
Outside it, the fast path was never available.

The bug that inconsistency causes

Deck's version compares only the arriving resource's own name, so a clash buried inside what is arriving is never looked at:

deck=STARLetDeck()
car_1=TIP_CAR_480_A00(name="car_1")
car_1[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01")
deck.assign_child_resource(car_1, rails=1)
car_2=TIP_CAR_480_A00(name="car_2")
car_2[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01") # same namedeck.assign_child_resource(car_2, rails=10) # accepted on main

Two resources called tips_01 in one tree, and the consequences are silent:

  • deck.get_resource("tips_01") returns the one on car_2. The rack on car_1 is unreachable by name at all.
  • serialize_all_state() produces 97 keys for the 194 resources across the two racks, so one rack's state overwrites the other's.

The equivalent tree under a non-Deck root is refused, because Resource's version checks the whole arriving subtree.
This PR refuses it everywhere.

What this PR changes

Deck stops being a privileged resource. Its ledger and its check become properties of whatever is currently the root.
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, so a detached subtree heads a tree of its own again.
Everything that is not a root holds None.

A lookup is then two steps: hash the name in the root's ledger, then walk up the parent pointers to check the hit is inside the subtree that was asked.
Both are bounded by depth rather than size, so a resource still finds only what is at or beneath it, at the same cost wherever it is asked from.
And because the first step knows the name exists before the second rejects it, asking the wrong resource now says where it actually is.

Performance

The naming check is quadratic in the size of whatever you are building, because every child assigned re-walks everything already there.
That is invisible at 96 wells and not at 1536.
Building one DeepWell_Greiner_1536_Well from the standard library:

mainthis PR
_check_naming_conflicts calls1,180,4161,536
time per plate~150 ms~13 ms
96-well plate, for comparison1.48 ms1.20 ms

1,180,416 is exactly 1536 * 1537 / 2. The timings vary about 20% run to run, the call count does not.

A plate builds its own wells before it has ever met a deck, so the Deck ledger never covered the part that costs.

Where a Deck does apply, main is fine and in one case faster:

treeoperationmainthis PR
Deck at the root, 2,612 resourcesask the deck0.09 us0.23 us
Deck at the root, 2,612 resourcesask a carrier29.64 us0.23 us
facility, a Deck per device, 98,221build it1.01 s0.83 s

Build that same facility with no decks in it and main takes 499 s against 0.50 s here.

The ledger costs one map: 3.84 MB for 98,221 resources, about 3% of the tree, holding references to resources that already exist.

Why now

This matters for v1 because there is no liquid handler concept any more.
A workcell or facility don't need to contain a deck at all, so "there is a Deck at the root" is no longer a safe assumption to hang the resource model's performance on.
Tip becoming a resource pushes the same way: it roughly doubles every tip rack, from 97 resources to 193, and tip racks are built standalone, which is exactly where the fast path never applied.

@BioCam

BioCam commented Sep 1, 2026

Copy link
Copy Markdown
CollaboratorAuthor

I'm a very visual person, so here a visual aid to root updating subtree ledger dict across resource tree mergers and separation:

FacilityAssignmentScene.mp4

@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

lookup before and after - from different levels:

GetResourceScene.mp4

Comment threadpylabrobot/resources/resource.py

@rickwierengarickwierenga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it

@rickwierenga

Copy link
Copy Markdown
Member

please

@BioCam
BioCam merged commit 543362f into mainSep 2, 2026
21 checks passed
@rickwierenga
rickwierenga deleted the v1-resource-name-index branch September 2, 2026 14:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@BioCam@rickwierenga
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Scaling: Resource: remember which names a tree holds instead of re-reading it - #1222

Merged
BioCam merged 14 commits into
mainfrom
v1-resource-name-index
Sep 2, 2026
Merged

Scaling: Resource: remember which names a tree holds instead of re-reading it#1222
BioCam merged 14 commits into
mainfrom
v1-resource-name-index

Conversation

@BioCam

Copy link
Copy Markdown
Collaborator

Problem

_check_naming_conflicts enforces tree-wide name uniqueness by recursing over the whole tree, and assign_child_resource calls it on get_root() for every resource assigned. An assignment costs the size of everything already in the tree rather than the size of what is arriving, so building n resources costs n² - measured between n^1.9 and n^2.0 - and every later assignment still walks all of it.

A loaded STARlet deck is around 2 000 resources. One plate move on it, an unassign and a re-assign, already costs 47 ms. Put several instruments in one tree and the cost grows with all of them, not with the plate:

resourcesinstrumentsbeforeafter
2 001147.0 ms0.0 ms
4 002293.1 ms0.0 ms
8 0044186.7 ms0.0 ms
16 0088379.3 ms0.0 ms

Changes

  • Resource._name_index holds the names in a tree, kept only by the root, built the first time _names_in_tree asks for it.
  • _subtree_names returns the names at or beneath a resource; assign_child_resource unions the arriving ones in, unassign_child_resource subtracts them.
  • _check_naming_conflicts walks the arriving subtree against that set instead of recursing over both trees. It reads the index off get_root(), where it was already called from, so it still covers the whole tree.

Behaviour: unchanged, and an assignment now costs the size of what is arriving. Carrying the index forward is safe because a name cannot change while a resource is assigned - the setter refuses - and a tree only changes shape in the two methods that maintain it.

Scope: cheaper, not narrower. The check still runs before the branch that detaches an already-attached resource, so a move within one tree is still refused and callers still unassign first. Excluding the arriving resource's own names would fix that, and is a follow-up since it changes behaviour the tests pin.

Tests

TestNameIndex (a duplicate name, one buried in the arriving subtree, unassigning freeing a name, a subtree carrying its names into whatever tree takes it, and the order a move has to happen in). Checked separately against the recursion it replaces over 4 000 generated tree shapes, and the index against a fresh walk after each step of 300 assign and unassign sequences. ruff format, ruff check --select I, ruff check and mypy pylabrobot --check-untyped-defs are clean; the full suite passes (2 450 passed, 2 skipped, 206 subtests).

`_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) <noreply@anthropic.com>
@BioCam
BioCam marked this pull request as ready for review August 27, 2026 15:38
@rickwierenga

Copy link
Copy Markdown
Member

let's remove the stuff from the Deck class in this case

Comment threadpylabrobot/resources/resource.py Outdated
Comment on lines +183 to +187
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo _name_index should be called _subtree_names and _subtree_names should be named _get_subtree_names or something

Comment threadpylabrobot/resources/resource.py Outdated
@rickwierenga

Copy link
Copy Markdown
Member

it would be clean to use the will/did assign child resource

  • will: check name as it currently does
  • did: update cache (so no _names_in_tree cache checking needed)

BioCamand others added 7 commits August 28, 2026 12:25
…n `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) <noreply@anthropic.com>
…hem 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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
Restores `Resource`, `Deck` and their tests to d884b2f, the state under
review. The reverted commits changed how the index is maintained, which is
the open question in review and not settled yet.
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

let's remove the stuff from the Deck class in this case

@rickwierenga - Done.
_resources, _register_resource, _deregister_resource, get_resource, has_resource and the _check_naming_conflicts override are all removed, so Deck no longer keeps its own copy of the tree.

get_all_resources is public and has callers, so I kept it and only changed the body:
it was reading the private dict, and now delegates to get_all_children(). Happy to deprecate it separately if you would rather it went too.

One behaviour change I think is worth flagging:
the old override compared only resource.name, so assigning a carrier that already held a resource named like one on the deck was accepted, and put a duplicate name in the tree.
The Resource version checks the whole arriving subtree, so that case now raises.

That behaviour is what prompted me to generate #1228 .
A name is meant to be an identifier: unique across the tree, and fixed for the life of the resource.
Neither was true.
It could be duplicated on assignment, as above, and it could still be reassigned after the resource existed, which left anything already named after it out of step.
#1228 makes name immutable after instantiation; this check closes the other half.

BioCamand others added 5 commits August 31, 2026 22:45
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

I have made a couple of changes to build a stronger root-based index ledger that fixes a naming bug and improves the performance (needed for scaling of the PLR resource model, including the upcoming change of Tip to be a resource).
So here is a (hopefully more) concise summary:

What main does today

The naming check runs on self.get_root(), so which implementation you get depends on what the root is -> i.e is inconsistent.
Deck overrides it with a single dict lookup, docstringed "overwrite for speed". Everything else uses Resource's recursive version, whose cost grows with the tree.

So main is fast in one shape: a Deck on the path, asked about itself.
That is the liquid handler shape we are removing in v1, and it is the shape Deck was built for.
Outside it, the fast path was never available.

The bug that inconsistency causes

Deck's version compares only the arriving resource's own name, so a clash buried inside what is arriving is never looked at:

deck=STARLetDeck()
car_1=TIP_CAR_480_A00(name="car_1")
car_1[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01")
deck.assign_child_resource(car_1, rails=1)
car_2=TIP_CAR_480_A00(name="car_2")
car_2[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01") # same namedeck.assign_child_resource(car_2, rails=10) # accepted on main

Two resources called tips_01 in one tree, and the consequences are silent:

  • deck.get_resource("tips_01") returns the one on car_2. The rack on car_1 is unreachable by name at all.
  • serialize_all_state() produces 97 keys for the 194 resources across the two racks, so one rack's state overwrites the other's.

The equivalent tree under a non-Deck root is refused, because Resource's version checks the whole arriving subtree.
This PR refuses it everywhere.

What this PR changes

Deck stops being a privileged resource. Its ledger and its check become properties of whatever is currently the root.
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, so a detached subtree heads a tree of its own again.
Everything that is not a root holds None.

A lookup is then two steps: hash the name in the root's ledger, then walk up the parent pointers to check the hit is inside the subtree that was asked.
Both are bounded by depth rather than size, so a resource still finds only what is at or beneath it, at the same cost wherever it is asked from.
And because the first step knows the name exists before the second rejects it, asking the wrong resource now says where it actually is.

Performance

The naming check is quadratic in the size of whatever you are building, because every child assigned re-walks everything already there.
That is invisible at 96 wells and not at 1536.
Building one DeepWell_Greiner_1536_Well from the standard library:

mainthis PR
_check_naming_conflicts calls1,180,4161,536
time per plate~150 ms~13 ms
96-well plate, for comparison1.48 ms1.20 ms

1,180,416 is exactly 1536 * 1537 / 2. The timings vary about 20% run to run, the call count does not.

A plate builds its own wells before it has ever met a deck, so the Deck ledger never covered the part that costs.

Where a Deck does apply, main is fine and in one case faster:

treeoperationmainthis PR
Deck at the root, 2,612 resourcesask the deck0.09 us0.23 us
Deck at the root, 2,612 resourcesask a carrier29.64 us0.23 us
facility, a Deck per device, 98,221build it1.01 s0.83 s

Build that same facility with no decks in it and main takes 499 s against 0.50 s here.

The ledger costs one map: 3.84 MB for 98,221 resources, about 3% of the tree, holding references to resources that already exist.

Why now

This matters for v1 because there is no liquid handler concept any more.
A workcell or facility don't need to contain a deck at all, so "there is a Deck at the root" is no longer a safe assumption to hang the resource model's performance on.
Tip becoming a resource pushes the same way: it roughly doubles every tip rack, from 97 resources to 193, and tip racks are built standalone, which is exactly where the fast path never applied.

@BioCam

BioCam commented Sep 1, 2026

Copy link
Copy Markdown
CollaboratorAuthor

I'm a very visual person, so here a visual aid to root updating subtree ledger dict across resource tree mergers and separation:

FacilityAssignmentScene.mp4

@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

lookup before and after - from different levels:

GetResourceScene.mp4

Comment threadpylabrobot/resources/resource.py

@rickwierengarickwierenga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it

@rickwierenga

Copy link
Copy Markdown
Member

please

@BioCam
BioCam merged commit 543362f into mainSep 2, 2026
21 checks passed
@rickwierenga
rickwierenga deleted the v1-resource-name-index branch September 2, 2026 14:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@BioCam@rickwierenga
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Scaling: Resource: remember which names a tree holds instead of re-reading it - #1222

Merged
BioCam merged 14 commits into
mainfrom
v1-resource-name-index
Sep 2, 2026
Merged

Scaling: Resource: remember which names a tree holds instead of re-reading it#1222
BioCam merged 14 commits into
mainfrom
v1-resource-name-index

Conversation

@BioCam

Copy link
Copy Markdown
Collaborator

Problem

_check_naming_conflicts enforces tree-wide name uniqueness by recursing over the whole tree, and assign_child_resource calls it on get_root() for every resource assigned. An assignment costs the size of everything already in the tree rather than the size of what is arriving, so building n resources costs n² - measured between n^1.9 and n^2.0 - and every later assignment still walks all of it.

A loaded STARlet deck is around 2 000 resources. One plate move on it, an unassign and a re-assign, already costs 47 ms. Put several instruments in one tree and the cost grows with all of them, not with the plate:

resourcesinstrumentsbeforeafter
2 001147.0 ms0.0 ms
4 002293.1 ms0.0 ms
8 0044186.7 ms0.0 ms
16 0088379.3 ms0.0 ms

Changes

  • Resource._name_index holds the names in a tree, kept only by the root, built the first time _names_in_tree asks for it.
  • _subtree_names returns the names at or beneath a resource; assign_child_resource unions the arriving ones in, unassign_child_resource subtracts them.
  • _check_naming_conflicts walks the arriving subtree against that set instead of recursing over both trees. It reads the index off get_root(), where it was already called from, so it still covers the whole tree.

Behaviour: unchanged, and an assignment now costs the size of what is arriving. Carrying the index forward is safe because a name cannot change while a resource is assigned - the setter refuses - and a tree only changes shape in the two methods that maintain it.

Scope: cheaper, not narrower. The check still runs before the branch that detaches an already-attached resource, so a move within one tree is still refused and callers still unassign first. Excluding the arriving resource's own names would fix that, and is a follow-up since it changes behaviour the tests pin.

Tests

TestNameIndex (a duplicate name, one buried in the arriving subtree, unassigning freeing a name, a subtree carrying its names into whatever tree takes it, and the order a move has to happen in). Checked separately against the recursion it replaces over 4 000 generated tree shapes, and the index against a fresh walk after each step of 300 assign and unassign sequences. ruff format, ruff check --select I, ruff check and mypy pylabrobot --check-untyped-defs are clean; the full suite passes (2 450 passed, 2 skipped, 206 subtests).

`_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) <noreply@anthropic.com>
@BioCam
BioCam marked this pull request as ready for review August 27, 2026 15:38
@rickwierenga

Copy link
Copy Markdown
Member

let's remove the stuff from the Deck class in this case

Comment threadpylabrobot/resources/resource.py Outdated
Comment on lines +183 to +187
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo _name_index should be called _subtree_names and _subtree_names should be named _get_subtree_names or something

Comment threadpylabrobot/resources/resource.py Outdated
@rickwierenga

Copy link
Copy Markdown
Member

it would be clean to use the will/did assign child resource

  • will: check name as it currently does
  • did: update cache (so no _names_in_tree cache checking needed)

BioCamand others added 7 commits August 28, 2026 12:25
…n `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) <noreply@anthropic.com>
…hem 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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
Restores `Resource`, `Deck` and their tests to d884b2f, the state under
review. The reverted commits changed how the index is maintained, which is
the open question in review and not settled yet.
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

let's remove the stuff from the Deck class in this case

@rickwierenga - Done.
_resources, _register_resource, _deregister_resource, get_resource, has_resource and the _check_naming_conflicts override are all removed, so Deck no longer keeps its own copy of the tree.

get_all_resources is public and has callers, so I kept it and only changed the body:
it was reading the private dict, and now delegates to get_all_children(). Happy to deprecate it separately if you would rather it went too.

One behaviour change I think is worth flagging:
the old override compared only resource.name, so assigning a carrier that already held a resource named like one on the deck was accepted, and put a duplicate name in the tree.
The Resource version checks the whole arriving subtree, so that case now raises.

That behaviour is what prompted me to generate #1228 .
A name is meant to be an identifier: unique across the tree, and fixed for the life of the resource.
Neither was true.
It could be duplicated on assignment, as above, and it could still be reassigned after the resource existed, which left anything already named after it out of step.
#1228 makes name immutable after instantiation; this check closes the other half.

BioCamand others added 5 commits August 31, 2026 22:45
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

I have made a couple of changes to build a stronger root-based index ledger that fixes a naming bug and improves the performance (needed for scaling of the PLR resource model, including the upcoming change of Tip to be a resource).
So here is a (hopefully more) concise summary:

What main does today

The naming check runs on self.get_root(), so which implementation you get depends on what the root is -> i.e is inconsistent.
Deck overrides it with a single dict lookup, docstringed "overwrite for speed". Everything else uses Resource's recursive version, whose cost grows with the tree.

So main is fast in one shape: a Deck on the path, asked about itself.
That is the liquid handler shape we are removing in v1, and it is the shape Deck was built for.
Outside it, the fast path was never available.

The bug that inconsistency causes

Deck's version compares only the arriving resource's own name, so a clash buried inside what is arriving is never looked at:

deck=STARLetDeck()
car_1=TIP_CAR_480_A00(name="car_1")
car_1[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01")
deck.assign_child_resource(car_1, rails=1)
car_2=TIP_CAR_480_A00(name="car_2")
car_2[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01") # same namedeck.assign_child_resource(car_2, rails=10) # accepted on main

Two resources called tips_01 in one tree, and the consequences are silent:

  • deck.get_resource("tips_01") returns the one on car_2. The rack on car_1 is unreachable by name at all.
  • serialize_all_state() produces 97 keys for the 194 resources across the two racks, so one rack's state overwrites the other's.

The equivalent tree under a non-Deck root is refused, because Resource's version checks the whole arriving subtree.
This PR refuses it everywhere.

What this PR changes

Deck stops being a privileged resource. Its ledger and its check become properties of whatever is currently the root.
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, so a detached subtree heads a tree of its own again.
Everything that is not a root holds None.

A lookup is then two steps: hash the name in the root's ledger, then walk up the parent pointers to check the hit is inside the subtree that was asked.
Both are bounded by depth rather than size, so a resource still finds only what is at or beneath it, at the same cost wherever it is asked from.
And because the first step knows the name exists before the second rejects it, asking the wrong resource now says where it actually is.

Performance

The naming check is quadratic in the size of whatever you are building, because every child assigned re-walks everything already there.
That is invisible at 96 wells and not at 1536.
Building one DeepWell_Greiner_1536_Well from the standard library:

mainthis PR
_check_naming_conflicts calls1,180,4161,536
time per plate~150 ms~13 ms
96-well plate, for comparison1.48 ms1.20 ms

1,180,416 is exactly 1536 * 1537 / 2. The timings vary about 20% run to run, the call count does not.

A plate builds its own wells before it has ever met a deck, so the Deck ledger never covered the part that costs.

Where a Deck does apply, main is fine and in one case faster:

treeoperationmainthis PR
Deck at the root, 2,612 resourcesask the deck0.09 us0.23 us
Deck at the root, 2,612 resourcesask a carrier29.64 us0.23 us
facility, a Deck per device, 98,221build it1.01 s0.83 s

Build that same facility with no decks in it and main takes 499 s against 0.50 s here.

The ledger costs one map: 3.84 MB for 98,221 resources, about 3% of the tree, holding references to resources that already exist.

Why now

This matters for v1 because there is no liquid handler concept any more.
A workcell or facility don't need to contain a deck at all, so "there is a Deck at the root" is no longer a safe assumption to hang the resource model's performance on.
Tip becoming a resource pushes the same way: it roughly doubles every tip rack, from 97 resources to 193, and tip racks are built standalone, which is exactly where the fast path never applied.

@BioCam

BioCam commented Sep 1, 2026

Copy link
Copy Markdown
CollaboratorAuthor

I'm a very visual person, so here a visual aid to root updating subtree ledger dict across resource tree mergers and separation:

FacilityAssignmentScene.mp4

@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

lookup before and after - from different levels:

GetResourceScene.mp4

Comment threadpylabrobot/resources/resource.py

@rickwierengarickwierenga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it

@rickwierenga

Copy link
Copy Markdown
Member

please

@BioCam
BioCam merged commit 543362f into mainSep 2, 2026
21 checks passed
@rickwierenga
rickwierenga deleted the v1-resource-name-index branch September 2, 2026 14:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@BioCam@rickwierenga
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Scaling: Resource: remember which names a tree holds instead of re-reading it - #1222

Merged
BioCam merged 14 commits into
mainfrom
v1-resource-name-index
Sep 2, 2026
Merged

Scaling: Resource: remember which names a tree holds instead of re-reading it#1222
BioCam merged 14 commits into
mainfrom
v1-resource-name-index

Conversation

@BioCam

Copy link
Copy Markdown
Collaborator

Problem

_check_naming_conflicts enforces tree-wide name uniqueness by recursing over the whole tree, and assign_child_resource calls it on get_root() for every resource assigned. An assignment costs the size of everything already in the tree rather than the size of what is arriving, so building n resources costs n² - measured between n^1.9 and n^2.0 - and every later assignment still walks all of it.

A loaded STARlet deck is around 2 000 resources. One plate move on it, an unassign and a re-assign, already costs 47 ms. Put several instruments in one tree and the cost grows with all of them, not with the plate:

resourcesinstrumentsbeforeafter
2 001147.0 ms0.0 ms
4 002293.1 ms0.0 ms
8 0044186.7 ms0.0 ms
16 0088379.3 ms0.0 ms

Changes

  • Resource._name_index holds the names in a tree, kept only by the root, built the first time _names_in_tree asks for it.
  • _subtree_names returns the names at or beneath a resource; assign_child_resource unions the arriving ones in, unassign_child_resource subtracts them.
  • _check_naming_conflicts walks the arriving subtree against that set instead of recursing over both trees. It reads the index off get_root(), where it was already called from, so it still covers the whole tree.

Behaviour: unchanged, and an assignment now costs the size of what is arriving. Carrying the index forward is safe because a name cannot change while a resource is assigned - the setter refuses - and a tree only changes shape in the two methods that maintain it.

Scope: cheaper, not narrower. The check still runs before the branch that detaches an already-attached resource, so a move within one tree is still refused and callers still unassign first. Excluding the arriving resource's own names would fix that, and is a follow-up since it changes behaviour the tests pin.

Tests

TestNameIndex (a duplicate name, one buried in the arriving subtree, unassigning freeing a name, a subtree carrying its names into whatever tree takes it, and the order a move has to happen in). Checked separately against the recursion it replaces over 4 000 generated tree shapes, and the index against a fresh walk after each step of 300 assign and unassign sequences. ruff format, ruff check --select I, ruff check and mypy pylabrobot --check-untyped-defs are clean; the full suite passes (2 450 passed, 2 skipped, 206 subtests).

`_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) <noreply@anthropic.com>
@BioCam
BioCam marked this pull request as ready for review August 27, 2026 15:38
@rickwierenga

Copy link
Copy Markdown
Member

let's remove the stuff from the Deck class in this case

Comment threadpylabrobot/resources/resource.py Outdated
Comment on lines +183 to +187
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo _name_index should be called _subtree_names and _subtree_names should be named _get_subtree_names or something

Comment threadpylabrobot/resources/resource.py Outdated
@rickwierenga

Copy link
Copy Markdown
Member

it would be clean to use the will/did assign child resource

  • will: check name as it currently does
  • did: update cache (so no _names_in_tree cache checking needed)

BioCamand others added 7 commits August 28, 2026 12:25
…n `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) <noreply@anthropic.com>
…hem 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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
Restores `Resource`, `Deck` and their tests to d884b2f, the state under
review. The reverted commits changed how the index is maintained, which is
the open question in review and not settled yet.
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

let's remove the stuff from the Deck class in this case

@rickwierenga - Done.
_resources, _register_resource, _deregister_resource, get_resource, has_resource and the _check_naming_conflicts override are all removed, so Deck no longer keeps its own copy of the tree.

get_all_resources is public and has callers, so I kept it and only changed the body:
it was reading the private dict, and now delegates to get_all_children(). Happy to deprecate it separately if you would rather it went too.

One behaviour change I think is worth flagging:
the old override compared only resource.name, so assigning a carrier that already held a resource named like one on the deck was accepted, and put a duplicate name in the tree.
The Resource version checks the whole arriving subtree, so that case now raises.

That behaviour is what prompted me to generate #1228 .
A name is meant to be an identifier: unique across the tree, and fixed for the life of the resource.
Neither was true.
It could be duplicated on assignment, as above, and it could still be reassigned after the resource existed, which left anything already named after it out of step.
#1228 makes name immutable after instantiation; this check closes the other half.

BioCamand others added 5 commits August 31, 2026 22:45
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

I have made a couple of changes to build a stronger root-based index ledger that fixes a naming bug and improves the performance (needed for scaling of the PLR resource model, including the upcoming change of Tip to be a resource).
So here is a (hopefully more) concise summary:

What main does today

The naming check runs on self.get_root(), so which implementation you get depends on what the root is -> i.e is inconsistent.
Deck overrides it with a single dict lookup, docstringed "overwrite for speed". Everything else uses Resource's recursive version, whose cost grows with the tree.

So main is fast in one shape: a Deck on the path, asked about itself.
That is the liquid handler shape we are removing in v1, and it is the shape Deck was built for.
Outside it, the fast path was never available.

The bug that inconsistency causes

Deck's version compares only the arriving resource's own name, so a clash buried inside what is arriving is never looked at:

deck=STARLetDeck()
car_1=TIP_CAR_480_A00(name="car_1")
car_1[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01")
deck.assign_child_resource(car_1, rails=1)
car_2=TIP_CAR_480_A00(name="car_2")
car_2[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01") # same namedeck.assign_child_resource(car_2, rails=10) # accepted on main

Two resources called tips_01 in one tree, and the consequences are silent:

  • deck.get_resource("tips_01") returns the one on car_2. The rack on car_1 is unreachable by name at all.
  • serialize_all_state() produces 97 keys for the 194 resources across the two racks, so one rack's state overwrites the other's.

The equivalent tree under a non-Deck root is refused, because Resource's version checks the whole arriving subtree.
This PR refuses it everywhere.

What this PR changes

Deck stops being a privileged resource. Its ledger and its check become properties of whatever is currently the root.
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, so a detached subtree heads a tree of its own again.
Everything that is not a root holds None.

A lookup is then two steps: hash the name in the root's ledger, then walk up the parent pointers to check the hit is inside the subtree that was asked.
Both are bounded by depth rather than size, so a resource still finds only what is at or beneath it, at the same cost wherever it is asked from.
And because the first step knows the name exists before the second rejects it, asking the wrong resource now says where it actually is.

Performance

The naming check is quadratic in the size of whatever you are building, because every child assigned re-walks everything already there.
That is invisible at 96 wells and not at 1536.
Building one DeepWell_Greiner_1536_Well from the standard library:

mainthis PR
_check_naming_conflicts calls1,180,4161,536
time per plate~150 ms~13 ms
96-well plate, for comparison1.48 ms1.20 ms

1,180,416 is exactly 1536 * 1537 / 2. The timings vary about 20% run to run, the call count does not.

A plate builds its own wells before it has ever met a deck, so the Deck ledger never covered the part that costs.

Where a Deck does apply, main is fine and in one case faster:

treeoperationmainthis PR
Deck at the root, 2,612 resourcesask the deck0.09 us0.23 us
Deck at the root, 2,612 resourcesask a carrier29.64 us0.23 us
facility, a Deck per device, 98,221build it1.01 s0.83 s

Build that same facility with no decks in it and main takes 499 s against 0.50 s here.

The ledger costs one map: 3.84 MB for 98,221 resources, about 3% of the tree, holding references to resources that already exist.

Why now

This matters for v1 because there is no liquid handler concept any more.
A workcell or facility don't need to contain a deck at all, so "there is a Deck at the root" is no longer a safe assumption to hang the resource model's performance on.
Tip becoming a resource pushes the same way: it roughly doubles every tip rack, from 97 resources to 193, and tip racks are built standalone, which is exactly where the fast path never applied.

@BioCam

BioCam commented Sep 1, 2026

Copy link
Copy Markdown
CollaboratorAuthor

I'm a very visual person, so here a visual aid to root updating subtree ledger dict across resource tree mergers and separation:

FacilityAssignmentScene.mp4

@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

lookup before and after - from different levels:

GetResourceScene.mp4

Comment threadpylabrobot/resources/resource.py

@rickwierengarickwierenga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it

@rickwierenga

Copy link
Copy Markdown
Member

please

@BioCam
BioCam merged commit 543362f into mainSep 2, 2026
21 checks passed
@rickwierenga
rickwierenga deleted the v1-resource-name-index branch September 2, 2026 14:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@BioCam@rickwierenga
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Scaling: Resource: remember which names a tree holds instead of re-reading it - #1222

Merged
BioCam merged 14 commits into
mainfrom
v1-resource-name-index
Sep 2, 2026
Merged

Scaling: Resource: remember which names a tree holds instead of re-reading it#1222
BioCam merged 14 commits into
mainfrom
v1-resource-name-index

Conversation

@BioCam

Copy link
Copy Markdown
Collaborator

Problem

_check_naming_conflicts enforces tree-wide name uniqueness by recursing over the whole tree, and assign_child_resource calls it on get_root() for every resource assigned. An assignment costs the size of everything already in the tree rather than the size of what is arriving, so building n resources costs n² - measured between n^1.9 and n^2.0 - and every later assignment still walks all of it.

A loaded STARlet deck is around 2 000 resources. One plate move on it, an unassign and a re-assign, already costs 47 ms. Put several instruments in one tree and the cost grows with all of them, not with the plate:

resourcesinstrumentsbeforeafter
2 001147.0 ms0.0 ms
4 002293.1 ms0.0 ms
8 0044186.7 ms0.0 ms
16 0088379.3 ms0.0 ms

Changes

  • Resource._name_index holds the names in a tree, kept only by the root, built the first time _names_in_tree asks for it.
  • _subtree_names returns the names at or beneath a resource; assign_child_resource unions the arriving ones in, unassign_child_resource subtracts them.
  • _check_naming_conflicts walks the arriving subtree against that set instead of recursing over both trees. It reads the index off get_root(), where it was already called from, so it still covers the whole tree.

Behaviour: unchanged, and an assignment now costs the size of what is arriving. Carrying the index forward is safe because a name cannot change while a resource is assigned - the setter refuses - and a tree only changes shape in the two methods that maintain it.

Scope: cheaper, not narrower. The check still runs before the branch that detaches an already-attached resource, so a move within one tree is still refused and callers still unassign first. Excluding the arriving resource's own names would fix that, and is a follow-up since it changes behaviour the tests pin.

Tests

TestNameIndex (a duplicate name, one buried in the arriving subtree, unassigning freeing a name, a subtree carrying its names into whatever tree takes it, and the order a move has to happen in). Checked separately against the recursion it replaces over 4 000 generated tree shapes, and the index against a fresh walk after each step of 300 assign and unassign sequences. ruff format, ruff check --select I, ruff check and mypy pylabrobot --check-untyped-defs are clean; the full suite passes (2 450 passed, 2 skipped, 206 subtests).

`_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) <noreply@anthropic.com>
@BioCam
BioCam marked this pull request as ready for review August 27, 2026 15:38
@rickwierenga

Copy link
Copy Markdown
Member

let's remove the stuff from the Deck class in this case

Comment threadpylabrobot/resources/resource.py Outdated
Comment on lines +183 to +187
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo _name_index should be called _subtree_names and _subtree_names should be named _get_subtree_names or something

Comment threadpylabrobot/resources/resource.py Outdated
@rickwierenga

Copy link
Copy Markdown
Member

it would be clean to use the will/did assign child resource

  • will: check name as it currently does
  • did: update cache (so no _names_in_tree cache checking needed)

BioCamand others added 7 commits August 28, 2026 12:25
…n `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) <noreply@anthropic.com>
…hem 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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
Restores `Resource`, `Deck` and their tests to d884b2f, the state under
review. The reverted commits changed how the index is maintained, which is
the open question in review and not settled yet.
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

let's remove the stuff from the Deck class in this case

@rickwierenga - Done.
_resources, _register_resource, _deregister_resource, get_resource, has_resource and the _check_naming_conflicts override are all removed, so Deck no longer keeps its own copy of the tree.

get_all_resources is public and has callers, so I kept it and only changed the body:
it was reading the private dict, and now delegates to get_all_children(). Happy to deprecate it separately if you would rather it went too.

One behaviour change I think is worth flagging:
the old override compared only resource.name, so assigning a carrier that already held a resource named like one on the deck was accepted, and put a duplicate name in the tree.
The Resource version checks the whole arriving subtree, so that case now raises.

That behaviour is what prompted me to generate #1228 .
A name is meant to be an identifier: unique across the tree, and fixed for the life of the resource.
Neither was true.
It could be duplicated on assignment, as above, and it could still be reassigned after the resource existed, which left anything already named after it out of step.
#1228 makes name immutable after instantiation; this check closes the other half.

BioCamand others added 5 commits August 31, 2026 22:45
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

I have made a couple of changes to build a stronger root-based index ledger that fixes a naming bug and improves the performance (needed for scaling of the PLR resource model, including the upcoming change of Tip to be a resource).
So here is a (hopefully more) concise summary:

What main does today

The naming check runs on self.get_root(), so which implementation you get depends on what the root is -> i.e is inconsistent.
Deck overrides it with a single dict lookup, docstringed "overwrite for speed". Everything else uses Resource's recursive version, whose cost grows with the tree.

So main is fast in one shape: a Deck on the path, asked about itself.
That is the liquid handler shape we are removing in v1, and it is the shape Deck was built for.
Outside it, the fast path was never available.

The bug that inconsistency causes

Deck's version compares only the arriving resource's own name, so a clash buried inside what is arriving is never looked at:

deck=STARLetDeck()
car_1=TIP_CAR_480_A00(name="car_1")
car_1[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01")
deck.assign_child_resource(car_1, rails=1)
car_2=TIP_CAR_480_A00(name="car_2")
car_2[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01") # same namedeck.assign_child_resource(car_2, rails=10) # accepted on main

Two resources called tips_01 in one tree, and the consequences are silent:

  • deck.get_resource("tips_01") returns the one on car_2. The rack on car_1 is unreachable by name at all.
  • serialize_all_state() produces 97 keys for the 194 resources across the two racks, so one rack's state overwrites the other's.

The equivalent tree under a non-Deck root is refused, because Resource's version checks the whole arriving subtree.
This PR refuses it everywhere.

What this PR changes

Deck stops being a privileged resource. Its ledger and its check become properties of whatever is currently the root.
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, so a detached subtree heads a tree of its own again.
Everything that is not a root holds None.

A lookup is then two steps: hash the name in the root's ledger, then walk up the parent pointers to check the hit is inside the subtree that was asked.
Both are bounded by depth rather than size, so a resource still finds only what is at or beneath it, at the same cost wherever it is asked from.
And because the first step knows the name exists before the second rejects it, asking the wrong resource now says where it actually is.

Performance

The naming check is quadratic in the size of whatever you are building, because every child assigned re-walks everything already there.
That is invisible at 96 wells and not at 1536.
Building one DeepWell_Greiner_1536_Well from the standard library:

mainthis PR
_check_naming_conflicts calls1,180,4161,536
time per plate~150 ms~13 ms
96-well plate, for comparison1.48 ms1.20 ms

1,180,416 is exactly 1536 * 1537 / 2. The timings vary about 20% run to run, the call count does not.

A plate builds its own wells before it has ever met a deck, so the Deck ledger never covered the part that costs.

Where a Deck does apply, main is fine and in one case faster:

treeoperationmainthis PR
Deck at the root, 2,612 resourcesask the deck0.09 us0.23 us
Deck at the root, 2,612 resourcesask a carrier29.64 us0.23 us
facility, a Deck per device, 98,221build it1.01 s0.83 s

Build that same facility with no decks in it and main takes 499 s against 0.50 s here.

The ledger costs one map: 3.84 MB for 98,221 resources, about 3% of the tree, holding references to resources that already exist.

Why now

This matters for v1 because there is no liquid handler concept any more.
A workcell or facility don't need to contain a deck at all, so "there is a Deck at the root" is no longer a safe assumption to hang the resource model's performance on.
Tip becoming a resource pushes the same way: it roughly doubles every tip rack, from 97 resources to 193, and tip racks are built standalone, which is exactly where the fast path never applied.

@BioCam

BioCam commented Sep 1, 2026

Copy link
Copy Markdown
CollaboratorAuthor

I'm a very visual person, so here a visual aid to root updating subtree ledger dict across resource tree mergers and separation:

FacilityAssignmentScene.mp4

@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

lookup before and after - from different levels:

GetResourceScene.mp4

Comment threadpylabrobot/resources/resource.py

@rickwierengarickwierenga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it

@rickwierenga

Copy link
Copy Markdown
Member

please

@BioCam
BioCam merged commit 543362f into mainSep 2, 2026
21 checks passed
@rickwierenga
rickwierenga deleted the v1-resource-name-index branch September 2, 2026 14:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@BioCam@rickwierenga
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Scaling: Resource: remember which names a tree holds instead of re-reading it - #1222

Merged
BioCam merged 14 commits into
mainfrom
v1-resource-name-index
Sep 2, 2026
Merged

Scaling: Resource: remember which names a tree holds instead of re-reading it#1222
BioCam merged 14 commits into
mainfrom
v1-resource-name-index

Conversation

@BioCam

Copy link
Copy Markdown
Collaborator

Problem

_check_naming_conflicts enforces tree-wide name uniqueness by recursing over the whole tree, and assign_child_resource calls it on get_root() for every resource assigned. An assignment costs the size of everything already in the tree rather than the size of what is arriving, so building n resources costs n² - measured between n^1.9 and n^2.0 - and every later assignment still walks all of it.

A loaded STARlet deck is around 2 000 resources. One plate move on it, an unassign and a re-assign, already costs 47 ms. Put several instruments in one tree and the cost grows with all of them, not with the plate:

resourcesinstrumentsbeforeafter
2 001147.0 ms0.0 ms
4 002293.1 ms0.0 ms
8 0044186.7 ms0.0 ms
16 0088379.3 ms0.0 ms

Changes

  • Resource._name_index holds the names in a tree, kept only by the root, built the first time _names_in_tree asks for it.
  • _subtree_names returns the names at or beneath a resource; assign_child_resource unions the arriving ones in, unassign_child_resource subtracts them.
  • _check_naming_conflicts walks the arriving subtree against that set instead of recursing over both trees. It reads the index off get_root(), where it was already called from, so it still covers the whole tree.

Behaviour: unchanged, and an assignment now costs the size of what is arriving. Carrying the index forward is safe because a name cannot change while a resource is assigned - the setter refuses - and a tree only changes shape in the two methods that maintain it.

Scope: cheaper, not narrower. The check still runs before the branch that detaches an already-attached resource, so a move within one tree is still refused and callers still unassign first. Excluding the arriving resource's own names would fix that, and is a follow-up since it changes behaviour the tests pin.

Tests

TestNameIndex (a duplicate name, one buried in the arriving subtree, unassigning freeing a name, a subtree carrying its names into whatever tree takes it, and the order a move has to happen in). Checked separately against the recursion it replaces over 4 000 generated tree shapes, and the index against a fresh walk after each step of 300 assign and unassign sequences. ruff format, ruff check --select I, ruff check and mypy pylabrobot --check-untyped-defs are clean; the full suite passes (2 450 passed, 2 skipped, 206 subtests).

`_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) <noreply@anthropic.com>
@BioCam
BioCam marked this pull request as ready for review August 27, 2026 15:38
@rickwierenga

Copy link
Copy Markdown
Member

let's remove the stuff from the Deck class in this case

Comment threadpylabrobot/resources/resource.py Outdated
Comment on lines +183 to +187
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo _name_index should be called _subtree_names and _subtree_names should be named _get_subtree_names or something

Comment threadpylabrobot/resources/resource.py Outdated
@rickwierenga

Copy link
Copy Markdown
Member

it would be clean to use the will/did assign child resource

  • will: check name as it currently does
  • did: update cache (so no _names_in_tree cache checking needed)

BioCamand others added 7 commits August 28, 2026 12:25
…n `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) <noreply@anthropic.com>
…hem 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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
Restores `Resource`, `Deck` and their tests to d884b2f, the state under
review. The reverted commits changed how the index is maintained, which is
the open question in review and not settled yet.
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

let's remove the stuff from the Deck class in this case

@rickwierenga - Done.
_resources, _register_resource, _deregister_resource, get_resource, has_resource and the _check_naming_conflicts override are all removed, so Deck no longer keeps its own copy of the tree.

get_all_resources is public and has callers, so I kept it and only changed the body:
it was reading the private dict, and now delegates to get_all_children(). Happy to deprecate it separately if you would rather it went too.

One behaviour change I think is worth flagging:
the old override compared only resource.name, so assigning a carrier that already held a resource named like one on the deck was accepted, and put a duplicate name in the tree.
The Resource version checks the whole arriving subtree, so that case now raises.

That behaviour is what prompted me to generate #1228 .
A name is meant to be an identifier: unique across the tree, and fixed for the life of the resource.
Neither was true.
It could be duplicated on assignment, as above, and it could still be reassigned after the resource existed, which left anything already named after it out of step.
#1228 makes name immutable after instantiation; this check closes the other half.

BioCamand others added 5 commits August 31, 2026 22:45
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

I have made a couple of changes to build a stronger root-based index ledger that fixes a naming bug and improves the performance (needed for scaling of the PLR resource model, including the upcoming change of Tip to be a resource).
So here is a (hopefully more) concise summary:

What main does today

The naming check runs on self.get_root(), so which implementation you get depends on what the root is -> i.e is inconsistent.
Deck overrides it with a single dict lookup, docstringed "overwrite for speed". Everything else uses Resource's recursive version, whose cost grows with the tree.

So main is fast in one shape: a Deck on the path, asked about itself.
That is the liquid handler shape we are removing in v1, and it is the shape Deck was built for.
Outside it, the fast path was never available.

The bug that inconsistency causes

Deck's version compares only the arriving resource's own name, so a clash buried inside what is arriving is never looked at:

deck=STARLetDeck()
car_1=TIP_CAR_480_A00(name="car_1")
car_1[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01")
deck.assign_child_resource(car_1, rails=1)
car_2=TIP_CAR_480_A00(name="car_2")
car_2[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01") # same namedeck.assign_child_resource(car_2, rails=10) # accepted on main

Two resources called tips_01 in one tree, and the consequences are silent:

  • deck.get_resource("tips_01") returns the one on car_2. The rack on car_1 is unreachable by name at all.
  • serialize_all_state() produces 97 keys for the 194 resources across the two racks, so one rack's state overwrites the other's.

The equivalent tree under a non-Deck root is refused, because Resource's version checks the whole arriving subtree.
This PR refuses it everywhere.

What this PR changes

Deck stops being a privileged resource. Its ledger and its check become properties of whatever is currently the root.
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, so a detached subtree heads a tree of its own again.
Everything that is not a root holds None.

A lookup is then two steps: hash the name in the root's ledger, then walk up the parent pointers to check the hit is inside the subtree that was asked.
Both are bounded by depth rather than size, so a resource still finds only what is at or beneath it, at the same cost wherever it is asked from.
And because the first step knows the name exists before the second rejects it, asking the wrong resource now says where it actually is.

Performance

The naming check is quadratic in the size of whatever you are building, because every child assigned re-walks everything already there.
That is invisible at 96 wells and not at 1536.
Building one DeepWell_Greiner_1536_Well from the standard library:

mainthis PR
_check_naming_conflicts calls1,180,4161,536
time per plate~150 ms~13 ms
96-well plate, for comparison1.48 ms1.20 ms

1,180,416 is exactly 1536 * 1537 / 2. The timings vary about 20% run to run, the call count does not.

A plate builds its own wells before it has ever met a deck, so the Deck ledger never covered the part that costs.

Where a Deck does apply, main is fine and in one case faster:

treeoperationmainthis PR
Deck at the root, 2,612 resourcesask the deck0.09 us0.23 us
Deck at the root, 2,612 resourcesask a carrier29.64 us0.23 us
facility, a Deck per device, 98,221build it1.01 s0.83 s

Build that same facility with no decks in it and main takes 499 s against 0.50 s here.

The ledger costs one map: 3.84 MB for 98,221 resources, about 3% of the tree, holding references to resources that already exist.

Why now

This matters for v1 because there is no liquid handler concept any more.
A workcell or facility don't need to contain a deck at all, so "there is a Deck at the root" is no longer a safe assumption to hang the resource model's performance on.
Tip becoming a resource pushes the same way: it roughly doubles every tip rack, from 97 resources to 193, and tip racks are built standalone, which is exactly where the fast path never applied.

@BioCam

BioCam commented Sep 1, 2026

Copy link
Copy Markdown
CollaboratorAuthor

I'm a very visual person, so here a visual aid to root updating subtree ledger dict across resource tree mergers and separation:

FacilityAssignmentScene.mp4

@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

lookup before and after - from different levels:

GetResourceScene.mp4

Comment threadpylabrobot/resources/resource.py

@rickwierengarickwierenga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it

@rickwierenga

Copy link
Copy Markdown
Member

please

@BioCam
BioCam merged commit 543362f into mainSep 2, 2026
21 checks passed
@rickwierenga
rickwierenga deleted the v1-resource-name-index branch September 2, 2026 14:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@BioCam@rickwierenga
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Scaling: Resource: remember which names a tree holds instead of re-reading it - #1222

Merged
BioCam merged 14 commits into
mainfrom
v1-resource-name-index
Sep 2, 2026
Merged

Scaling: Resource: remember which names a tree holds instead of re-reading it#1222
BioCam merged 14 commits into
mainfrom
v1-resource-name-index

Conversation

@BioCam

Copy link
Copy Markdown
Collaborator

Problem

_check_naming_conflicts enforces tree-wide name uniqueness by recursing over the whole tree, and assign_child_resource calls it on get_root() for every resource assigned. An assignment costs the size of everything already in the tree rather than the size of what is arriving, so building n resources costs n² - measured between n^1.9 and n^2.0 - and every later assignment still walks all of it.

A loaded STARlet deck is around 2 000 resources. One plate move on it, an unassign and a re-assign, already costs 47 ms. Put several instruments in one tree and the cost grows with all of them, not with the plate:

resourcesinstrumentsbeforeafter
2 001147.0 ms0.0 ms
4 002293.1 ms0.0 ms
8 0044186.7 ms0.0 ms
16 0088379.3 ms0.0 ms

Changes

  • Resource._name_index holds the names in a tree, kept only by the root, built the first time _names_in_tree asks for it.
  • _subtree_names returns the names at or beneath a resource; assign_child_resource unions the arriving ones in, unassign_child_resource subtracts them.
  • _check_naming_conflicts walks the arriving subtree against that set instead of recursing over both trees. It reads the index off get_root(), where it was already called from, so it still covers the whole tree.

Behaviour: unchanged, and an assignment now costs the size of what is arriving. Carrying the index forward is safe because a name cannot change while a resource is assigned - the setter refuses - and a tree only changes shape in the two methods that maintain it.

Scope: cheaper, not narrower. The check still runs before the branch that detaches an already-attached resource, so a move within one tree is still refused and callers still unassign first. Excluding the arriving resource's own names would fix that, and is a follow-up since it changes behaviour the tests pin.

Tests

TestNameIndex (a duplicate name, one buried in the arriving subtree, unassigning freeing a name, a subtree carrying its names into whatever tree takes it, and the order a move has to happen in). Checked separately against the recursion it replaces over 4 000 generated tree shapes, and the index against a fresh walk after each step of 300 assign and unassign sequences. ruff format, ruff check --select I, ruff check and mypy pylabrobot --check-untyped-defs are clean; the full suite passes (2 450 passed, 2 skipped, 206 subtests).

`_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) <noreply@anthropic.com>
@BioCam
BioCam marked this pull request as ready for review August 27, 2026 15:38
@rickwierenga

Copy link
Copy Markdown
Member

let's remove the stuff from the Deck class in this case

Comment threadpylabrobot/resources/resource.py Outdated
Comment on lines +183 to +187
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo _name_index should be called _subtree_names and _subtree_names should be named _get_subtree_names or something

Comment threadpylabrobot/resources/resource.py Outdated
@rickwierenga

Copy link
Copy Markdown
Member

it would be clean to use the will/did assign child resource

  • will: check name as it currently does
  • did: update cache (so no _names_in_tree cache checking needed)

BioCamand others added 7 commits August 28, 2026 12:25
…n `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) <noreply@anthropic.com>
…hem 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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
Restores `Resource`, `Deck` and their tests to d884b2f, the state under
review. The reverted commits changed how the index is maintained, which is
the open question in review and not settled yet.
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

let's remove the stuff from the Deck class in this case

@rickwierenga - Done.
_resources, _register_resource, _deregister_resource, get_resource, has_resource and the _check_naming_conflicts override are all removed, so Deck no longer keeps its own copy of the tree.

get_all_resources is public and has callers, so I kept it and only changed the body:
it was reading the private dict, and now delegates to get_all_children(). Happy to deprecate it separately if you would rather it went too.

One behaviour change I think is worth flagging:
the old override compared only resource.name, so assigning a carrier that already held a resource named like one on the deck was accepted, and put a duplicate name in the tree.
The Resource version checks the whole arriving subtree, so that case now raises.

That behaviour is what prompted me to generate #1228 .
A name is meant to be an identifier: unique across the tree, and fixed for the life of the resource.
Neither was true.
It could be duplicated on assignment, as above, and it could still be reassigned after the resource existed, which left anything already named after it out of step.
#1228 makes name immutable after instantiation; this check closes the other half.

BioCamand others added 5 commits August 31, 2026 22:45
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

I have made a couple of changes to build a stronger root-based index ledger that fixes a naming bug and improves the performance (needed for scaling of the PLR resource model, including the upcoming change of Tip to be a resource).
So here is a (hopefully more) concise summary:

What main does today

The naming check runs on self.get_root(), so which implementation you get depends on what the root is -> i.e is inconsistent.
Deck overrides it with a single dict lookup, docstringed "overwrite for speed". Everything else uses Resource's recursive version, whose cost grows with the tree.

So main is fast in one shape: a Deck on the path, asked about itself.
That is the liquid handler shape we are removing in v1, and it is the shape Deck was built for.
Outside it, the fast path was never available.

The bug that inconsistency causes

Deck's version compares only the arriving resource's own name, so a clash buried inside what is arriving is never looked at:

deck=STARLetDeck()
car_1=TIP_CAR_480_A00(name="car_1")
car_1[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01")
deck.assign_child_resource(car_1, rails=1)
car_2=TIP_CAR_480_A00(name="car_2")
car_2[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01") # same namedeck.assign_child_resource(car_2, rails=10) # accepted on main

Two resources called tips_01 in one tree, and the consequences are silent:

  • deck.get_resource("tips_01") returns the one on car_2. The rack on car_1 is unreachable by name at all.
  • serialize_all_state() produces 97 keys for the 194 resources across the two racks, so one rack's state overwrites the other's.

The equivalent tree under a non-Deck root is refused, because Resource's version checks the whole arriving subtree.
This PR refuses it everywhere.

What this PR changes

Deck stops being a privileged resource. Its ledger and its check become properties of whatever is currently the root.
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, so a detached subtree heads a tree of its own again.
Everything that is not a root holds None.

A lookup is then two steps: hash the name in the root's ledger, then walk up the parent pointers to check the hit is inside the subtree that was asked.
Both are bounded by depth rather than size, so a resource still finds only what is at or beneath it, at the same cost wherever it is asked from.
And because the first step knows the name exists before the second rejects it, asking the wrong resource now says where it actually is.

Performance

The naming check is quadratic in the size of whatever you are building, because every child assigned re-walks everything already there.
That is invisible at 96 wells and not at 1536.
Building one DeepWell_Greiner_1536_Well from the standard library:

mainthis PR
_check_naming_conflicts calls1,180,4161,536
time per plate~150 ms~13 ms
96-well plate, for comparison1.48 ms1.20 ms

1,180,416 is exactly 1536 * 1537 / 2. The timings vary about 20% run to run, the call count does not.

A plate builds its own wells before it has ever met a deck, so the Deck ledger never covered the part that costs.

Where a Deck does apply, main is fine and in one case faster:

treeoperationmainthis PR
Deck at the root, 2,612 resourcesask the deck0.09 us0.23 us
Deck at the root, 2,612 resourcesask a carrier29.64 us0.23 us
facility, a Deck per device, 98,221build it1.01 s0.83 s

Build that same facility with no decks in it and main takes 499 s against 0.50 s here.

The ledger costs one map: 3.84 MB for 98,221 resources, about 3% of the tree, holding references to resources that already exist.

Why now

This matters for v1 because there is no liquid handler concept any more.
A workcell or facility don't need to contain a deck at all, so "there is a Deck at the root" is no longer a safe assumption to hang the resource model's performance on.
Tip becoming a resource pushes the same way: it roughly doubles every tip rack, from 97 resources to 193, and tip racks are built standalone, which is exactly where the fast path never applied.

@BioCam

BioCam commented Sep 1, 2026

Copy link
Copy Markdown
CollaboratorAuthor

I'm a very visual person, so here a visual aid to root updating subtree ledger dict across resource tree mergers and separation:

FacilityAssignmentScene.mp4

@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

lookup before and after - from different levels:

GetResourceScene.mp4

Comment threadpylabrobot/resources/resource.py

@rickwierengarickwierenga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it

@rickwierenga

Copy link
Copy Markdown
Member

please

@BioCam
BioCam merged commit 543362f into mainSep 2, 2026
21 checks passed
@rickwierenga
rickwierenga deleted the v1-resource-name-index branch September 2, 2026 14:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@BioCam@rickwierenga
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Scaling: Resource: remember which names a tree holds instead of re-reading it - #1222

Merged
BioCam merged 14 commits into
mainfrom
v1-resource-name-index
Sep 2, 2026
Merged

Scaling: Resource: remember which names a tree holds instead of re-reading it#1222
BioCam merged 14 commits into
mainfrom
v1-resource-name-index

Conversation

@BioCam

Copy link
Copy Markdown
Collaborator

Problem

_check_naming_conflicts enforces tree-wide name uniqueness by recursing over the whole tree, and assign_child_resource calls it on get_root() for every resource assigned. An assignment costs the size of everything already in the tree rather than the size of what is arriving, so building n resources costs n² - measured between n^1.9 and n^2.0 - and every later assignment still walks all of it.

A loaded STARlet deck is around 2 000 resources. One plate move on it, an unassign and a re-assign, already costs 47 ms. Put several instruments in one tree and the cost grows with all of them, not with the plate:

resourcesinstrumentsbeforeafter
2 001147.0 ms0.0 ms
4 002293.1 ms0.0 ms
8 0044186.7 ms0.0 ms
16 0088379.3 ms0.0 ms

Changes

  • Resource._name_index holds the names in a tree, kept only by the root, built the first time _names_in_tree asks for it.
  • _subtree_names returns the names at or beneath a resource; assign_child_resource unions the arriving ones in, unassign_child_resource subtracts them.
  • _check_naming_conflicts walks the arriving subtree against that set instead of recursing over both trees. It reads the index off get_root(), where it was already called from, so it still covers the whole tree.

Behaviour: unchanged, and an assignment now costs the size of what is arriving. Carrying the index forward is safe because a name cannot change while a resource is assigned - the setter refuses - and a tree only changes shape in the two methods that maintain it.

Scope: cheaper, not narrower. The check still runs before the branch that detaches an already-attached resource, so a move within one tree is still refused and callers still unassign first. Excluding the arriving resource's own names would fix that, and is a follow-up since it changes behaviour the tests pin.

Tests

TestNameIndex (a duplicate name, one buried in the arriving subtree, unassigning freeing a name, a subtree carrying its names into whatever tree takes it, and the order a move has to happen in). Checked separately against the recursion it replaces over 4 000 generated tree shapes, and the index against a fresh walk after each step of 300 assign and unassign sequences. ruff format, ruff check --select I, ruff check and mypy pylabrobot --check-untyped-defs are clean; the full suite passes (2 450 passed, 2 skipped, 206 subtests).

`_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) <noreply@anthropic.com>
@BioCam
BioCam marked this pull request as ready for review August 27, 2026 15:38
@rickwierenga

Copy link
Copy Markdown
Member

let's remove the stuff from the Deck class in this case

Comment threadpylabrobot/resources/resource.py Outdated
Comment on lines +183 to +187
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo _name_index should be called _subtree_names and _subtree_names should be named _get_subtree_names or something

Comment threadpylabrobot/resources/resource.py Outdated
@rickwierenga

Copy link
Copy Markdown
Member

it would be clean to use the will/did assign child resource

  • will: check name as it currently does
  • did: update cache (so no _names_in_tree cache checking needed)

BioCamand others added 7 commits August 28, 2026 12:25
…n `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) <noreply@anthropic.com>
…hem 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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
Restores `Resource`, `Deck` and their tests to d884b2f, the state under
review. The reverted commits changed how the index is maintained, which is
the open question in review and not settled yet.
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

let's remove the stuff from the Deck class in this case

@rickwierenga - Done.
_resources, _register_resource, _deregister_resource, get_resource, has_resource and the _check_naming_conflicts override are all removed, so Deck no longer keeps its own copy of the tree.

get_all_resources is public and has callers, so I kept it and only changed the body:
it was reading the private dict, and now delegates to get_all_children(). Happy to deprecate it separately if you would rather it went too.

One behaviour change I think is worth flagging:
the old override compared only resource.name, so assigning a carrier that already held a resource named like one on the deck was accepted, and put a duplicate name in the tree.
The Resource version checks the whole arriving subtree, so that case now raises.

That behaviour is what prompted me to generate #1228 .
A name is meant to be an identifier: unique across the tree, and fixed for the life of the resource.
Neither was true.
It could be duplicated on assignment, as above, and it could still be reassigned after the resource existed, which left anything already named after it out of step.
#1228 makes name immutable after instantiation; this check closes the other half.

BioCamand others added 5 commits August 31, 2026 22:45
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

I have made a couple of changes to build a stronger root-based index ledger that fixes a naming bug and improves the performance (needed for scaling of the PLR resource model, including the upcoming change of Tip to be a resource).
So here is a (hopefully more) concise summary:

What main does today

The naming check runs on self.get_root(), so which implementation you get depends on what the root is -> i.e is inconsistent.
Deck overrides it with a single dict lookup, docstringed "overwrite for speed". Everything else uses Resource's recursive version, whose cost grows with the tree.

So main is fast in one shape: a Deck on the path, asked about itself.
That is the liquid handler shape we are removing in v1, and it is the shape Deck was built for.
Outside it, the fast path was never available.

The bug that inconsistency causes

Deck's version compares only the arriving resource's own name, so a clash buried inside what is arriving is never looked at:

deck=STARLetDeck()
car_1=TIP_CAR_480_A00(name="car_1")
car_1[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01")
deck.assign_child_resource(car_1, rails=1)
car_2=TIP_CAR_480_A00(name="car_2")
car_2[0] =hamilton_96_tiprack_1000uL_filter(name="tips_01") # same namedeck.assign_child_resource(car_2, rails=10) # accepted on main

Two resources called tips_01 in one tree, and the consequences are silent:

  • deck.get_resource("tips_01") returns the one on car_2. The rack on car_1 is unreachable by name at all.
  • serialize_all_state() produces 97 keys for the 194 resources across the two racks, so one rack's state overwrites the other's.

The equivalent tree under a non-Deck root is refused, because Resource's version checks the whole arriving subtree.
This PR refuses it everywhere.

What this PR changes

Deck stops being a privileged resource. Its ledger and its check become properties of whatever is currently the root.
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, so a detached subtree heads a tree of its own again.
Everything that is not a root holds None.

A lookup is then two steps: hash the name in the root's ledger, then walk up the parent pointers to check the hit is inside the subtree that was asked.
Both are bounded by depth rather than size, so a resource still finds only what is at or beneath it, at the same cost wherever it is asked from.
And because the first step knows the name exists before the second rejects it, asking the wrong resource now says where it actually is.

Performance

The naming check is quadratic in the size of whatever you are building, because every child assigned re-walks everything already there.
That is invisible at 96 wells and not at 1536.
Building one DeepWell_Greiner_1536_Well from the standard library:

mainthis PR
_check_naming_conflicts calls1,180,4161,536
time per plate~150 ms~13 ms
96-well plate, for comparison1.48 ms1.20 ms

1,180,416 is exactly 1536 * 1537 / 2. The timings vary about 20% run to run, the call count does not.

A plate builds its own wells before it has ever met a deck, so the Deck ledger never covered the part that costs.

Where a Deck does apply, main is fine and in one case faster:

treeoperationmainthis PR
Deck at the root, 2,612 resourcesask the deck0.09 us0.23 us
Deck at the root, 2,612 resourcesask a carrier29.64 us0.23 us
facility, a Deck per device, 98,221build it1.01 s0.83 s

Build that same facility with no decks in it and main takes 499 s against 0.50 s here.

The ledger costs one map: 3.84 MB for 98,221 resources, about 3% of the tree, holding references to resources that already exist.

Why now

This matters for v1 because there is no liquid handler concept any more.
A workcell or facility don't need to contain a deck at all, so "there is a Deck at the root" is no longer a safe assumption to hang the resource model's performance on.
Tip becoming a resource pushes the same way: it roughly doubles every tip rack, from 97 resources to 193, and tip racks are built standalone, which is exactly where the fast path never applied.

@BioCam

BioCam commented Sep 1, 2026

Copy link
Copy Markdown
CollaboratorAuthor

I'm a very visual person, so here a visual aid to root updating subtree ledger dict across resource tree mergers and separation:

FacilityAssignmentScene.mp4

@BioCam

Copy link
Copy Markdown
CollaboratorAuthor

lookup before and after - from different levels:

GetResourceScene.mp4

Comment threadpylabrobot/resources/resource.py

@rickwierengarickwierenga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ship it

@rickwierenga

Copy link
Copy Markdown
Member

please

@BioCam
BioCam merged commit 543362f into mainSep 2, 2026
21 checks passed
@rickwierenga
rickwierenga deleted the v1-resource-name-index branch September 2, 2026 14:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@BioCam@rickwierenga