-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(api): properly handle non-labware jsons in sim #17198
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,7 +4,7 @@ | |||||
import json | ||||||
import os | ||||||
from pathlib import Path | ||||||
from typing import Any, AnyStr, Dict, Optional, Union, List | ||||||
from typing import Any, AnyStr, Dict, Optional, Union, List, Sequence, Literal | ||||||
|
||||||
import jsonschema # type: ignore | ||||||
|
||||||
|
@@ -17,10 +17,30 @@ | |||||
USER_DEFS_PATH, | ||||||
) | ||||||
from opentrons_shared_data.labware.types import LabwareDefinition | ||||||
from opentrons_shared_data.errors.exceptions import InvalidProtocolData | ||||||
|
||||||
|
||||||
MODULE_LOG = logging.getLogger(__name__) | ||||||
|
||||||
LabwareProblem = Literal[ | ||||||
"no-schema-id", "bad-schema-id", "schema-mismatch", "invalid-json" | ||||||
] | ||||||
|
||||||
|
||||||
class NotALabwareError(InvalidProtocolData): | ||||||
def __init__( | ||||||
self, problem: LabwareProblem, wrapping: Sequence[BaseException] | ||||||
) -> None: | ||||||
messages: dict[LabwareProblem, str] = { | ||||||
"no-schema-id": "No schema ID present in file", | ||||||
"bad-schema-id": "Bad schema ID in file", | ||||||
"invalid-json": "File does not contain valid JSON", | ||||||
"schema-mismatch": "File does not match labware schema", | ||||||
} | ||||||
super().__init__( | ||||||
message=messages[problem], detail={"kind": problem}, wrapping=wrapping | ||||||
) | ||||||
|
||||||
|
||||||
def get_labware_definition( | ||||||
load_name: str, | ||||||
|
@@ -126,7 +146,7 @@ def save_definition( | |||||
json.dump(labware_def, f) | ||||||
|
||||||
|
||||||
def verify_definition( | ||||||
def verify_definition( # noqa: C901 | ||||||
contents: Union[AnyStr, LabwareDefinition, Dict[str, Any]] | ||||||
) -> LabwareDefinition: | ||||||
"""Verify that an input string is a labware definition and return it. | ||||||
|
@@ -146,15 +166,24 @@ def verify_definition( | |||||
if isinstance(contents, dict): | ||||||
to_return = contents | ||||||
else: | ||||||
to_return = json.loads(contents) | ||||||
try: | ||||||
to_return = json.loads(contents) | ||||||
except json.JSONDecodeError as e: | ||||||
raise NotALabwareError("invalid-json", [e]) | ||||||
try: | ||||||
schema_version = to_return["schemaVersion"] | ||||||
except KeyError as e: | ||||||
raise NotALabwareError("no-schema-id", [e]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
try: | ||||||
schema = schemata_by_version[schema_version] | ||||||
except KeyError: | ||||||
raise RuntimeError( | ||||||
f'Invalid or unknown labware schema version {to_return.get("schemaVersion", None)}' | ||||||
) | ||||||
jsonschema.validate(to_return, schema) | ||||||
except KeyError as e: | ||||||
raise NotALabwareError("bad-schema-id", [e]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
try: | ||||||
jsonschema.validate(to_return, schema) | ||||||
except jsonschema.ValidationError as e: | ||||||
raise NotALabwareError("schema-mismatch", [e]) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
# we can type ignore this because if it passes the jsonschema it has | ||||||
# the correct structure | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,6 @@ | |
from dataclasses import dataclass | ||
import json | ||
import logging | ||
from json import JSONDecodeError | ||
import pathlib | ||
import subprocess | ||
import sys | ||
|
@@ -21,8 +20,6 @@ | |
TYPE_CHECKING, | ||
) | ||
|
||
from jsonschema import ValidationError # type: ignore | ||
|
||
from opentrons.calibration_storage.deck_configuration import ( | ||
deserialize_deck_configuration, | ||
) | ||
|
@@ -32,7 +29,7 @@ | |
JUPYTER_NOTEBOOK_LABWARE_DIR, | ||
SystemArchitecture, | ||
) | ||
from opentrons.protocol_api import labware | ||
from opentrons.protocols import labware | ||
from opentrons.calibration_storage import helpers | ||
from opentrons.protocol_engine.errors.error_occurrence import ( | ||
ErrorOccurrence as ProtocolEngineErrorOccurrence, | ||
|
@@ -79,7 +76,7 @@ def labware_from_paths( | |
if child.is_file() and child.suffix.endswith("json"): | ||
try: | ||
defn = labware.verify_definition(child.read_bytes()) | ||
except (ValidationError, JSONDecodeError): | ||
except labware.NotALabwareError: | ||
Comment on lines
-82
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Much better, thank you. |
||
log.info(f"{child}: invalid labware, ignoring") | ||
log.debug( | ||
f"{child}: labware invalid because of this exception.", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
"""Exception hierarchy for error codes.""" | ||
|
||
from typing import Dict, Any, Optional, List, Iterator, Union, Sequence, overload | ||
from logging import getLogger | ||
from traceback import format_exception_only, format_tb | ||
|
@@ -1099,7 +1100,7 @@ def __init__( | |
self, | ||
message: Optional[str] = None, | ||
detail: Optional[Dict[str, str]] = None, | ||
wrapping: Optional[Sequence[EnumeratedError]] = None, | ||
wrapping: Optional[Sequence[Union[EnumeratedError, BaseException]]] = None, | ||
Comment on lines
-1102
to
+1103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not for this PR, but I think we're losing the plot on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it has always included those actually, we've just been uneven about propagating the type signature down to things that inherit from the general exception. and while it does do basically the same thing as cause and context, it also represents a specific point at which we leave native exception land and transition into ours. |
||
) -> None: | ||
"""Build an InvalidProtocolData.""" | ||
super().__init__(ErrorCodes.INVALID_PROTOCOL_DATA, message, detail, wrapping) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
from e
so the tracebacks reflect that we're deliberately converting exceptionA
to exceptionB
, instead of encountering an unexpected exceptionB
while we're trying to handle exceptionA
.from None
instead offrom e
would also make sense here. Just a matter of taste I think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh whoops yeah good call