Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(api): Port tip consumption to StateUpdate #16469

Merged
merged 9 commits into from
Oct 11, 2024
Merged

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Oct 11, 2024

Overview

A refactor towards EXEC-758 and EXEC-764.

Test Plan and Hands on Testing

  • Make sure analysis snapshot tests continue to pass, indicating that tip tracking has not changed.

Changelog

  • TipStore was looking for PickUpTipResults to track tips being consumed from tip racks. Convert this to use the newer StateUpdate mechanism.
  • Various other small refactors. See the commit messages for details.

Review requests

None in particular.

Risk assessment

Low if tests pass.

@SyntaxColoring SyntaxColoring requested a review from a team October 11, 2024 17:11
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner October 11, 2024 17:11
Comment on lines +32 to +41
# todo(mm, 2024-10-10): This info is duplicated between here and PipetteState because
# TipStore is using it to compute which tips a PickUpTip removes from the tip rack,
# given the pipette's current nozzle map. We could avoid this duplication by moving the
# computation to TipView, calling it from PickUpTipImplementation, and passing the
# precomputed list of wells to TipStore.
@dataclass
class _PipetteInfo:
channels: int
active_channels: int
nozzle_map: NozzleMap
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this seem like a reasonable plan?

Copy link
Member

Choose a reason for hiding this comment

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

yes, definitely

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks good, and agree with the path forward

Comment on lines +32 to +41
# todo(mm, 2024-10-10): This info is duplicated between here and PipetteState because
# TipStore is using it to compute which tips a PickUpTip removes from the tip rack,
# given the pipette's current nozzle map. We could avoid this duplication by moving the
# computation to TipView, calling it from PickUpTipImplementation, and passing the
# precomputed list of wells to TipStore.
@dataclass
class _PipetteInfo:
channels: int
active_channels: int
nozzle_map: NozzleMap
Copy link
Member

Choose a reason for hiding this comment

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

yes, definitely

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Makes sense to me

Comment on lines -82 to -93
if nozzle_map:
self._state.active_channels_by_pipette_id[
pipette_id
] = nozzle_map.tip_count
self._state.nozzle_map_by_pipette_id[pipette_id] = nozzle_map
else:
self._state.active_channels_by_pipette_id[
pipette_id
] = self._state.channels_by_pipette_id[pipette_id]

elif isinstance(action, FailCommandAction):
self._handle_failed_command(action)
Copy link
Member

Choose a reason for hiding this comment

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

Oh nice! Is NozzleMap always available now? I think there was some state that had nozzle map optional even though I think the map was indeed available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice! Is NozzleMap always available now?

Yep, for the purposes of TipStore, at least.

I think there was some state that had nozzle map optional even though I think the map was indeed available.

Good memory. I think that's this, over in PipetteStore. I'll play around with that and see if we can simplify it now. #14529 (comment)

@SyntaxColoring SyntaxColoring merged commit 76df073 into edge Oct 11, 2024
45 checks passed
@SyntaxColoring SyntaxColoring deleted the port_tip_state branch October 11, 2024 19:14
Sign up for free to 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.

3 participants