fix(api): Addition of distribution error to prevent invalid disposal values #13659
Conversation
Prevents the distribute functionality from accepting a disposal value equal to or greater than the maximum value of the instrument.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## edge #13659 +/- ##
==========================================
- Coverage 71.15% 71.15% -0.01%
==========================================
Files 2431 2431
Lines 68420 68419 -1
Branches 8053 8053
==========================================
- Hits 48682 48681 -1
Misses 17865 17865
Partials 1873 1873
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
|
||
| if self._strategy.disposal_volume >= self._instr.max_volume: | ||
| raise ValueError( | ||
| "The Disposal Volume must be less than the Maximum Volume of the Instrument" |
There was a problem hiding this comment.
Looks good! We can probably lowercase all these terms in the error text:
| "The Disposal Volume must be less than the Maximum Volume of the Instrument" | |
| "The disposal volume must be less than the maximum volume of the pipette" |
Updated error message to be consistent with desired message, moved error raise to a validation function call to ensure _plan_distribute is not too complex
|
Okay, I've done some archeology and found that this was previously reported as GitHub issue #6170 and we tried to fix it in PR #6754. We closed that PR without merging it because of these reasons. 2.5 years later, I no longer think those reasons are good. In particular, screw this:
Versioning in the Python Protocol API is good to avoid flagrantly breaking people's protocols. But it shouldn't prevent us from fixing flagrant bugs like infinite loops. That's not helping anybody. So, for this PR, here's what we should do:
|
Working volume used to better reflect operating case of distribute, test case added to test transfers regarding distribute test
Test cases added/modified for disposal vol and air gaps, addition of calls to check for this issue across all transfer plans, assertion of check in validation
api/tests/opentrons/protocols/advanced_control/test_transfers.py
Outdated
Show resolved
Hide resolved
| @@ -796,6 +796,83 @@ def test_all_options(_instr_labware): | |||
| assert xfer_plan_list == exp1 | |||
|
|
|||
|
|
|||
| def test_invalid_air_gap_disposal_sum_distribute(_instr_labware): | |||
There was a problem hiding this comment.
I think we should leave this to a separate PR, but:
This adds a few test functions that differ only in the values that they provide for disposal_volume and air_gap. This would be a good use case for pytest parametrization. We could write this as one function that gets parametrized with the different disposal_volume and air_gap combinations.
We could also parametrize this over the mode of TransferPlan: "distribute", "consolidate", or "transfer". That would be good because our bugfix does touch those other functions.
Adjustment of calls, change over to max volume control, formatting and improvement of test case formatting
_plan_consolidate volume parameter check disabled
|
Primary fail issues cleared, formatting good, I believe this is ready for merge. |
There was a problem hiding this comment.
This looks good to merge, assuming CI passes. I'm a little worried about that failing G-code test. I'm checking now to see if it's something that was introduced by this PR, or whether it's the known issue that was fixed in #13670.
CaseyBatten
deleted the
Distribute-disposal-value-error-case-to-prevent-invalid-values
branch
Overview
Prevents the distribute functionality from accepting a disposal value equal to or greater than the maximum value of the instrument. This was necessary to prevent an infinite looping issue where when disposal values greater than or equal to the maximum volume of an instrument were provided, which would cause the looping component of the _plan_distribute function to get caught in an state that was impossible to complete. This would seek to solve the issue detailed in RSS-347.