fix(api): add runner load error handling to analyze cli #15400
Conversation
api/src/opentrons/cli/analyze.py
Outdated
| async def _do_analyze(protocol_source: ProtocolSource) -> RunResult: | ||
|
|
||
| runner = await create_simulating_runner( | ||
| robot_type=protocol_source.robot_type, protocol_config=protocol_source.config | ||
| ) | ||
| return await runner.run(deck_configuration=[], protocol_source=protocol_source) | ||
| if isinstance(runner, PythonAndLegacyRunner): |
There was a problem hiding this comment.
Oops! Forgot to load the non-python runner. Doing that now
api/src/opentrons/cli/analyze.py
Outdated
| ) | ||
|
|
||
|
|
||
| @track_analysis |
There was a problem hiding this comment.
Is this @track_analysis addition intentional, or is it a merge conflict mistake?
There was a problem hiding this comment.
Oh! Good catch! Definitely merge conflict mistake
| commands=[], | ||
| state_summary=StateSummary( | ||
| errors=[error_occ], | ||
| status=EngineStatus.IDLE, |
There was a problem hiding this comment.
We're supplying a placeholder EngineStatus.IDLE because that's needed to construct the returned RunResult. But the returned RunResult is, elsewhere in this file, mostly just thrown away and translated into the AnalyzeResults that we actually care about.
Can we avoid this throwaway placeholder thing by directly returning the AnalyzeResults from this function?
There was a problem hiding this comment.
Trying to avoid creating this placeholder ends up mostly adding bloat code or some funny branching logic because when there's no error during runner load, the analysis result does need to be constructed from a RunResult.
I don't think the small placeholder is worth the trouble.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #15400 +/- ##
===========================================
+ Coverage 63.20% 92.43% +29.23%
===========================================
Files 287 77 -210
Lines 14891 1283 -13608
===========================================
- Hits 9412 1186 -8226
+ Misses 5479 97 -5382
Flags with carried forward coverage won't be shown. Click here to find out more. |
sanni-t
deleted the
AUTH-486-in-app-analysis-failure-during-rtp-validation-is-buggy
branch
Closes AUTH-486
Overview
Fixes the issues caught in #15211
Previously, RTP validation occurred as part of a simulating runner's run process. This run process executes the protocol inside a separate task, and any errors raised while running the task are wrapped and presented in the 'Rune result" as an analysis error.
Now that RTP validation is happening as part of the runner load process, we have to handle the error raised in the load process in a similar way as well. This PR adds this missing error handling in
analyzecli function.Test Plan
Review requests
Risk assessment
Medium. Updates a critical part of in-app analysis, but is a fix and is isolated to error handling of
runner.loadonly.