-
Notifications
You must be signed in to change notification settings - Fork 180
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
feat(api): implement loadLiquidClass command in PE #16814
Conversation
@@ -148,6 +156,7 @@ def get_summary(self) -> StateSummary: | |||
wells=self._wells.get_all(), | |||
hasEverEnteredErrorRecovery=self._commands.get_has_entered_recovery_mode(), | |||
files=self._state.files.file_ids, | |||
# TODO(dc): Do we want to just dump all the liquid classes into the summary? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely!
Could someone teach me what StateView.get_summary() is supposed to do?
Will answer your question here. StateView.get_summary()
returns a snapshot of the state of a selection of the engine components. The selection is determined mostly by client needs (or any entity that starts a run and needs to monitor its progress) as this state summary is a big part of the 'run data'.
Places where run data (and by implication, state summary) is provided-
- as a response to GET
/runs
,/maintenance_runs
andruns/{runId}
- as a response to all
/protocols
endpoints that return an analysis result. The analysis result is simply the 'final run data' of a run with a virtual robot. - as a result of the
analyze
cli tool as well as the opentronssimulate
andexecute
cli commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that info!
I think I want to handle the summary implementation in a separate PR, since this PR is quite large already.
Apparently, the StateSummary
feeds into AnalyzeResults
, which says it needs to be kept in sync with robot-server
's model, etc., so there are a lot of places I'll need to update. I'll ask you later how those classes are related to each other. But I'd like to handle that in a separate PR, and merge this PR as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!!
Overview
This is the second half of AUTH-851.
This implements the
loadLiquidClass()
command using the liquid class store from the previous PR.Liquid classes are read-only in the Protocol Engine, so each
liquidClassId
refers to one specific liquid class definition. Each mutation of a liquid class needs to be stored under a differentliquidClassId
. The caller can specify theliquidClassId
if they want to, or else we will generate one for them.So there are 4 cases we need to handle:
liquidClassId
and the liquid class is new: Generate a newliquidClassId
.liquidClassId
but the liquid class has already been stored: Reuse the existingliquidClassId
.liquidClassId
that we haven't seen before: Store the liquid class under the newliquidClassId
.liquidClassId
that's already been loaded: Check that the incoming liquid class matches the one we previously stored, and:Test Plan and Hands on Testing
I added 5 test cases to cover each of the scenarios above.
Review requests
Could someone teach me what
StateView.get_summary()
is supposed to do?Risk assessment
Low risk, liquid classes are not released yet and these changes should be dev-only. This change makes the Protocol Engine state marginally larger.