fix(hamilton): resolve trash_core96 from the deck instead of a stale pointer - #1197
Open
WillVT84 wants to merge 1 commit into
Open
fix(hamilton): resolve trash_core96 from the deck instead of a stale pointer#1197WillVT84 wants to merge 1 commit into
WillVT84 wants to merge 1 commit into
Conversation
…pointer `HamiltonSTARDeck._trash96` is only assigned in `__init__`, so `get_trash_area96()` returned a reference that could not survive the deck being rebuilt or the trash being unassigned. Two failures followed. `serialize()` encodes the 96 trash as a child and emits `with_trash96=False`, so a deck restored via `Deck.deserialize()` ran `__init__` with the flag off and left `_trash96` as None -- the resource was present and correctly positioned, but `get_trash_area96()` raised. Callers had to re-establish the pointer by hand. Separately, `clear(include_trash=True)` unassigns the trash without clearing the attribute, so the getter handed back an orphaned `Trash` with no parent, failing later and further from the cause. Resolve the resource by name against the child tree, matching `Deck.get_trash_area()`, and only trust the cached reference while it is still assigned to this deck. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
@rickwierenga This may be an artifact of how we manage decks in our implementation, but I've been having to evoke a helper function to re-establish this pointer whenever we load our deck in a protocol. Figured I'd submit a bug fix here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
HamiltonSTARDeck._trash96is assigned only in__init__, andget_trash_area96()returns it directly. That cached reference does not survive the deck being rebuilt or the trash being unassigned, which produces two distinct failures.1. Deserialized decks raise even though the trash is present.
serialize()encodes the 96 trash as a child resource and emitswith_trash96=False. A deck restored throughDeck.deserialize()therefore runs__init__with the flag off and leaves_trash96asNone, whiletrash_core96is re-attached from the serialized children. The resource is on the deck and correctly positioned, but the getter raises:Anyone loading a saved layout has to re-establish the pointer by hand.
2.
clear(include_trash=True)leaves a stale pointer.clear()unassigns the trash but never clears the attribute, so the getter hands back an orphanedTrashwithparent = None. That fails later and further from the cause than an outright error would.Fix
Resolve the resource by name against the child tree, mirroring how the base class's
Deck.get_trash_area()already works, and only trust the cached reference while it is still assigned to this deck. Theparent is selfguard is what fixes the second case: an unassigned trash falls through to the lookup and then raises honestly.Behaviour when the trash genuinely was not created (
with_trash96=False) is unchanged.Tests
Two regression tests in
hamilton_deck_tests.py, one per failure mode. Both were confirmed to fail against the unfixed source — the round-trip test errors with the originalRuntimeError, and theinclude_trashtest fails with "RuntimeError not raised" — and to pass with the fix.Verification
pytest pylabrobot/resources— 225 passed. The 5 failures are pre-existing Opentrons deck tests that fetch labware over the network and hitSSL: CERTIFICATE_VERIFY_FAILEDlocally; identical on baseline.ruff check/ruff format --check(0.15.4, per the pin) — clean on both changed files.mypy(1.18.2, per the pin) —Success: no issues found in 2 source files.Note
Not addressed here, but the underlying design smell is that
serialize()writeswith_trash/with_trash96/core_grippersasFalse/Nonebecause the data lives in the children — the "not very pretty to have this key though" comments. That constructor-flags-vs-children mismatch is what produced this bug, and may affect other cached references the same way. Happy to follow up separately if that's of interest.🤖 Generated with Claude Code