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.

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

Open
wants to merge 4 commits into
base: edge
Choose a base branch
from

Conversation

ddcc4
Copy link
Contributor

@ddcc4 ddcc4 commented Nov 14, 2024

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 different liquidClassId. The caller can specify the liquidClassId if they want to, or else we will generate one for them.

So there are 4 cases we need to handle:

  1. Caller did not specify a liquidClassId and the liquid class is new: Generate a new liquidClassId.
  2. Caller did not specify a liquidClassId but the liquid class has already been stored: Reuse the existing liquidClassId.
  3. Caller specified a liquidClassId that we haven't seen before: Store the liquid class under the new liquidClassId.
  4. Caller specified a liquidClassId that's already been loaded: Check that the incoming liquid class matches the one we previously stored, and:
    1. If the incoming liquid class exactly matches the existing one, do nothing.
    2. If they don't match, raise an error.

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.

@ddcc4 ddcc4 requested a review from a team as a code owner November 14, 2024 15:42
@@ -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?
Copy link
Member

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-

  1. as a response to GET /runs, /maintenance_runs and runs/{runId}
  2. 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.
  3. as a result of the analyze cli tool as well as the opentrons simulate and execute cli commands

@ddcc4 ddcc4 requested a review from a team as a code owner November 14, 2024 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants