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): JSON protocol pipette loading support in Protocol Engine #7766

Merged
merged 3 commits into from
May 4, 2021

Conversation

amitlissack
Copy link
Contributor

@amitlissack amitlissack commented May 4, 2021

Overview

JSON Protocols define IDs for the required pipettes. This PR provides a JSON protocol runner the ability to send commands to the ProtocolEngine to load a pipette with a specific id.

closes #7430

Changelog

  • LoadPipetteRequest has optional pipetteId parameter. The pipetteId will be used as the state store ID if specified otherwise a new ID will be generated.

Review requests

Risk assessment

None

@amitlissack amitlissack added api Affects the `api` project robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). labels May 4, 2021
@amitlissack amitlissack requested review from a team, celsasser and mcous and removed request for a team and celsasser May 4, 2021 13:47
@amitlissack amitlissack marked this pull request as ready for review May 4, 2021 13:50
@amitlissack amitlissack requested review from a team as code owners May 4, 2021 13:50
@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #7766 (15c0570) into edge (60df556) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #7766   +/-   ##
=======================================
  Coverage   83.06%   83.06%           
=======================================
  Files         331      331           
  Lines       21405    21407    +2     
=======================================
+ Hits        17780    17782    +2     
  Misses       3625     3625           
Impacted Files Coverage Δ
opentrons/protocol_engine/execution/equipment.py 100.00% <0.00%> (ø)
opentrons/protocol_engine/commands/load_pipette.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60df556...15c0570. Read the comment docs.

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -51,11 +51,13 @@ def load_pipette(
self,
pipette_name: PipetteName,
mount: MountType,
pipette_id: Optional[str],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this ever be needed in this client? I'd be down to leave it out (or default it to None if that feels better)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I'll remove it.

@@ -97,6 +108,7 @@ async def load_pipette(
except RuntimeError as e:
raise FailedToLoadPipetteError(str(e)) from e

pipette_id = self._resources.id_generator.generate_id()
pipette_id = pipette_id if pipette_id else \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be pedantic about checking is not None vs "is falsey"?

@@ -19,6 +22,11 @@ class LoadPipetteRequest(BaseModel):
...,
description="The mount the pipette should be present on.",
)
pipetteId: Optional[str] = Field(
...,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question I think you posted in the labware PR, but should we default this to None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@amitlissack amitlissack merged commit 7aa9034 into edge May 4, 2021
@amitlissack amitlissack deleted the api-pipette-id-pe branch May 4, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON load pipettes to Protocol Engine requests
2 participants