refactor(api, robot-server): add error for multiple CSV definitions and less reliance on file reading/creating

api/src/opentrons/protocol_api/_parameter_context.py

@@ -15,6 +15,7 @@
 from opentrons.protocols.parameters.exceptions import (
     ParameterDefinitionError,
     ParameterValueError,
+    IncompatibleParameterError,
 )
 from opentrons.protocol_engine.types import (
     RunTimeParameter,
@@ -183,6 +184,14 @@ def add_csv_file(
             variable_name: The variable name the CSV parameter will be referred to in the run context.
             description: A description of the parameter as it will show up on the frontend.
         """
+        if any(
+            isinstance(parameter, csv_parameter_definition.CSVParameterDefinition)
+            for parameter in self._parameters.values()
+        ):
+            raise IncompatibleParameterError(
+                "Only one CSV File parameter can be defined per protocol."
+            )
+
         validation.validate_variable_name_unique(variable_name, set(self._parameters))
         parameter = csv_parameter_definition.create_csv_parameter(
             display_name=display_name,
@@ -246,8 +255,11 @@ def initialize_csv_files(
             file_id = file_path.parent.name
             file_name = file_path.name
 
+            with file_path.open("rb") as fh:
+                contents = fh.read()
+
             parameter.file_info = FileInfo(id=file_id, name=file_name)
-            parameter.value = file_path
+            parameter.value = contents
 
     def export_parameters_for_analysis(self) -> List[RunTimeParameter]:
         """Exports all parameters into a protocol engine models for reporting in analysis.

api/src/opentrons/protocols/execution/execute.py

@@ -16,7 +16,6 @@
 from opentrons.protocols.api_support.types import APIVersion
 
 from opentrons.protocols.parameters.csv_parameter_interface import CSVParameter
-from opentrons.protocols.parameters.exceptions import RuntimeParameterRequired
 
 MODULE_LOG = logging.getLogger(__name__)
 
@@ -51,13 +50,8 @@ def run_protocol(
         finally:
             if protocol.api_level >= APIVersion(2, 18):
                 for parameter in context.params.get_all().values():
-                    if isinstance(parameter, CSVParameter):
-                        try:
-                            parameter.file.close()
-                        # This will be raised if the csv file wasn't set, which means it was never opened,
-                        # so we can safely skip this.
-                        except RuntimeParameterRequired:
-                            pass
+                    if isinstance(parameter, CSVParameter) and parameter.file_opened:
+                        parameter.file.close()
     else:
         if protocol.contents["schemaVersion"] == 3:
             ins = execute_json_v3.load_pipettes_from_json(context, protocol.contents)

api/src/opentrons/protocols/parameters/csv_parameter_definition.py

@@ -1,5 +1,4 @@
 """CSV Parameter definition and associated classes/functions."""
-from pathlib import Path
 from typing import Optional
 
 from opentrons.protocol_engine.types import (
@@ -14,7 +13,7 @@
 from .csv_parameter_interface import CSVParameter
 
 
-class CSVParameterDefinition(AbstractParameterDefinition[Optional[Path]]):
+class CSVParameterDefinition(AbstractParameterDefinition[Optional[bytes]]):
     """The definition for a user defined CSV file parameter."""
 
     def __init__(
@@ -30,7 +29,7 @@ def __init__(
         self._display_name = validation.ensure_display_name(display_name)
         self._variable_name = validation.ensure_variable_name(variable_name)
         self._description = validation.ensure_description(description)
-        self._value: Optional[Path] = None
+        self._value: Optional[bytes] = None
         self._file_info: Optional[FileInfo] = None
 
     @property
@@ -39,13 +38,13 @@ def variable_name(self) -> str:
         return self._variable_name
 
     @property
-    def value(self) -> Optional[Path]:
+    def value(self) -> Optional[bytes]:
         """The current set file for the CSV parameter. Defaults to None on definition creation."""
         return self._value
 
     @value.setter
-    def value(self, new_path: Path) -> None:
-        self._value = new_path
+    def value(self, contents: bytes) -> None:
+        self._value = contents
 
     @property
     def file_info(self) -> Optional[FileInfo]:
@@ -56,7 +55,7 @@ def file_info(self, file_info: FileInfo) -> None:
         self._file_info = file_info
 
     def as_csv_parameter_interface(self, api_version: APIVersion) -> CSVParameter:
-        return CSVParameter(csv_path=self._value, api_version=api_version)
+        return CSVParameter(contents=self._value, api_version=api_version)
 
     def as_protocol_engine_type(self) -> RunTimeParameter:
         """Returns CSV parameter as a Protocol Engine type to send to client."""

api/src/opentrons/protocols/parameters/csv_parameter_interface.py

@@ -1,35 +1,46 @@
 import csv
-from pathlib import Path
+from tempfile import NamedTemporaryFile
 from typing import Optional, TextIO, Any, List
 
 from opentrons.protocols.api_support.types import APIVersion
 
-from . import parameter_file_reader
-from .exceptions import ParameterValueError
+from .exceptions import ParameterValueError, RuntimeParameterRequired
 
 
 # TODO(jbl 2024-08-02) This is a public facing class and as such should be moved to the protocol_api folder
 class CSVParameter:
-    def __init__(self, csv_path: Optional[Path], api_version: APIVersion) -> None:
-        self._path = csv_path
-        self._file: Optional[TextIO] = None
-        self._contents: Optional[str] = None
+    def __init__(self, contents: Optional[bytes], api_version: APIVersion) -> None:
+        self._contents = contents
         self._api_version = api_version
+        self._file: Optional[TextIO] = None
 
     @property
     def file(self) -> TextIO:
         """Returns the file handler for the CSV file."""
         if self._file is None:
-            self._file = parameter_file_reader.open_file_path(self._path)
+            text = self.contents
+            temporary_file = NamedTemporaryFile("r+")
+            temporary_file.write(text)
+            temporary_file.flush()
+
+            # Open a new file handler for the temporary file with read-only permissions and close the other
+            self._file = open(temporary_file.name, "r")
+            temporary_file.close()
         return self._file
 
+    @property
+    def file_opened(self) -> bool:
+        """Return if a file handler has been opened for the CSV parameter."""
+        return self._file is not None
+
     @property
     def contents(self) -> str:
         """Returns the full contents of the CSV file as a single string."""
         if self._contents is None:
-            self.file.seek(0)
-            self._contents = self.file.read()
-        return self._contents
+            raise RuntimeParameterRequired(
+                "CSV parameter needs to be set to a file for full analysis or run."
+            )
+        return self._contents.decode("utf-8")
 
     def parse_as_csv(
         self, detect_dialect: bool = True, **kwargs: Any
@@ -42,23 +53,20 @@ def parse_as_csv(
         rows: List[List[str]] = []
         if detect_dialect:
             try:
-                self.file.seek(0)
-                dialect = csv.Sniffer().sniff(self.file.read(1024))
-                self.file.seek(0)
-                reader = csv.reader(self.file, dialect, **kwargs)
+                dialect = csv.Sniffer().sniff(self.contents[:1024])
+                reader = csv.reader(self.contents.split("\n"), dialect, **kwargs)
             except (UnicodeDecodeError, csv.Error):
                 raise ParameterValueError(
-                    "Cannot parse dialect or contents from provided CSV file."
+                    "Cannot parse dialect or contents from provided CSV contents."
                 )
         else:
             try:
-                reader = csv.reader(self.file, **kwargs)
+                reader = csv.reader(self.contents.split("\n"), **kwargs)
             except (UnicodeDecodeError, csv.Error):
-                raise ParameterValueError("Cannot parse provided CSV file.")
+                raise ParameterValueError("Cannot parse provided CSV contents.")
         try:
             for row in reader:
                 rows.append(row)
         except (UnicodeDecodeError, csv.Error):
-            raise ParameterValueError("Cannot parse provided CSV file.")
-        self.file.seek(0)
+            raise ParameterValueError("Cannot parse provided CSV contents.")
         return rows

api/src/opentrons/protocols/parameters/exceptions.py

@@ -28,3 +28,7 @@ class ParameterDefinitionError(ValueError):
 
 class ParameterNameError(ValueError):
     """An error raised when a parameter name or description is not valid."""
+
+
+class IncompatibleParameterError(ValueError):
+    """An error raised when a parameter conflicts with another parameter."""

api/src/opentrons/protocols/parameters/types.py

@@ -1,11 +1,10 @@
-from pathlib import Path
 from typing import TypeVar, Union, TypedDict
 
 from .csv_parameter_interface import CSVParameter
 
 
 PrimitiveAllowedTypes = Union[str, int, float, bool]
-AllAllowedTypes = Union[str, int, float, bool, Path, None]
+AllAllowedTypes = Union[str, int, float, bool, bytes, None]
 UserFacingTypes = Union[str, int, float, bool, CSVParameter]
 
 ParamType = TypeVar("ParamType", bound=AllAllowedTypes)

api/src/opentrons/protocols/parameters/validation.py

@@ -237,7 +237,7 @@ def validate_type(value: ParamType, parameter_type: type) -> None:
     """Validate parameter value is the correct type."""
     if not isinstance(value, parameter_type):
         raise ParameterValueError(
-            f"Parameter value {value} has type '{type(value).__name__}',"
+            f"Parameter value {value!r} has type '{type(value).__name__}',"
             f" but must be of type '{parameter_type.__name__}'."
         )
 
@@ -252,7 +252,7 @@ def validate_options(
     """Validate default values and all possible constraints for a valid parameter definition."""
     if not isinstance(default, parameter_type):
         raise ParameterValueError(
-            f"Parameter default {default} has type '{type(default).__name__}',"
+            f"Parameter default {default!r} has type '{type(default).__name__}',"
             f" but must be of type '{parameter_type.__name__}'."
         )
 

api/tests/opentrons/protocol_api/test_parameter_context.py

@@ -13,7 +13,10 @@
     parameter_definition as mock_parameter_definition,
     validation as mock_validation,
 )
-from opentrons.protocols.parameters.exceptions import ParameterDefinitionError
+from opentrons.protocols.parameters.exceptions import (
+    ParameterDefinitionError,
+    IncompatibleParameterError,
+)
 from opentrons.protocol_engine.types import BooleanParameter
 
 from opentrons.protocol_api._parameter_context import ParameterContext
@@ -196,7 +199,7 @@ def test_add_string(decoy: Decoy, subject: ParameterContext) -> None:
 def test_add_csv(decoy: Decoy, subject: ParameterContext) -> None:
     """It should create and add a CSV parameter definition."""
     subject._parameters["other_param"] = decoy.mock(
-        cls=mock_csv_parameter_definition.CSVParameterDefinition
+        cls=mock_parameter_definition.ParameterDefinition
     )
     param_def = decoy.mock(cls=mock_csv_parameter_definition.CSVParameterDefinition)
     decoy.when(param_def.variable_name).then_return("my potentially cool variable")
@@ -220,6 +223,22 @@ def test_add_csv(decoy: Decoy, subject: ParameterContext) -> None:
     )
 
 
+def test_add_csv_raises_for_multiple_csvs(
+    decoy: Decoy, subject: ParameterContext
+) -> None:
+    """It should raise with a IncompatibleParameterError if there's already a CSV parameter.."""
+    subject._parameters["other_param"] = decoy.mock(
+        cls=mock_csv_parameter_definition.CSVParameterDefinition
+    )
+
+    with pytest.raises(IncompatibleParameterError):
+        subject.add_csv_file(
+            display_name="jkl",
+            variable_name="qwerty",
+            description="fee foo fum",
+        )
+
+
 def test_set_parameters(decoy: Decoy, subject: ParameterContext) -> None:
     """It should set the parameter values."""
     param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition)

api/tests/opentrons/protocols/parameters/test_csv_parameter_definition.py

@@ -1,6 +1,5 @@
 """Tests for the CSV Parameter Definitions."""
 import inspect
-from pathlib import Path
 
 import pytest
 from decoy import Decoy
@@ -62,9 +61,9 @@ def test_create_csv_parameter(decoy: Decoy) -> None:
 def test_set_csv_value(
     decoy: Decoy, csv_parameter_subject: CSVParameterDefinition
 ) -> None:
-    """It should set the CSV parameter value to a path."""
-    csv_parameter_subject.value = Path("123")
-    assert csv_parameter_subject.value == Path("123")
+    """It should set the CSV parameter value to a byte string."""
+    csv_parameter_subject.value = b"123"
+    assert csv_parameter_subject.value == b"123"
 
 
 def test_csv_parameter_as_protocol_engine_type(
@@ -79,13 +78,13 @@ def test_csv_parameter_as_protocol_engine_type(
         file=None,
     )
 
-    csv_parameter_subject.file_info = FileInfo(id="123abc", name="")
+    csv_parameter_subject.file_info = FileInfo(id="123", name="abc")
     result = csv_parameter_subject.as_protocol_engine_type()
     assert result == CSVParameter(
         displayName="My cool CSV",
         variableName="my_cool_csv",
         description="Comma Separated Value",
-        file=FileInfo(id="123abc", name=""),
+        file=FileInfo(id="123", name="abc"),
     )
 
 
@@ -98,6 +97,6 @@ def test_csv_parameter_as_csv_parameter_interface(
     with pytest.raises(RuntimeParameterRequired):
         result.file
 
-    csv_parameter_subject.value = Path("abc")
+    csv_parameter_subject.value = b"abc"
     result = csv_parameter_subject.as_csv_parameter_interface(api_version)
-    assert result._path == Path("abc")
+    assert result.contents == "abc"