feat(shared-data): add robot definitions #11428

sfoster1 · 2022-09-02T16:37:48Z

Add some quick and small json files that can represent kinds of robot,
primarily as a thing for other files to reference.

Closes RCORE-126

Add some quick and small json files that can represent kinds of robot, primarily as a thing for other files to reference.

codecov · 2022-09-02T16:42:28Z

Codecov Report

Merging #11428 (1c45153) into edge (220c15c) will decrease coverage by 0.02%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##             edge   #11428      +/-   ##
==========================================
- Coverage   74.40%   74.38%   -0.03%     
==========================================
  Files        2049     2069      +20     
  Lines       56732    57260     +528     
  Branches     5420     5537     +117     
==========================================
+ Hits        42214    42591     +377     
- Misses      13270    13411     +141     
- Partials     1248     1258      +10

Flag	Coverage Δ
app	`74.61% <ø> (-0.06%)`	⬇️
protocol-designer	`45.81% <ø> (ø)`
shared-data	`86.44% <95.45%> (-7.99%)`	⬇️
step-generation	`88.39% <ø> (ø)`

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

Impacted Files	Coverage Δ
...ata/python/opentrons_shared_data/robot/__init__.py	`92.30% <92.30%> (ø)`
...ta/python/opentrons_shared_data/robot/dev_types.py	`100.00% <100.00%> (ø)`
app/src/pages/AppSettings/GeneralSettings.tsx	`51.85% <0.00%> (-6.05%)`	⬇️
shared-data/js/schema.ts	`100.00% <0.00%> (ø)`
shared-data/js/cypressUtils.ts	`0.00% <0.00%> (ø)`
shared-data/js/modules.ts	`68.42% <0.00%> (ø)`
shared-data/js/helpers/getVectorDifference.ts	`100.00% <0.00%> (ø)`
shared-data/js/labwareTools/index.ts	`95.34% <0.00%> (ø)`
shared-data/js/pipettes.ts	`72.22% <0.00%> (ø)`
shared-data/js/getLabware.ts	`0.00% <0.00%> (ø)`
... and 11 more

mcous

Without anything actually using these configs, I'm having a little bit of trouble understanding the intent behind the various fields. Some thoughts, though

mcous · 2022-09-02T21:38:43Z

shared-data/python/opentrons_shared_data/robot/dev_types.py

+RobotId = Literal["ot-2", "ot-3"]
+
+
+class RobotDefinition(TypedDict):


Would you be opposed to not using TypedDict for these? They're fairly annoying to work with as a caller

What should we use instead? I really like the pattern of having this module provide a descriptive type for the result of json.load(), which is a typed dict.

mcous · 2022-09-02T21:38:59Z

shared-data/python/opentrons_shared_data/robot/dev_types.py

+    """A python version of the robot definition type."""
+
+    otId: str
+    friendlyName: str


displayName, to match other resources?

mcous · 2022-09-02T21:40:02Z

shared-data/python/opentrons_shared_data/robot/dev_types.py

+
+RobotSchemaVersion1 = Literal[1]
+
+RobotSchema = NewType("RobotSchema", Dict[str, Any])


I don't see this used anywhere

it's not right now, but then again none of this is - just seemed like a reasonable thing to provide

mcous · 2022-09-02T21:41:07Z

shared-data/python/opentrons_shared_data/robot/dev_types.py

+
+RobotSchema = NewType("RobotSchema", Dict[str, Any])
+
+RobotName = Literal["OT-2 Standard", "OT-3 Standard"]


Similar comment as the TypeDict one: these type literals are annoying to use as a caller. I see a lot of use with Literal for doing stuff like @overload, but here, and Enum feels more appropriate

I really hate the use of Enum(str) as synonymous with string, but I guess I could use that instead

mcous · 2022-09-02T21:43:56Z

shared-data/robot/schemas/1.json

+      "type": "#/definitions/robotId"
+    },
+    "otId": {
+      "description": "A UUID for the machine type. Intended for use as a reference in other documents.",


Both the field name otId and the value type of UUID seems a bit weird for the purposes of "identify the machine type." Could we use a part number instead?

I think we should get rid of it - I added it by analogy to

opentrons/shared-data/deck/schemas/3.json

Lines 67 to 71 in 2041697

"properties": {

"otId": {

"description": "Unique internal ID generated using UUID",

"type": "string"

},

but I'm not sure why that's there either

mcous · 2022-09-02T21:45:08Z

shared-data/js/types.ts

+  robotId: RobotId
+  otId: string


I don't quite understand the different between these two things

robotId is for a future interface that would have shorter, nicer, machine-readable machine identification keys. otId is a uuid I added because other stuff seemed to but I would happily get rid of it.

mcous · 2022-09-02T21:45:21Z

shared-data/js/types.ts

+  friendlyName: string
+  robotName: RobotName


I also don't really understand the difference between these two things

this and the IDs are a consequence of the only actual backward compatibility concerns. Robots in the field use "OT-2 Standard" as their robot type in their health responses (and edge uses this as mdns txt records), and that value is used to discriminate between robots. That makes it a machine-readable value, if a little gross and overspecific. That's the robotName.

It also seems like a good idea to have a friendlyName, or displayName or whatever, specified separately for use in UIs. For now, it just happens that this is also OT-2 Standard, but in theory it might not be.

sfoster1

@mcous added some inline responses, but I have two bigger questions -

You have some notes about some fields, either their values or names; those notes are mostly on the js and python type bindings. These should change in the schema if we change them, right?
In general, what I've tried to do with the python and js type bindings here - as we've done with other modules in shared_data - is to provide descriptive types for the result of loading the json into the language, hence the use of TypedDict and Literal. I'd kind of like to keep them as this kind of descriptive thing rather than deserializing to dataclass and enum.

sfoster1 · 2022-09-06T13:53:09Z

shared-data/python/opentrons_shared_data/robot/dev_types.py

+
+RobotSchema = NewType("RobotSchema", Dict[str, Any])
+
+RobotName = Literal["OT-2 Standard", "OT-3 Standard"]


I really hate the use of Enum(str) as synonymous with string, but I guess I could use that instead

sfoster1 · 2022-09-06T13:53:31Z

shared-data/python/opentrons_shared_data/robot/dev_types.py

+
+RobotSchemaVersion1 = Literal[1]
+
+RobotSchema = NewType("RobotSchema", Dict[str, Any])


it's not right now, but then again none of this is - just seemed like a reasonable thing to provide

sfoster1 · 2022-09-06T13:55:26Z

shared-data/js/types.ts

+  friendlyName: string
+  robotName: RobotName


this and the IDs are a consequence of the only actual backward compatibility concerns. Robots in the field use "OT-2 Standard" as their robot type in their health responses (and edge uses this as mdns txt records), and that value is used to discriminate between robots. That makes it a machine-readable value, if a little gross and overspecific. That's the robotName.

It also seems like a good idea to have a friendlyName, or displayName or whatever, specified separately for use in UIs. For now, it just happens that this is also OT-2 Standard, but in theory it might not be.

sfoster1 · 2022-09-06T13:56:11Z

shared-data/js/types.ts

+  robotId: RobotId
+  otId: string


robotId is for a future interface that would have shorter, nicer, machine-readable machine identification keys. otId is a uuid I added because other stuff seemed to but I would happily get rid of it.

sfoster1 · 2022-09-06T13:56:46Z

shared-data/python/opentrons_shared_data/robot/dev_types.py

+RobotId = Literal["ot-2", "ot-3"]
+
+
+class RobotDefinition(TypedDict):


What should we use instead? I really like the pattern of having this module provide a descriptive type for the result of json.load(), which is a typed dict.

sfoster1 · 2022-09-06T13:57:45Z

shared-data/robot/schemas/1.json

+      "type": "#/definitions/robotId"
+    },
+    "otId": {
+      "description": "A UUID for the machine type. Intended for use as a reference in other documents.",


I think we should get rid of it - I added it by analogy to

opentrons/shared-data/deck/schemas/3.json

Lines 67 to 71 in 2041697

"properties": {

"otId": {

"description": "Unique internal ID generated using UUID",

"type": "string"

},

but I'm not sure why that's there either

mcous

Talked through this with @sfoster1 and @shlokamin. This PR is good to move forward, and in the longer term, our plan is to reduce our reliance on the shared-data package for this sort of information, which is more appropriate as robot-side implementation details.

The world we'd like to be for implementation details like robot definitions and pipette definitions involves one-way data flows from the robot (or API code) to the client, with the device being the singular source of truth rather than shared-data as a side channel that is assumed to be in sync between the two systems. Will get started on a proposal for that in the mean time

Add some quick and small json files that can represent kinds of robot, primarily as a thing for other files to reference. Closes RCORE-126

feat(shared-data): add robot definitions

7ddaded

Add some quick and small json files that can represent kinds of robot, primarily as a thing for other files to reference.

sfoster1 requested a review from shlokamin September 2, 2022 16:37

sfoster1 requested review from a team as code owners September 2, 2022 16:37

ahiuchingau approved these changes Sep 2, 2022

View reviewed changes

mcous reviewed Sep 2, 2022

View reviewed changes

sfoster1 commented Sep 6, 2022

View reviewed changes

remove unused data, rename elements

1c45153

mcous approved these changes Sep 6, 2022

View reviewed changes

sfoster1 merged commit 7e3f1e6 into edge Sep 6, 2022

sfoster1 deleted the RCORE-126-add-robot-definitions branch September 6, 2022 16:56

sfoster1 added a commit that referenced this pull request Oct 21, 2022

feat(shared-data): add robot definitions (#11428)

f0f3482

Add some quick and small json files that can represent kinds of robot, primarily as a thing for other files to reference. Closes RCORE-126

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(shared-data): add robot definitions #11428

feat(shared-data): add robot definitions #11428

sfoster1 commented Sep 2, 2022

codecov bot commented Sep 2, 2022 •

edited

Loading

mcous left a comment

mcous Sep 2, 2022

sfoster1 Sep 6, 2022

mcous Sep 2, 2022

mcous Sep 2, 2022

sfoster1 Sep 6, 2022

mcous Sep 2, 2022

sfoster1 Sep 6, 2022

mcous Sep 2, 2022

sfoster1 Sep 6, 2022

mcous Sep 2, 2022

sfoster1 Sep 6, 2022

mcous Sep 2, 2022

sfoster1 Sep 6, 2022

sfoster1 left a comment

sfoster1 Sep 6, 2022

sfoster1 Sep 6, 2022

sfoster1 Sep 6, 2022

sfoster1 Sep 6, 2022

sfoster1 Sep 6, 2022

sfoster1 Sep 6, 2022

mcous left a comment

		RobotId = Literal["ot-2", "ot-3"]


		class RobotDefinition(TypedDict):


		RobotSchemaVersion1 = Literal[1]

		RobotSchema = NewType("RobotSchema", Dict[str, Any])


		RobotSchema = NewType("RobotSchema", Dict[str, Any])

		RobotName = Literal["OT-2 Standard", "OT-3 Standard"]

	"properties": {
	"otId": {
	"description": "Unique internal ID generated using UUID",
	"type": "string"
	},

feat(shared-data): add robot definitions #11428

feat(shared-data): add robot definitions #11428

Conversation

sfoster1 commented Sep 2, 2022

codecov bot commented Sep 2, 2022 • edited Loading

Codecov Report

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2022 •

edited

Loading