fix(api): Report Python protocol line numbers for errors from Protocol Engine #13537
Conversation
There was a problem hiding this comment.
No substantive changes in here, just adding type annotations and rearranging things for readability.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.0.0 #13537 +/- ##
=======================================================
- Coverage 71.30% 71.30% -0.01%
=======================================================
Files 2418 2418
Lines 68007 68002 -5
Branches 7896 7896
=======================================================
- Hits 48492 48487 -5
Misses 17668 17668
Partials 1847 1847
Flags with carried forward coverage won't be shown. Click here to find out more.
|
SyntaxColoring
requested review from
sfoster1,
TamarZanzouri,
jbleon95 and
CaseyBatten
| # TODO(mm, 2023-09-12): Do we need to scan the exception tree like this? Instead, can we | ||
| # directly inspect the E-stop state through self._hardware_api.get_estop_state()? |
There was a problem hiding this comment.
@TamarZanzouri @sfoster1 Thoughts on this # TODO?
There was a problem hiding this comment.
We need to scan the exception tree because someone in theory could have released the estop really fast after pressing it and the physical estop state would not be the same as the one that caused the error.
There was a problem hiding this comment.
Mm, yeah, okay.
Would that be likely in practice? I think someone would have to physically release the E-stop button and PUT /robot/control/acknowledgeEstopDisengage before the exception finishes its journey from the hardware controller to ProtocolEngine.finish(). (I don't think that means it doesn't matter—just developing a mental model.)
Overview
Fixes RSS-317.
Test Plan
Changelog
Bug fix: When an exception bubbles out of the Python protocol
exec(), do not handle it specially just because that exception passed through Protocol Engine. Handle it like any other exception. This lets us attach the protocol line number to it.We were handling it specially before because, I guess, we wanted it to collude with this
isinstancecheck up inProtocolEngine.finish(). Apparently this was unnecessary, because theExceptionInProtocolErrorthat we would wrap it with passes theisinstancecheck just the same.Add some analysis integration tests that make sure the line number shows up.
Resolve some type-checking exclusions in
opentrons.protocols.execute.Review requests
Risk assessment
Probably medium-high. The special E-stop handling is hard to reason about and unit-test because it works by several far-away places colluding with each other.