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(robot-server): Define default deck configurations #13990

Merged
merged 12 commits into from
Nov 17, 2023

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Nov 15, 2023

Overview

Before this PR, if you do GET /deck_configuration on a fresh robot, it returns an empty list. This is confusing and wrong because an empty list is, by our own rules, invalid data for this endpoint.

We need to either:

  • Return null, explicitly indicating that there is no deck configuration.
  • Return some kind of default.

We discussed this and decided to go with returning a default.

Test Plan

Basic functionality is covered by integration tests.

End-to-end functionality is not covered. By "end-to-end functionality," I mean things like "these defaults let you run all the old protocols that you were running before when you upgrade from v7.0 to v7.1." I'm not sure how to cover that.

Changelog

  • Hard-code some default deck configurations for the ot2_standard, ot2_short_trash, and ot3_standard deck definitions.
    • Per discussion, the defaults have a trash in the back-right corner.
    • For immediate convenience, I've defined these in robot-server, in terms of its HTTP-facing Pydantic models. This is definitely wrong: we will need this stuff to be in api instead, in order to support opentrons_execute and friends. Moving everything over is for another PR.
  • Make GET /deck_configuration return those defaults if the client has not yet issued a PUT /deck_configuration request.

Review requests

  • Does this make sense for the OT-2, given its "legacy fixtures" mechanism?

Risk assessment

Low.

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #13990 (eade227) into edge (18addf6) will decrease coverage by 0.01%.
Report is 1 commits behind head on edge.
The diff coverage is 38.46%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13990      +/-   ##
==========================================
- Coverage   70.64%   70.64%   -0.01%     
==========================================
  Files        2506     2506              
  Lines       70932    70937       +5     
  Branches     8781     8783       +2     
==========================================
+ Hits        50112    50115       +3     
- Misses      18663    18665       +2     
  Partials     2157     2157              
Flag Coverage Δ
app 67.66% <ø> (ø)
protocol-designer 45.66% <0.00%> (-0.02%) ⬇️
shared-data 73.60% <100.00%> (+0.02%) ⬆️
step-generation 86.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...-server/robot_server/runs/router/actions_router.py 100.00% <ø> (ø)
...bot-server/robot_server/runs/router/base_router.py 96.33% <ø> (ø)
...data/python/opentrons_shared_data/deck/__init__.py 65.62% <100.00%> (+3.55%) ⬆️
...l-designer/src/components/DeckSetup/SlotLabels.tsx 0.00% <0.00%> (ø)
...igner/src/components/modules/StagingAreasModal.tsx 43.13% <0.00%> (+0.82%) ⬆️
...otocol-designer/src/components/DeckSetup/index.tsx 0.00% <0.00%> (ø)

@SyntaxColoring SyntaxColoring requested review from a team and removed request for jbleon95 November 17, 2023 17:58
Resolve conflicts in robot-server/robot_server/deck_configuration/store.py.
The edge branch's DeckConfigurationStore.get_deck_configuration() method needs to be async, and it also needs to use the same defaults that .get() uses.
@SyntaxColoring SyntaxColoring marked this pull request as ready for review November 17, 2023 18:13
@SyntaxColoring SyntaxColoring requested review from a team as code owners November 17, 2023 18:13
Copy link
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

Changes all sensible - ran end to end on RSS Flex and successfully returned a default Deck value on GET all, was overwritten on a PUT call, and maintained through subsequent JSON Protocol runs during that session referencing non-default areas on the deck.

@SyntaxColoring SyntaxColoring merged commit 103ed71 into edge Nov 17, 2023
36 of 37 checks passed
@SyntaxColoring SyntaxColoring deleted the default_deck_configs branch November 17, 2023 19:41
@rickwierenga

This comment was marked as off-topic.

@SyntaxColoring

This comment was marked as off-topic.

@rickwierenga

This comment was marked as off-topic.

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.

3 participants