Skip to content
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(app,api): Display thermocycler profile cycles #16414

Merged
merged 18 commits into from
Oct 8, 2024

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Oct 3, 2024

We identified a simple bug with the app: it displays wonky numbers for thermocycler profile cycles:
image (1)

This wouldn't repeat anything at all, what's going on?

Well, it turns out that when Protocol Designer added thermocycler support, they wanted something a bit more in depth than the Python protocol API's "list of steps, number of times to repeat the cycle" format. They wanted users to be able to add single steps and cycles, in arbitrary order.

To make the two styles work with the engine, we made the engine's thermocycler/runProfile command take a flat list of steps - any of the structure of repeated commands was removed. In the app's CommandText display, we then had to fix up the way we displayed profiles to remove references to a repetitions value that now didn't exist... and we just didn't, instead setting the repetitions element to just be the number of steps in the profile.

Fixing this ended up being a bit involved.

summary

  • ad17260 Make a new interface for the hardware controller execute_profile function of the thermocycler, since it's important that thermocycler cycles execute inside an asyncio worker so that they won't be interrupted by e.g. pausing. This new interface takes the PD style of protocol. It relies under the hood on the same function that actually sends stuff to the thermocycler, so there shouldn't be any functional execution changes.
  • 335e25a Make a new thermocycler/runExtendedProfile command in the protocol engine, that takes as its parameters a structured list of either steps or cycles, adhering to PD's expectations since the PAPI's expectations are a strict subset of PD. This implements its command by using the new hardware controller.
  • e842a2a Use the new structured data to implement a command text for the new command that can render the equivalent of what PD makes (and, again, what the PAPI does, which is a strict subset)
  • e842a2a Also make the old command text not use a "repetitions" keyword that doesn't exist
  • 0043ab7 Switch the PAPI to emitting the new command in api 2.21

new UI

These UI changes will be on https://s3-us-west-2.amazonaws.com/opentrons-components/rqa-2771-thermocycler-extended-profiles/index.html?path=/docs/app-molecules-command-commandtext--docs for the desktop and general text and https://s3-us-west-2.amazonaws.com/opentrons-components/rqa-2771-thermocycler-extended-profiles/index.html?path=/docs/app-molecules-command-command--docs for the ODD colored-background elements whenever they upload. Here's a screenshot of what they were like when the PR was opened:

CommandText of thermocycler/runProfile :
runProfileCommandText

Note the change to the first line. This needs wordsmithing.

CommandText of thermocycler/runExtendedProfile:
runProfileExtended

Note the two-level rendering. The common case will probably be that there's only cycles

todo

  • wordsmith
  • [ ] add to PD? no, but structured in a way that it will be easy eventually
  • Test. Oh boy

Closes RQA-2771

@sfoster1 sfoster1 requested review from a team as code owners October 3, 2024 20:02
@sfoster1 sfoster1 requested review from jerader, ecormany, SyntaxColoring, emilywools, TamarZanzouri and mjhuff and removed request for a team October 3, 2024 20:02
@sfoster1
Copy link
Member Author

sfoster1 commented Oct 4, 2024

Screenshot 2024-10-04 at 9 47 38 AM

After those latest fixups, a protocol like this displays as above:

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

def run(ctx):
    tr = ctx.load_labware('opentrons_96_tiprack_1000ul', 'D2')
    instr = ctx.load_instrument('flex_1channel_1000', 'left', tip_racks=[tr])
    tc = ctx.load_module('thermocyclerModuleV2')
    tc.load_labware("nest_96_wellplate_100ul_pcr_full_skirt")
    tc.close_lid()
    tc.execute_profile(
        steps=[{'temperature': 30, 'hold_time_seconds': 10},
         {'temperature': 50, 'hold_time_minutes': 1},
         {'temperature': 60, 'hold_time_seconds': 15}],
        repetitions=5,
        block_max_volume=100
    )
    instr.pick_up_tip()
    instr.return_tip()

with the following M104 (set temp) commands for the first couple reps of the profile

Oct 04 13:41:26 002504 opentrons-api-serial[324432]: /dev/ot_module_thermocycler1: Write -> b'M104 S30.0 H10.0 V100.0 \r\n'
Oct 04 13:41:41 002504 opentrons-api-serial[324432]: /dev/ot_module_thermocycler1: Write -> b'M104 S50.0 H60.0 V100.0 \r\n'
Oct 04 13:42:54 002504 opentrons-api-serial[324432]: /dev/ot_module_thermocycler1: Write -> b'M104 S60.0 H15.0 V100.0 \r\n'
Oct 04 13:46:58 002504 opentrons-api-serial[324432]: /dev/ot_module_thermocycler1: Write -> b'M104 S30.0 H10.0 V100.0 \r\n'
Oct 04 13:47:51 002504 opentrons-api-serial[324432]: /dev/ot_module_thermocycler1: Write -> b'M104 S50.0 H60.0 V100.0 \r\n'
Oct 04 13:49:03 002504 opentrons-api-serial[324432]: /dev/ot_module_thermocycler1: Write -> b'M104 S60.0 H15.0 V100.0 \r\n'

@sfoster1
Copy link
Member Author

sfoster1 commented Oct 4, 2024

A test PD protocol using the old command now looks like this:
Screenshot 2024-10-04 at 10 31 58 AM
so at least it says the correct number of things
tc test.json

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend bits all look good to me, as discussed in Slack and on a call. TY!

@sfoster1
Copy link
Member Author

sfoster1 commented Oct 4, 2024

Screenshot 2024-10-04 at 10 48 26 AM The same protocol but manually modified to use the new command. Here's the protocol; it implements the same thing as the previous protocol but using the real command. Don't try and view this in PD though

https://github.com/user-attachments/files/17260291/tc-test-extended.json

Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

App side looks good! Thanks for tackling this one...definitely not the innocuous UI bug as it first seemed.

Oh yeah, and thanks for adding the new commandText kind

Copy link
Collaborator

@ncdiehl11 ncdiehl11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessing this from a PD perspective, this looks good and shouldn't cause a big headache on our end. The main implication will be in thermocyclerFormToArgs to maintain a cycle profile step's dimensionality (and repetitions property) rather than flattening it into its comprising steps.

: profileElementTexts[0].cycleText}
</li>
) : (
profileElementTexts.map((element, index: number) =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

These extended profiles match the way that you can configure a
thermocycler in protocol designer. They support a mix of cycles (more
than one per profile) and out-of-cycle steps, while keeping structure
down to hardware. That last part is sadly important to support
currently-exposed details up from the hardware controller.
This is a thermocycler command that accepts a structured sequence of
temperatures and wait times instead of a flat list.

A profile is a list of either steps or cycles; cycles are a repetition
count plus a list of steps.

The actual _functionality_ of the command is exactly the same as
thermocycler/runProfile; the only change is pushing down the structure
of the information, which is currently lost when creating a runProfile
command and thus unavailable to protocol clients for display.
This PR does a couple things with thermocycler command text.

First, it changes the phrasing of the thermocycler/runProfile command
text to make it more clear that no cycles are involved: it's just a big
list of steps.

Second, it adds a command text for the new
thermocycler/runExtendedProfile command, which does have the structured
data necessary to render things as cycles. This unfortunately makes it a
multilevel list, thus worsening the annoying problems with it.

Closes RQA-2771
Use the new thermocycler/runExtendedProfile to implement
ThermocyclerContext.execute_profile() at or above protocol api version
2.21.

This new version of the command preserves the structure of the thermal
cycles and will allow the desktop app and ODD to present appropriate
information to the user.
@sfoster1 sfoster1 force-pushed the rqa-2771-thermocycler-extended-profiles branch from e9b1e6e to 7296217 Compare October 4, 2024 19:02
Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫡 🔡 ⚒️ wordsmith reporting for duty

Comment on lines 75 to 77
"tc_starting_extended_profile_cycle": "A cycle of {{repetitions}} repetitions of the following steps:",
"tc_starting_extended_profile": "Thermocycler starting {{elementCount}} element profile composed of the following elements:",
"tc_starting_profile": "Thermocycler starting {{stepCount}} step profile composed of the following:",
Copy link
Contributor

@ecormany ecormany Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good timing for wordsmithing here, because I'm taking a look at our other command text across PD and opentrons_simulate as well.

My suggestions here are based on these general factors:

  • We prefer "-ing" forms of verbs (which are present here already).
  • We generally describe steps as doing something to a module (direct object) rather than as the module doing something (subject). The existing TC step is an outlier in that regard.

In addition, I think that the top level should look very similar regardless of whether it has the flat or embedded structure.

Suggested change
"tc_starting_extended_profile_cycle": "A cycle of {{repetitions}} repetitions of the following steps:",
"tc_starting_extended_profile": "Thermocycler starting {{elementCount}} element profile composed of the following elements:",
"tc_starting_profile": "Thermocycler starting {{stepCount}} step profile composed of the following:",
"tc_starting_extended_profile_cycle": "{{repetitions}} repetitions of the following steps:",
"tc_starting_extended_profile": "Running thermocycler profile with {{elementCount}} total steps and cycles:",
"tc_starting_profile": "Running thermocycler profile with {{stepCount}} steps:",

Something about line 74 strikes me as very "here is some data" instead of plain English, too. Pros and cons of going with something like "{{seconds}} seconds at {{celsius}}°C" instead:

  • It sounds more like a human.
  • Except it counts everything in seconds, and these could be long spans of time. Pop quiz: how long is 2700 seconds? Hours, minutes, seconds would be cool but probably a refactor for another day.
  • Variable length cycles will make the output less tabular, and potentially harder to scan.

Bare minimum let's capitalize the T in Temperature, either in the string or with a transformation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the suggested changes and will apply.

I think for changing the individual step texts, definitely sentence capsing, but I agree about the variable length thing, and temperatures should be a lot less variable.

For timestamps, we can pretty trivially transform them to nicer durations - do you prefer mm:ss, [mm minutes ] ss seconds, something else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two unambiguous options:

  • 0:01:02
  • 0h 01m 02s

Even though it raises the question of "does this need to be i18n'd?" I like including the literal text h, m, s because it reduces mental burden of going "wait, which units is that colon separating?"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I agree, and yeah, it would need to be i18n'd but that's just incremental work and c'est la vie. Will do!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that will be Temperature: {temperature}°C, hold time: {hh}h {mm}m {ss}s? Something else?

Copy link
Member Author

@sfoster1 sfoster1 Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, since it looks like we use durations in other places that we'll have to fix, let's just specify it as a duration in the i18n strings

@sfoster1
Copy link
Member Author

sfoster1 commented Oct 7, 2024

@ecormany ok here's where we are now, what do you think
Screenshot 2024-10-07 at 9 20 34 AM

for extremely long times, hours will just be extended - so you'd have 25h, not 1d 1h

@ecormany
Copy link
Contributor

ecormany commented Oct 7, 2024

for extremely long times, hours will just be extended - so you'd have 25h, not 1d 1h

I think if anything, times will tend to be on the short side of an hour. Nit, but a single 0 instead of 00 would probably suffice for any sub-hour time. If you get ragged text because you have a step that exceeds 10h, so be it.

@sfoster1
Copy link
Member Author

sfoster1 commented Oct 8, 2024

Untitled

How's this @ecormany

Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command text is good. 🚢

@sfoster1 sfoster1 merged commit 9d16384 into edge Oct 8, 2024
59 checks passed
@sfoster1 sfoster1 deleted the rqa-2771-thermocycler-extended-profiles branch October 8, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants