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.

Sign up for GitHub

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

Jump to bottom

feat(protocol-designer, app, api): step grouping foundation #15737

Merged
merged 23 commits into from
Dec 11, 2024
Merged

feat(protocol-designer, app, api): step grouping foundation #15737

merged 23 commits into from
Dec 11, 2024
+911 −38

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Jul 22, 2024 •
edited
Loading

addresses AUTH-573

Overview

co-authored by @jbleon95

This is the foundation for Step grouping in the system. Step grouping is part of the PD redesign follow-up (originally it was part of the PD redesign but got descoped a few days ago. it IS still part of the redesign plans but will be in a follow up): https://opentrons.atlassian.net/wiki/spaces/RPDO/pages/4331339778/WIP+Step+Grouping+Architecture+Proposal+for+JSON+protocols+only

The way step grouping works is the user in PD adds the step to a group and that information gets parsed into commandAnnotations in the schema as part of a secondOrderCommand.

When uploading the protocol to the app, the commandAnnotations is part of the CompletedProtocolAnalysis which is displayed in the Timeline tab in ProtocolDetails - only on the desktop app. Note that the Timeline tab is only visible if you have the Protocol timeline feature flag turned on.

NOTE: the following video is from old PD navigation and components and can't be tested anymore. Since the redesign is soon to be released and the old components are getting deleted here, the usage of the old components has been deleted in lieu of adding the actions/selectors created in this pr to the new designs.

367801019-1642d2f9-c518-49da-b453-fcf480573d8a.mov

Test Plan

  1. examine the attached protocol, look specifically at "commandAnnotations" key. See that there is are 2 second order commands.

commandAnnotationsTest.json

  1. upload the protocol to the app. make sure the Protocol timeline ff is turned on and go to the "timeline" tab in protocol details. You should see the group displayed.

Changelog

  • create a bunch of redux actions in PD
  • add 2 new keys to RootState in PD (unsavedGroup, stepGroups)
  • extend AnnotatedSteps in the app to display the steps in the group: this involved adding commandAnnotations key to completedProtocolAnalysis and a groupedCommands key to the StoredProtocolData type used in getStoredProtocols and getStoredProtocol selector
  • @jbleon95 added api and robot-server support for commandAnnotations to show up in the CompletedProtocolAnalysis

Review requests

Risk assessment

low, behind ff in app and in PD

@jerader jerader changed the title feat(protocol-designer, app, api): step grouping proof of concept feat(protocol-designer, app, api): step grouping foundation Jul 22, 2024
@github-actions GitHub Actions
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/

1 similar comment
@github-actions GitHub Actions
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/

@jerader jerader force-pushed the app_command-annotations branch from 9f72818 to b1a30ae Compare July 22, 2024 18:25
@github-actions GitHub Actions
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/

2 similar comments
@github-actions GitHub Actions
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/

@github-actions GitHub Actions
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/

@jerader jerader mentioned this pull request Jul 23, 2024
Closed
@jerader jerader force-pushed the app_command-annotations branch from 6e7020d to f3e3a19 Compare August 14, 2024 15:59
@codecov Codecov
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.18%. Comparing base (58a7d19) to head (f3e3a19).
Report is 17 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #15737      +/-   ##
==========================================
- Coverage   63.31%   63.18%   -0.13%     
==========================================
  Files         300      288      -12     
  Lines       15773    15276     -497     
==========================================
- Hits         9986     9652     -334     
+ Misses       5787     5624     -163
Flag Coverage Δ
shared-data 74.63% <ø> (+1.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 12 files with indirect coverage changes

@jerader jerader force-pushed the app_command-annotations branch from f3e3a19 to 3a9212a Compare August 26, 2024 15:32
@jerader jerader marked this pull request as ready for review September 16, 2024 15:25
@jerader jerader requested review from a team as code owners September 16, 2024 15:25
@jerader jerader requested review from smb2268, koji and SyntaxColoring and removed request for a team September 16, 2024 15:25
sfoster1
sfoster1 requested changes Dec 6, 2024
View reviewed changes
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Couple things I think we need to look at it in the app and api

api/src/opentrons/cli/analyze.py Outdated Show resolved Hide resolved
app/src/organisms/Desktop/ProtocolDetails/AnnotatedSteps.tsx Outdated Show resolved Hide resolved
@jbleon95 jbleon95 added the gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails label Dec 6, 2024
@github-actions github-actions bot mentioned this pull request Dec 6, 2024
Merged
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 6, 2024

A PR has been opened to address analyses snapshot changes. Please review the changes here: #17058

@jerader jerader requested review from sfoster1, sanni-t and koji December 9, 2024 19:44
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024

A PR has been opened to address analyses snapshot changes. Please review the changes here: #17058

sfoster1
sfoster1 approved these changes Dec 9, 2024
View reviewed changes
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks awesome to me!

sanni-t
sanni-t approved these changes Dec 9, 2024
View reviewed changes
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

🙌 Annotations backend work looks great!

@github-actions GitHub Actions
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: #17058

@github-actions GitHub Actions
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: #17058

@jerader jerader merged commit b32aa12 into edge Dec 11, 2024
49 checks passed
@jerader jerader deleted the app_command-annotations branch December 11, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbleon95 jbleon95 jbleon95 left review comments

@sanni-t sanni-t sanni-t approved these changes

@sfoster1 sfoster1 sfoster1 approved these changes

@smb2268 smb2268 Awaiting requested review from smb2268 smb2268 is a code owner automatically assigned from Opentrons/js

@SyntaxColoring SyntaxColoring Awaiting requested review from SyntaxColoring

@shlokamin shlokamin Awaiting requested review from shlokamin

@ncdiehl11 ncdiehl11 Awaiting requested review from ncdiehl11

@TamarZanzouri TamarZanzouri Awaiting requested review from TamarZanzouri

@koji koji Awaiting requested review from koji

Assignees
No one assigned
Labels
gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jerader @koji @sfoster1 @sanni-t @jbleon95