feat(api, robot-server): use runtime parameter files set in protocols and runs to set in-protocol values

[jbleon95]

Overview

Closes AUTH-419.

This PR hooks up the setting of runtime parameter files in the POST /runs, /protocols and /protocols/{protocolId}/analyses to the actual CSVParameter object used in the protocol context.

This file is sent via a dictionary of parameter name and file-id, so we resolve the file-id into a Path by using the data_files_directory and data_files_store. This is then passed to the ParameterContext and used to open up a temporary file that contains the contents of the actual file stored on the robot.

Test Plan and Hands on Testing

Tested end to end via the following protocol and two CSV files

metadata = {
    'protocolName': 'CSV End to End Test',
}

requirements = {
    "robotType": "Flex",
    "apiLevel": "2.18"
}


def add_parameters(parameters):
    parameters.add_str(
        display_name="Pipette Name",
        variable_name="pipette",
        choices=[
            {"display_name": "Single channel 50µL", "value": "flex_1channel_50"},
            {"display_name": "Single channel 1000µL", "value": "flex_1channel_1000"},
            {"display_name": "Eight Channel 50µL", "value": "flex_8channel_50"},
            {"display_name": "Eight Channel 1000µL", "value": "flex_8channel_1000"},
        ],
        default="flex_1channel_50",
        description="What pipette to use during the protocol.",
    )
    parameters.add_csv_file(
        display_name="CSV Data",
        variable_name="csv_data",
        description="CSV file containing labware and volume information."
    )



def run(context):
    PIPETTE_NAME = context.params.pipette

    trash_bin = context.load_trash_bin('A3')
    tip_rack = context.load_labware('opentrons_flex_96_tiprack_50ul', 'D2')

    pipette = context.load_instrument(PIPETTE_NAME, mount="left", tip_racks=[tip_rack])
    csv_file_data = context.params.csv_data.parse_as_csv()

    labware_name = csv_file_data[1][0].strip()
    location = csv_file_data[1][1].strip()
    volume = float(csv_file_data[1][2])


    labware = context.load_labware(labware_name, location)
    pipette.pick_up_tip()
    pipette.aspirate(volume, labware.wells()[0].top())
    pipette.dispense(volume, labware.wells()[1].top())
    pipette.drop_tip()

Labware Name, Location, Volume
opentrons_96_wellplate_200ul_pcr_full_skirt, C1, 20

Labware Name, Location, Volume
nest_96_wellplate_100ul_pcr_full_skirt, C2, 30

Changelog

• Resolve runTimeParameterFiles into a dictionary of Paths
• pass the run_time_param_paths all the way to the ParameterContext where they can be opened as temporary file handlers
• close the file handlers upon protocol end
• Allow importing of CSVParameter from the protocol_api namespace.

Review requests

Risk assessment

Medium.

[SyntaxColoring]

This mostly makes sense to me and looks good to merge, though I'm not super familiar with the whole RTP flow.

My main concern going forward is the open()-close()-pair resource management thing.

[SyntaxColoring]

It doesn't really matter, but out of curiosity, why are we adding a second run_time_param_paths argument all the way down, instead of extending the existing run_time_param_values arg to also carry CSV paths?

Like, it's not like we have 4 different arguments for int params, string params, boolean params, and float params.

Structurally, one argument seems more correct to me because it makes it structurally impossible for different parameters to have colliding keys.

[jbleon95]

Good point, this is an artifact of these originally being a dictionary of param names to file-ids, which since file IDs were strings required a different variable for disambiguation. Now that they're Path objects that isn't needed, but I'm gonna put a TODO to fix that up next week before the code freeze.

[sanni-t]

I prefer the separate arguments for two reasons-

1. it keeps the meaning of a parameter's 'value' consistent throughout code. A CSV parameter's 'value' is its file with its contents, while we are passing the file IDs/ paths in these arguments, not the file itself. So the two arguments keep the two concerns separate.
2. because a parameter's value and csv parameter's file paths have to be handled differently, aka, you 'set a param value' vs. 'initialize a file using file path', the two separate arguments make it very easy to split the handling. If the prim. param values & csv param paths are passed in as one entity, you end up having to look up the type of the parameter and then handle the param accordingly. There are a few places in code where we need to know the param type to do this so it adds a lot of repetitive 'is this a CSV param' code.

But if you still think it's better to have one arg for both then I won't oppose it.

[SyntaxColoring]

Should this have validation so invalid file IDs don't raise a 500 error? Or a # todo.

[jbleon95]

Hmmm, interesting point. Raising a 500 error here would be very different behavior compared to other parameter validation we do, but just plain skipping invalid file IDs would currently lead to the same error as not providing any file-id to begin with. We can discuss this further but I'll make a TODO so this gets addressed in next week's follow up.

[SyntaxColoring]

Yeah, I don't think you'd want to skip it. I think you'd want to treat it like when you try to create a run with a protocolId that doesn't point to a valid protocol.

[sanni-t]

I think if the file ID doesn't exist, an error will eventually get raised when we set the CSV parameter during orchestrator load. So it will definitely error out shortly after this, even if it's not validated here and I think that's ok. Because a CSV file isn't exactly like a protocol file. The CSV file's validation will happen in the same place where rest of the RTP parameters' validation occurs.

[SyntaxColoring]

I'm glad we're thinking about closing the files when we're done with them, but is this the correct place to do it? Where are the files opened?

For example, it looks like one route for them to be opened is ProtocolExecutor.extract_run_parameters(), when a protocol is loaded, and that does not necessarily seem paired with any parameter_context.close_csv_files().

(The guideline that I'm aiming to follow is that every open has exactly one close, and the thing that does the open is the same as the thing that does the close.)

[jbleon95]

Yeah, this was probably my biggest ❓ with this PR. I didn't immediately see a good place to tightly couple the two as you suggest and so I put it in the place where the majority of protocols would enter in the first place, but I agree it's imperfect. I think this is probably a sign that these should be opened in a different location which would also tie into the next comment you have about file opening. Again, another TODO for next week so science can be unblocked from a functional version of this.

[SyntaxColoring]

Makes sense. I would look into doing the open() and close() in exec_run() or one of exec_run()'s callers. I haven't looked at them too closely, so big grain of salt.

[SyntaxColoring]

If I'm reading this correctly:

1. We are given a path, the "original file," which we want to treat as read-only.
2. We open the file at that path and copy its contents to a new, temporary file. This makes sense.
3. We close that temporary file and then reopen it, and this reopened file is what we use for general use during the protocol.

Why are we doing step 3?

---

Also, I know you're not supporting this now, but heads up for the future: Any kind of open() at this stage is making an assumption about the file's encoding before the user's Python has a chance to run and tell us what encoding it expects. If you ever support custom encodings, the open() will have to move somewhere else, fundamentally. So let's not bake it too deep into the structure.

[jbleon95]

Step 3 is to enforce read-only permissions on the file, since to write to the temporary file we need to make it read-write privileges. Since it's a temporary file there's no inherent danger in giving write privileges but it seemed the more "correct" thing to enforce.

As for encoding you can actually change the encoding of an active file even after opening it via reconfigure, but again with the previous reply to your comment this code may end up moving closer to where the user is using it, more easily allowing this for future types of files (CSV files I feel are pretty safe to assume UTF-8 text)

[SyntaxColoring]

What's this reexport for? Are we expecting users of the Python Protocol API to import this in their scripts?

[jbleon95]

This was tacked on but via a discussion with Ed we want this easily exposed since this interface is a first class object in our API now.

[SyntaxColoring]

Oh, I see now, that makes sense. We should aim to define the class inside opentrons.protocol_api instead of opentrons.protocols.parameters.types, then. Until we do that, mind leaving a note on the class definition so it's obvious that it's user-facing?

[SyntaxColoring]

Also, reminder to rebase onto chore_release-8.0.0 before merging.

[TamarZanzouri]

agree with @SyntaxColoring comments and also agree with adding a TODO for these cases for now. besides that LGTM!

[sanni-t]

LGTM! Left a couple of comments.
Tested on OT-2 and verified that CSV protocol works end-to-end! 🙌 🙌 🙌