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.

Sign up for GitHub

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

fix(api): Error instead of infinite-looping when disposal_volume is too high to make progress #6754

Closed
wants to merge 17 commits into from
+224 −85
Closed

fix(api): Error instead of infinite-looping when disposal_volume is too high to make progress #6754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a4d8384
Add failing (hanging, really) test case.
SyntaxColoring Mar 9, 2021
91f300a
Refactor and expand unit test.
SyntaxColoring Oct 12, 2020
b83572f
Improve test readability.
SyntaxColoring Mar 10, 2021
ff19cd3
Add todo comment for TransferPlan refactor.
SyntaxColoring Mar 10, 2021
09a427a
First pass at a solution, with some todo comments.
SyntaxColoring Mar 9, 2021
79d3603
xfail out-of-scope bugs.
SyntaxColoring Mar 10, 2021
4900539
Format.
SyntaxColoring Mar 10, 2021
3cc63eb
Remove todo comment for out-of-scope bug.
SyntaxColoring Mar 10, 2021
fde3cd4
Single-space error string.
SyntaxColoring Mar 10, 2021
f00b9d6
Exhaust iterator with `list()` instead of a loop.
SyntaxColoring Mar 10, 2021
44ed13e
Clarify xfail comment.
SyntaxColoring Mar 10, 2021
fd7787c
Add dev dependency on pytest-timeout.
SyntaxColoring Mar 19, 2021
3e9c3ca
Add even more hanging and failing tests.
SyntaxColoring Mar 19, 2021
35568d5
Strictly expect failing tests to fail.
SyntaxColoring Mar 19, 2021
f9e6831
Parametrize which mode (transfer/distribute/consolidate).
SyntaxColoring Mar 22, 2021
04259e1
Leave todo notes.
SyntaxColoring Mar 22, 2021
364b00f
Elaborate on todo notes.
SyntaxColoring Mar 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/Pipfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ flake8-annotations = "~=2.4.1"
flake8-docstrings = "~=1.5.0"
decoy = "~=1.2.0"
pytest-lazy-fixture = "==0.6.3"
pytest-timeout = "*"

[packages]
aionotify = "==0.2.0"
Expand Down
179 changes: 94 additions & 85 deletions api/Pipfile.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions api/src/opentrons/protocols/advanced_control/transfers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,9 @@ def __init__(self,
volume,
sources,
dests,
# todo(mm, 2021-03-10):
# Refactor to not need an InstrumentContext, so we can more
# easily test this class's logic on its own.
instr: 'InstrumentContext',
max_volume: float,
api_version: APIVersion,
Expand Down Expand Up @@ -554,6 +557,8 @@ def _plan_distribute(self):
# the other maintains consistency in default behaviors of all functions
plan_iter = self._expand_for_volume_constraints(
self._volumes, self._dests,
# todo(mm, 2021-03-09): Is this a bug?
# Should this be the *working volume*?
self._instr.max_volume
- self._strategy.disposal_volume
- self._strategy.air_gap)
Expand Down Expand Up @@ -600,6 +605,11 @@ def _expand_for_volume_constraints(
""" Split a sequence of proposed transfers if necessary to keep each
transfer under the given max volume.
"""

if max_volume <= 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might make our future rework a little bit harder, but it's probably worth guarding this in an apiVersion check, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original hope was that this PR would be narrowly scoped to just the variants of this bug whose fixes wouldn't need an apiVersion check. See "Risk assessment" in my OP.

Maybe that's an exercise in futility. Maybe we should expand the scope of this PR to fix more variants of the bug, and wrap them all in an apiVersion check?

raise ValueError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we take this opportunity to define a more specific error type for transfer build errors?

Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I'm thinking about this too.

If you think just swapping the type of ValueError to something more specific would help, I can certainly do that.

I'm also trying to figure out how to get more user-friendly errors for people using the outer protocol API, like:

ValueError: Can't distribute with a disposal_volume of 400 µL when the tip can only hold 300 µL.

As opposed to either of these:

ValueError: max_volume must be greater than 0. (Got 0.)

UnplannableTransferError: max_volume must be greater than 0. (Got 0.)

The tension is:

  • _expand_for_volume_constraints() is central, so if we validate its arguments here, that validation can cover a lot of ground.
  • But on the other hand, it's too low-level for that validation to raise user-friendly diagnostics. It can't know why max_volume is 0, so error messages will necessarily be opaque and jargony.

f"max_volume must be greater than 0. (Got {max_volume}.)")

for volume, target in zip(volumes, targets):
while volume > max_volume * 2:
yield max_volume, target
Expand Down Expand Up @@ -648,6 +658,8 @@ def _plan_consolidate(self):
.. Aspirate -> .....*
"""
plan_iter = self._expand_for_volume_constraints(
# todo(mm, 2021-03-09): Is this right? Why don't we account for
# disposal volume?
self._volumes, self._sources, self._instr.max_volume)
current_xfer = next(plan_iter)
if self._strategy.new_tip == types.TransferTipPolicy.ALWAYS:
Expand Down
Loading