chore(shared-data): create liquid class schema v1 and fixture

[ncdiehl11]

Overview

This PR introduces the first version of an Opentrons liquid class schema. Single liquid class definitions will adhere to this schema and be keyed by pipette and tip type.

Closes AUTH-832

Test Plan and Hands on Testing

• Look through schema and ensure it adheres to our finalized liquid class testing matrix. Any feedback on schema structure/style is appreciated.
• Try out some test data in a JSON validator. I created a base fixture that passes schema validation. We can modify that for testing.

Changelog

• create liquid class schema v1
• add fixture

Review requests

See test plan. Should we leave keys for patternProperties pipette and tip type less restrictive strings? Perhaps any safeString rather than following more stringent regex?

Risk assessment

low

[sfoster1]

I'm a little concerned with the regexes that you specify for tips and for pipettes. Because these are keys, they're tough to document; and because we want to keep this open for more pipettes and tips, it's tough to make a properly restrictive and self-documenting regex (insofar as regexes can ever be self-documenting).

What about instead having these levels be arrays, something like

"properties: {
    "liquidName": ...,
    ...,
    "byPipette": {
        "type": "array",
        "description": "The settings for each kind of pipette for this liquid",
       "items": {
             "type": "object",
             "description": "The settings for a specific kind of pipette when handling this liquid",
             "properties": {
                   "pipetteModel": {
                         "type": "string",
                          "description": "The pipette model this applies to",
                          // free to have an enum or a pattern or just a string here
                    },
                    "byTipClass": {
                           "description": "Settings for each kind of tip this pipette can use",
                           "type": "array",
                           "items": {
                                "type": "object",
                                "properties": {
                                     "tipClass": {
                                          // again, free to have different enums or patterns or whatever
                                     },
                                     ...
                                }
                           }
                    }
             }
        }
    }

[sfoster1]

I see where you're going with the patternProperties here but I think it's going to be tough to come up with an appropriate regex and I'm not sure this is usefully it.

[ncdiehl11]

Yeah, that's a good callout. I like the idea of nesting the properties in arrays of objects containing the specific pipette/tip properties. Just pushed a commit to that effect and made some other refactors for only requiring certain properties if enable is true.

[sfoster1]

same comment as with the pipette type

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.62%. Comparing base (a0f42de) to head (ea481a4).
Report is 23 commits behind head on edge.

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #16267   +/-   ##
=======================================
  Coverage   73.62%   73.62%           
=======================================
  Files          41       41           
  Lines        2992     2992           
=======================================
  Hits         2203     2203           
  Misses        789      789           

Flag	Coverage Δ
shared-data	73.62% <ø> (ø)

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

[sfoster1]

Looks great to me! Sorry it took me a while to re-review.

[sanni-t]

This is awesome @ncdiehl! Just a couple of nitpicks.

We also need to add volume corrections but we can add those fields later

[sanni-t]

This works, but I think a more concise way of writing the required fields would be something like this-

"touchTip": {
      "type": "object",
      "description": "Shared properties for the touch-tip function.",
      "properties": {
        "enable": {
          "type": "boolean",
          "description": "Whether touch-tip is enabled."
        },
        "zOffset": {
          "type": "number",
          "description": "Offset from the top of the well for touch-tip, in millimeters."
        },
        "mmToEdge": {
          "type": "number",
          "description": "Offset away from the the well edge, in millimeters."
        },
        "speed": {
          "$ref": "#/definitions/positiveNumber",
          "description": "Touch-tip speed, in millimeters per second."
        }
      },
      "required": ["enable"],
      "if": {
        "properties": {
          "enable": { "const": true }
        }
      },
      "then": {
        "required": ["zOffset", "mmToEdge", "speed"]
      }
    },

i.e., enable is required always. And if enable is true, require the additional properties

Same for all the other properties that have gating behind the enable key.

[jbleon95]

I'm going to go further than this and ask if there's a reason why we don't just get rid of enable and make the the availability of a particular liquid class property depend on it being defined for a particular tip type, i.e. if it's there it's enabled and if it's not defined it's not enabled. This would simplify the Python models and we could always use known defaults if a user wishes to enable a default disabled property.

[sanni-t]

This would simplify the Python models and we could always use known defaults if a user wishes to enable a default disabled property.

@jbleon95 while I agree that not having enable/disable makes the python models as well as this schema much more compact, using some known defaults if a user enables a property whose sub-properties aren't defined isn't a great solution. It would lead to confusion and uncertainty about what the defaults are for each of the enable-gated properties.

That said, maybe there's a middle ground. The schema doesn't include whether to enable or disable those properties but you specify it when using the transfer function. So essentially it'll behave the same way you described, except, the liquid class definition will contain definitions of all properties & sub-properties so that when a transfer function wants to use it, it uses the values from liquid class's definition instead of some defaults.

[sanni-t]

Can we add a more elaborate description like you've used for submerge?

[sanni-t]

Same comment as mix description

[sanni-t]

Some nitpicks but mainly want to make sure that the 'by volume' properties are as discussed. We want them to be objects where keys are volumes. So something like-

xyzByVolume: {
    "default": 123.
    "100": 1.2,
    .
    .
}

"default" will be required for all.

The schema for it would be something like-

"airGapByVolume": {
      "type": "object",
      "description": "Settings for air gap keyed by target aspiration volume.",
      "items": {
        "$comment": "Other keys in here should be volume in uL",
        "properties": {
           "default": { 
              "$ref": "#/definitions/positiveNumber"}
        },
        "required": ["default"],
        "additionalProperties": { "$ref": "#/definitions/positiveNumber", }
      }
    },

You can even assign pattern properties to the additional keys I think. So they'll always be strings of numbers

[sanni-t]

Changes look good!! Thanks!!