fix(shared-data): find latest definition if version not specified #17223
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
caila-marashaj
force-pushed
the
load-newest-available-labware-def
branch
from
a275f47 to
e3345df
Compare
caila-marashaj
force-pushed
the
load-newest-available-labware-def
branch
from
e3345df to
884f185
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #17223 +/- ##
==========================================
+ Coverage 73.84% 79.04% +5.20%
==========================================
Files 43 120 +77
Lines 3303 4586 +1283
==========================================
+ Hits 2439 3625 +1186
- Misses 864 961 +97
Flags with carried forward coverage won't be shown. Click here to find out more. |
| schema_3_dir = get_shared_data_root() / STANDARD_DEFS_PATH / "3" / load_name | ||
| if schema_3_dir.exists(): | ||
| schema_3_files = os.listdir(schema_3_dir) | ||
| version_filename = f"{version}.json" if version else max(schema_3_files) |
There was a problem hiding this comment.
Just a couple of minor code-level nitpicks:
- As a general habit in Python, I think we should favor writing
if versionasif version is not None. Becauseversionis an int, soif versionwould behave unexpectedly if somebody suppliedversion=0. Granted, I don't think any labware will actually have a version 0, so it probably doesn't matter here, but, habits. max(schema_3_files)would accidentally favor2.jsonover10.json(it will return the lexicographic maximum, not the numeric maximum). I'd also worry about it accidentally picking up things like.gitignoreand.DS_Store. If we want to find the highest-numbered JSON file, I think we should filter the filenames by regex and extract properly comparableints from them.
There was a problem hiding this comment.
Could you elaborate a bit on the context of this change and its intended effect?
Right now:
- For
apiLevel≥2.14 Python protocols, I don't think this will change anything, because of this higher-up code that setsversionexplicitly. - For
apiLevel≤2.13 Python protocols, because of this code, I think it will change their behavior from defaulting to labware version 1, to defaulting to the highest labware version, but only if the labware happens to have a schema 3 definition. (Example reproduction case.) That is probably not what we want, is it? I wouldn't think we want those protocols to be affected at all.
In my opinion:
- In terms of overall system behavior, we want the default labware version to be a higher-level concern, so it can change depending on higher-level concepts such as the protocol's
apiLevel. Here's my thinking on this from a while ago. - In terms of code implementation, a good way to organize that would be:
- These functions primarily take fully-resolved, explicit
namespace+load_name+versionparams, never assuming any default. Given anamespace+load_name+version, there should only be one file that matches, and that's something that we should enforce in theshared-datatests if we're not already. - Python protocols should automatically select a
version, depending on theirapiLevel. That code lives in load_labware_params.py. I do not think it's a good idea for Python protocols to automatically find the highest version of a labware, again for the reasons described here, but if we want to do it anyway, I still think the code should live in load_labware_params.py.
- These functions primarily take fully-resolved, explicit
Comment on lines
-241
to
+242
|
|
||
| namespace = namespace.lower() | ||
| def_path = _get_path_to_labware(load_name, namespace, checked_version) | ||
| def_path = _get_path_to_labware(load_name, namespace, version) |
There was a problem hiding this comment.
Hang on, there are multiple places where checked_version was used in this function, and only this one is being changed to version. That means the error messages are out of sync with the version that was actually read, for one thing. But more seriously, it looks like we now have different version-defaulting behavior depending on exactly how the namespace was provided (see line 233). Is that intentional?
Laura-Danielle
pushed a commit
that referenced
this pull request
Jan 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
When grabbing a labware definition, if a version is not specified, we default the file version to
1.jsonand check whether that file exists. This was done under the impression that all labware definitions in schema 3 would start at1.json, but since that isn't the case, if no version is specified, we should just find the latest available labware def version that exists and load that.