feat(shared-data): properly load new pipette configurations from shared data #11795
Conversation
…ed data We have a completely new shape for pipette configurations that need to be loaded in.
Codecov Report
@@ Coverage Diff @@
## RLIQ-131-support-96-channel #11795 +/- ##
===============================================================
+ Coverage 74.14% 74.19% +0.04%
===============================================================
Files 2106 2108 +2
Lines 58491 58603 +112
Branches 6218 6218
===============================================================
+ Hits 43371 43483 +112
Misses 13654 13654
Partials 1466 1466
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
|
||
| @dataclass(frozen=True) | ||
| class OT2PipetteConfigurations(SharedPipetteConfigurations): | ||
| tip_length: float # TODO(seth): remove |
There was a problem hiding this comment.
hey wait why are you dragging me into this?!?!?!
There was a problem hiding this comment.
LOL the comment is still relevant!
|
We tend to stick to using dataclasses (and pythonic snake case) in the hardware controller + objects used in the hardware controller. That's the main reason I'm loading in the pydantic model and then reformatting to a dataclass. |
| from typing_extensions import Literal | ||
|
|
||
| PLUNGER_CURRENT_MINIMUM = 0.1 | ||
| PLUNGER_CURRENT_MAXIMUM = 1.5 | ||
|
|
||
|
|
||
| PipetteModelMajorVersion = Literal[1] | ||
| PipetteModelMinorVersion = Literal[0, 1, 2, 3] |
There was a problem hiding this comment.
I've personally soured on the idea of a constants.py / constants.ts file in modules; it just makes things harder to find IMO. Could any of these things move elsewhere?
There was a problem hiding this comment.
I can move it into the pipette_definition file for now. I had created this file with the intention of sticking random things like 'quirks' and 'mutable_configs' in here that are only relevant for OT2 pipettes. I could probably call that file a different name at that point in time.
| load_shared_data("pipette/definitions/2/general/single_channel/p50/1_0.json") | ||
| ) | ||
|
|
||
| assert pipette_config.channels.as_int == general["channels"] |
There was a problem hiding this comment.
I think calling load_shared_data inside the test muddies the test. I think we should either have:
- An integration test, that knows nothing about how the pipette configuration files are stored
- Value of this test: test that the unit probably works
- Non-value: this test would provide no design feedback to unit internals
- How to get there: replace the expected values with hardcoded literals in the test
- A collaborator test:
- Value of this test: design feedback for unit internals and that the unit is wired up to other units as we expect
- Non-value: this test would not validate that the unit works, due to the use of mocks
- How to get there: stub out
load_shared_data, check that it's called with the correct path
What's currently written tries to split the difference between (1) and (2), which ends up compromising both and causing other problems (like tests failing for unclear reasons if/when we change things).
Given this unit's role, I would recommend option (1)
Overview
We have a completely new shape for pipette configurations that need to be loaded in. Loading in major and minor versions for pipettes will be addressed in a fast follow-up (and also changes required for the xfail test here). I wanted to avoid making the diff confusing by making changes to the json again.
The hope for this refactor is that
ot3_pipette_config.pywill replacepipette_config.pyonce we can support OT2 pipettes in this new configuration format.Changelog
OT3PipetteConfigurationmodel.To-Do in follow-ups
Review requests
Does this look OK? Missing doc strings/tests etc?
Risk assessment
Low. Still not being used in the hardware controller yet. That is for a follow-up PR.