-
Notifications
You must be signed in to change notification settings - Fork 178
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
Conversation
A test PD protocol using the old command now looks like this: |
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.
The backend bits all look good to me, as discussed in Slack and on a call. TY!
api/src/opentrons/protocol_engine/commands/thermocycler/run_extended_profile.py
Outdated
Show resolved
Hide resolved
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 |
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.
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
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.
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) => |
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.
👍
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.
e9b1e6e
to
7296217
Compare
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.
🫡 🔡 ⚒️ wordsmith reporting for duty
"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:", |
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.
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.
"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.
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.
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?
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.
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?"
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.
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!
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.
So that will be Temperature: {temperature}°C, hold time: {hh}h {mm}m {ss}s
? Something else?
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.
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
This handles non-normalized durations (i.e., all seconds) now. I think it is janky.
@ecormany ok here's where we are now, what do you think for extremely long times, hours will just be extended - so you'd have |
I think if anything, times will tend to be on the short side of an hour. Nit, but a single |
How's this @ecormany |
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.
Command text is good. 🚢
We identified a simple bug with the app: it displays wonky numbers for thermocycler profile cycles:
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'sCommandText
display, we then had to fix up the way we displayed profiles to remove references to arepetitions
value that now didn't exist... and we just didn't, instead setting therepetitions
element to just be the number of steps in the profile.Fixing this ended up being a bit involved.
summary
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.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.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
ofthermocycler/runProfile
:Note the change to the first line. This needs wordsmithing.
CommandText
ofthermocycler/runExtendedProfile
:Note the two-level rendering. The common case will probably be that there's only cycles
todo
[ ] add to PD?no, but structured in a way that it will be easy eventuallyCloses RQA-2771