feat(shared-data): add in-schema versions to liquid class schema and definitions#18401
feat(shared-data): add in-schema versions to liquid class schema and definitions#18401
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18401 +/- ##
==========================================
- Coverage 57.79% 57.75% -0.04%
==========================================
Files 3252 3252
Lines 276195 276814 +619
Branches 32191 32479 +288
==========================================
+ Hits 159614 159885 +271
- Misses 116388 116736 +348
Partials 193 193
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ncdiehl11
left a comment
There was a problem hiding this comment.
shared-data/js changes look good
jerader
left a comment
There was a problem hiding this comment.
changes make sense to me
| ) | ||
| version: int = Field( | ||
| ..., description="Version of the liquid class within the schema" | ||
| ) |
There was a problem hiding this comment.
Ah. So the version numbers will start over from 1 if we ever change the schemaVersion?
There was a problem hiding this comment.
FYI we're not doing it that way for labware definitions. We're doing it like:
- Schema 2
- Labware XYZ version 1
- Labware XYZ version 2
- Schema 3
- Labware XYZ version 3
- Labware XYZ version 4
Which I'm pretty sure is what we want. Otherwise it's ambiguous what "labware XYZ version 1" refers to, among other problems.
There was a problem hiding this comment.
Noted, I can update the doc string so it doesn't imply otherwise
| minimal_liquid_class_def2 | ||
| ) | ||
| assert subject.define_liquid_class("water") == expected_liquid_class | ||
| assert subject.define_liquid_class("water", version=456) == expected_liquid_class |
There was a problem hiding this comment.
Wait, why do we want this?
If the user explicitly wants to load version=456 after loading version=123, are you saying we have to return version=123 no matter what?
There was a problem hiding this comment.
Ooh I did not look closely enough at this test when I was fixing it. The way I have it now is that if you load say version 1 of a liquid class, and then try loading version 2, we will return version 1 because that's what we have cached. This is obviously not ideal, so I will fix that
…quid_class_schema_add_version
|
@jbleon95 , This isn't inclusive of "no liquid class selected" definitions in PD as well as OT-2 work, right? |
| liquid_class_def = liquid_classes.load_definition(name) | ||
| self._defined_liquid_class_defs_by_name[name] = liquid_class_def | ||
| liquid_class_def = liquid_classes.load_definition(name, version=version) | ||
| self._defined_liquid_class_defs_by_name_and_version[ |
There was a problem hiding this comment.
Nitpick: This is a very descriptive name :) but its length is making the rest of the code hard to read. Would it bad to call it something like self._liquid_class_cache instead?
Oh, one more thing to look into: If you want to be really fancy, I think there are some built-in Python decorators for memo-ization, so that you don't have to write this code yourself.
Overview
Closes AUTH-1806
This PR adds a
versionfield to the liquid class schema and the three existing liquid class definitions (plus test fixtures). This also has the side effect of changing the folder structure to match labware definitions, where now each liquid class definition is defined under the/liquid-class/definitions/1/folder asname/version.jsoninstead ofname.json.In addition to this, a
versionargument was added todefine_liquid_class. This currently defaults at 1, but in the future if and when we have additional liquid class definition versions it should automatically load the latest one for that API version.Test Plan and Hands on Testing
Unit and integration tests should catch any breaking changes.
Changelog
versionfield to liquid class definitionsversionargument todefine_liquid_classReview requests
Risk assessment
Low