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

fix(robot-server): Fix 500 error trying to start tip length calibration sessions #13607

Merged
merged 6 commits into from
Sep 20, 2023

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Sep 20, 2023

Overview

Closes RSS-349.

Test Plan

  • On an OT-2, follow the reproduction steps in RSS-349 and make sure you can complete tip length calibration now.

Fix details

According to our labware definition JSON schema, these are the valid shapes of gripperOffsets:

  • Totally omitted:

    <gripperOffsets omitted>
    
  • Containing just default:

    {
        "default": {
            "pickUpOffset": {"x": 1, "y": 2, "z": 3}
            "dropOffset": {"x": 1, "y": 2, "z": 3}
        }
    }
  • Containing default and slot-specific offsets:

    {
        "default": {
            "pickUpOffset": {"x": 1, "y": 2, "z": 3}
            "dropOffset": {"x": 1, "y": 2, "z": 3}
        }
        "A1": {
            "pickUpOffset": {"x": 1, "y": 2, "z": 3}
            "dropOffset": {"x": 1, "y": 2, "z": 3}
        }
    }

The problem was that this line in our Pydantic model did not match that.

It would cause the server and local analysis engine to return labware definitions where gripperOffsets was normalized to {}. This was invalid according to our JSON schema, which required gripperOffsets to either contain at least a "default" key, or be totally omitted.

Those incorrect {}s are all over the database by now, so we unfortunately can't correct the Pydantic model to match the JSON schema. Instead, we need to loosen the JSON schema to accept what the Pydantic model has been emitting.

So, we're loosening the JSON schema to now accept these additional shapes:

  • Containing nothing:

    { }
  • Containing just slot-specific offsets (this is an accidental byproduct of this implementation, but we believe it to be harmless)

    {
        "A1": {
            "pickUpOffset": {"x": 1, "y": 2, "z": 3}
            "dropOffset": {"x": 1, "y": 2, "z": 3}
        }
    }

Review requests

Is this the best way to fix this?

Does it look like our Pydantic model now conforms to our JSON schema?

Risk assessment

Low I guess? I tried not to touch any of the logic for deciding gripper offsets.

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #13607 (e3ca563) into chore_release-7.0.0 (ae74b67) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                  @@
##           chore_release-7.0.0   #13607   +/-   ##
====================================================
  Coverage                71.33%   71.33%           
====================================================
  Files                     2423     2423           
  Lines                    68162    68162           
  Branches                  7937     7937           
====================================================
  Hits                     48620    48620           
  Misses                   17687    17687           
  Partials                  1855     1855           
Flag Coverage Δ
app 68.99% <ø> (ø)
g-code-testing 96.44% <ø> (ø)
labware-library 49.17% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 46.09% <ø> (ø)
shared-data 74.33% <ø> (ø)
step-generation 86.82% <ø> (ø)

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

Files Changed Coverage Δ
api/src/opentrons/protocol_engine/state/labware.py 100.00% <ø> (ø)

@SyntaxColoring SyntaxColoring marked this pull request as ready for review September 20, 2023 17:38
@SyntaxColoring SyntaxColoring requested review from a team as code owners September 20, 2023 17:38
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Looks great! I like the integration test idea too

@@ -1,3 +1,9 @@
"""Helper functions for use inside Tavern tests.

https://tavern.readthedocs.io/en/latest/basics.html#calling-external-functions
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty cool!

@SyntaxColoring SyntaxColoring merged commit eacc4fe into chore_release-7.0.0 Sep 20, 2023
@SyntaxColoring SyntaxColoring deleted the RSS-349 branch September 20, 2023 19:10
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