Migrate Nimbus to Device/Driver/CapabilityBackend architecture#975
Migrate Nimbus to Device/Driver/CapabilityBackend architecture#975rickwierenga wants to merge 2 commits intov1b1from
Conversation
Split the monolithic NimbusBackend (~2300 lines) into the new PLR architecture: - NimbusDriver(Driver): TCP I/O, protocol init, device-level ops (park, door lock) - NimbusPIPBackend(PIPBackend): protocol translation for liquid handling - Nimbus(Device): wires driver + PIP capability frontend - NimbusChatterboxDriver: no-op driver for testing without hardware - commands.py: all HamiltonCommand subclasses extracted to shared module The legacy NimbusBackend is rewritten as a thin wrapper that delegates to the new classes, preserving backwards compatibility. All 66 existing tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates the Hamilton Nimbus implementation from a monolithic legacy backend into the repository’s newer Device/Driver/CapabilityBackend architecture, while preserving the legacy import path and API surface via a thin wrapper.
Changes:
- Introduces
NimbusDriverfor TCP/protocol lifecycle + discovery and device-level operations (park/door lock). - Adds
NimbusPIPBackendto translate PLR PIP operations into Nimbus TCP commands. - Rewrites legacy
NimbusBackendas a delegating wrapper and re-exports command helpers for backwards compatibility.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
pylabrobot/legacy/liquid_handling/backends/hamilton/nimbus_backend.py |
Replaces the large legacy implementation with a wrapper delegating to the new driver + PIP backend. |
pylabrobot/hamilton/liquid_handlers/nimbus/driver.py |
Adds the TCP-based driver handling initialization, discovery, and device-level commands. |
pylabrobot/hamilton/liquid_handlers/nimbus/pip_backend.py |
Adds the PIP backend translating pick up/drop/aspirate/dispense into Nimbus commands. |
pylabrobot/hamilton/liquid_handlers/nimbus/commands.py |
Moves Nimbus command definitions and tip helpers into a dedicated module. |
pylabrobot/hamilton/liquid_handlers/nimbus/nimbus.py |
Adds the user-facing Nimbus(Device) wiring the driver to the PIP capability. |
pylabrobot/hamilton/liquid_handlers/nimbus/chatterbox.py |
Adds a hardware-free “chatterbox” driver that logs commands and returns canned responses. |
pylabrobot/hamilton/liquid_handlers/nimbus/__init__.py |
Exposes Nimbus from the new module. |
pylabrobot/hamilton/liquid_handlers/__init__.py |
Re-exports Nimbus at the liquid_handlers package level. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert self._pip is not None | ||
| # Legacy Pickup and new Pickup are structurally identical | ||
| new_ops = [NewPickup(resource=op.resource, offset=op.offset, tip=op.tip) for op in ops] | ||
| params = PickUpTipsParams( | ||
| minimum_traverse_height_at_beginning_of_a_command=minimum_traverse_height_at_beginning_of_a_command, |
There was a problem hiding this comment.
Avoid using assert self._pip is not None for guarding public API calls (e.g., pick_up_tips). This changes the error type vs the legacy backend (AssertionError instead of a clear RuntimeError) and assertions can be stripped with python -O, potentially causing harder-to-diagnose failures later. Prefer an explicit check that raises a RuntimeError with a helpful message (and apply the same pattern to the other methods in this wrapper that currently assert _pip is initialized).
| minimum_traverse_height_at_beginning_of_a_command = self._channel_traversal_height | ||
| minimum_traverse_height_at_beginning_of_a_command_units = round( | ||
| minimum_traverse_height_at_beginning_of_a_command * 100 | ||
| assert self._pip is not None |
There was a problem hiding this comment.
drop_tips currently uses assert self._pip is not None as a precondition. For a legacy backend wrapper this should be a deterministic runtime error (and not removable with -O). Replace the assert with an explicit check that raises a clear RuntimeError (e.g., prompting the caller to run setup() first).
| assert self._pip is not None | |
| if self._pip is None: | |
| raise RuntimeError( | |
| "NimbusBackend PIP backend is not initialized. " | |
| "Call setup() before invoking drop_tips()." | |
| ) |
| assert self._pip is not None | ||
| new_ops = [ | ||
| Aspiration( |
There was a problem hiding this comment.
aspirate uses assert self._pip is not None to enforce setup. Asserts are not appropriate for user-facing validation (they can be optimized away and produce unhelpful AssertionErrors). Please switch to an explicit runtime check that raises a RuntimeError with a message consistent with the legacy backend (e.g., "Call setup() first").
| op.flow_rate | ||
| if op.flow_rate is not None | ||
| else _get_default_flow_rate(op.tip, is_aspirate=False) | ||
| assert self._pip is not None |
There was a problem hiding this comment.
dispense is guarded by assert self._pip is not None. Please use an explicit check + RuntimeError instead so callers get a stable exception type/message (and the guard is not removed under Python optimizations).
| assert self._pip is not None | |
| if self._pip is None: | |
| raise RuntimeError("NimbusBackend PIP backend is not initialized; cannot perform dispense.") |
| tube_section_ratio = [0] * self.num_channels | ||
| recording_mode = 0 | ||
| async def request_tip_presence(self) -> List[Optional[bool]]: | ||
| assert self._pip is not None |
There was a problem hiding this comment.
request_tip_presence currently uses assert self._pip is not None. This method is often called as part of error recovery (e.g., by higher-level frontends), so it should raise a predictable RuntimeError when the backend isn't set up rather than an AssertionError (and it shouldn't be removable with -O).
| assert self._pip is not None | |
| if self._pip is None: | |
| raise RuntimeError( | |
| "Nimbus backend is not initialized: cannot request tip presence." | |
| ) |
| def can_pick_up_tip(self, channel_idx: int, tip: Tip) -> bool: | ||
| if self._pip is not None: | ||
| return self._pip.can_pick_up_tip(channel_idx, tip) | ||
| return True |
There was a problem hiding this comment.
can_pick_up_tip returns True when _pip is not initialized yet. This is a behavior change from the previous implementation (which rejected non-Hamilton tips / XL tips even pre-setup) and can cause the legacy LiquidHandler frontend to accept incompatible tip models. Consider keeping the hardware-compatibility checks here independent of _pip (e.g., validate HamiltonTip + TipSize even when _pip is None).
| return True | |
| # When the PIP backend is not initialized, we cannot reliably assess | |
| # hardware compatibility, so be conservative and reject the tip. | |
| logger.debug( | |
| "can_pick_up_tip called before PIP backend initialization; " | |
| "rejecting tip %r on channel %d", | |
| tip, | |
| channel_idx, | |
| ) | |
| return False |
| indexes=[2], | ||
| ) | ||
| ) | ||
| assert config is not None |
There was a problem hiding this comment.
This code uses assert config is not None without an error message after GetChannelConfiguration. If send_command() unexpectedly returns None, this will raise a bare AssertionError that's hard to diagnose (and removable with -O). Prefer an explicit runtime check with a descriptive exception (or at least add an assertion message) so failures are actionable.
| assert config is not None | |
| if config is None: | |
| raise RuntimeError( | |
| f"GetChannelConfiguration returned None for channel {channel_num}" | |
| ) |
| indexes=[2], | ||
| ) | ||
| ) | ||
| assert config is not None |
There was a problem hiding this comment.
Same issue as in aspirate: assert config is not None here has no message and can be optimized away. Consider replacing with an explicit check that raises a RuntimeError including the channel/indexes being queried, so unexpected None responses are easier to debug.
| assert config is not None | |
| if config is None: | |
| raise RuntimeError( | |
| f"GetChannelConfiguration returned None for channel {channel_num} and indexes [2]" | |
| ) |
…ck_up_tip default Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rickwierenga
force-pushed
the
v1b1
branch
5 times, most recently
from
475dbab to
588abbb
Compare
Summary
NimbusBackend(~2300 lines) into the new PLR architecture following the STAR migration patternNimbusDriver(Driver): TCP I/O, protocol init (Protocol 7 + Protocol 3), object discovery, device-level ops (park, door lock)NimbusPIPBackend(PIPBackend): protocol translation for all liquid handling operations (pick_up_tips, drop_tips, aspirate, dispense)Nimbus(Device): wires driver + PIP capability frontendNimbusChatterboxDriver: no-op driver for hardware-free testingNimbusBackendrewritten as thin wrapper that delegates to new classes, preserving backwards compatibilityTest plan
from pylabrobot.hamilton.liquid_handlers.nimbus import Nimbusfrom pylabrobot.legacy.liquid_handling.backends.hamilton.nimbus_backend import NimbusBackend🤖 Generated with Claude Code