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(api): get_slice should return last executed command if a run failed #14449

Merged

Conversation

TamarZanzouri
Copy link
Contributor

@TamarZanzouri TamarZanzouri commented Feb 8, 2024

Overview

closes RQA-2243.
if a run has failed and the cursor is not supplied we should return the last executed command.

Test Plan

  • Upload a json protocol that will have a failed command.

  • Once run has failed fetch /runs/{runId}/commands make sure you are getting back the failed command and not the last command.

  • Test python protocols as well

  • Test successful protocols with and without cursor.

  • Added integration tests

Changelog

  • when getting command_slice check if there is a run_result (if the run ended). if it did and its in a failed status get the failed command index as the cursor.

Review requests

are you ok with this approach? is it better to store the failed command in a variable?

Risk assessment

low. should only affect failed protocols.

@TamarZanzouri TamarZanzouri requested review from a team as code owners February 8, 2024 15:01
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (chore_release-7.2.0@ab166dc). Click here to learn what that means.

Additional details and impacted files

Impacted file tree graph

@@                  Coverage Diff                   @@
##             chore_release-7.2.0   #14449   +/-   ##
======================================================
  Coverage                       ?   68.07%           
======================================================
  Files                          ?     2522           
  Lines                          ?    72043           
  Branches                       ?     9223           
======================================================
  Hits                           ?    49043           
  Misses                         ?    20809           
  Partials                       ?     2191           
Flag Coverage Δ
g-code-testing 92.43% <0.00%> (?)

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

@TamarZanzouri TamarZanzouri added the robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). label Feb 8, 2024
Copy link
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

Looks good and definitely fixes the issues faced by the current endpoint on JSON protocols. Tested on RSS Flex, performs as expected and does not raise incorrect modals in the case provided by RQA-2242.
Question below seeking some clarification:

api/src/opentrons/protocol_engine/state/commands.py Outdated Show resolved Hide resolved
@SyntaxColoring

This comment was marked as resolved.

@SyntaxColoring
Copy link
Contributor

SyntaxColoring commented Feb 9, 2024

Recapping a conversation that RSS had in a call:

As-is, this PR only fixes the behavior for the current run, which is served from memory, and not for historical runs, which are served from the database. Ideally, the behavior between the two should certainly be consistent. We need to ask the app team what the priority is for solving this for historical runs.

One way to solve this for historical runs is:

  • A new command status, like run-stopped-before-this-command-started. (Except with a better name.) We suspected we'd need this a while ago, in RCORE-390.
  • A new database column for the command status, for efficient querying.

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.

Check out my two comments above about the target branch and historical runs, but when those are answered, this looks great to merge. Thank you!

This reverts commit 3324fb3.
@TamarZanzouri
Copy link
Contributor Author

TamarZanzouri commented Feb 9, 2024

Recapping a conversation that RSS had in a call:

As-is, this PR only fixes the behavior for the current run, which is served from memory, and not for historical runs, which are served from the database. Ideally, the behavior between the two should certainly be consistent. We need to ask the app team what the priority is for solving this for historical runs.

One way to solve this for historical runs is:

  • A new command status, like run-stopped-before-this-command-started. (Except with a better name.) We suspected we'd need this a while ago, in RCORE-390.
  • A new database column for the command status, for efficient querying.

spoke to @shlokamin and historical runs are not urgent. will create a ticket

@TamarZanzouri TamarZanzouri changed the base branch from edge to chore_release-7.2.0 February 9, 2024 20:41
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 good to me!

@TamarZanzouri TamarZanzouri merged commit 18fbbe1 into chore_release-7.2.0 Feb 9, 2024
19 of 22 checks passed
@TamarZanzouri TamarZanzouri deleted the RQA-2243-bug-json-failed-command-result branch February 9, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants