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(robot-server): append error to analysis error list in case an internal error occurred #14510

Merged

Conversation

TamarZanzouri
Copy link
Contributor

@TamarZanzouri TamarZanzouri commented Feb 16, 2024

Overview

closes RSS-169.
append internal error to analysis errors list in case a un expected error occured.

Test Plan

Tested this by adding a raise in the runner.run method. made sure that the analysis is not stuck in a pending state.
Can do the same test on a robot.
Make sure analysis completed with "NOT_OK" result and that the error appears.

{
    "data": [
        {
            "id": "45aeefdd-8450-470d-8289-bde0d6c17597",
            "status": "completed",
            "result": "not-ok",
            "robotType": "OT-2 Standard",
            "commands": [],
            "labware": [],
            "pipettes": [],
            "modules": [],
            "liquids": [],
            "errors": [
                {
                    "id": "internal-error",
                    "createdAt": "2024-02-16T02:24:08.423768+00:00",
                    "errorCode": "4000",
                    "errorType": "PythonException",
                    "detail": "RuntimeError: No active exception to reraise",
                    "errorInfo": {
                        "args": "('No active exception to reraise',)",
                        "traceback": "  File \"/Users/tamarzanzouri/opentrons/robot-server/robot_server/protocols/protocol_analyzer.py\", line 37, in analyze\n    result = await runner.run(\n\n  File \"/Users/tamarzanzouri/opentrons/api/src/opentrons/protocol_runner/protocol_runner.py\", line 307, in run\n    raise\n",
                        "class": "RuntimeError"
                    },
                    "wrappedErrors": []
                }
            ]
        }
    ],
    "meta": {
        "cursor": 0,
        "totalLength": 1
    }
}

Changelog

  • Added a "catch all" for errors that the PE does not catch internally in analyze.
  • Create an error from the internal error and append it to the analysis error list.

Risk assessment

low.

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f2978c7) 67.76% compared to head (832b783) 67.76%.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                  @@
##           chore_release-7.2.0   #14510   +/-   ##
====================================================
  Coverage                67.76%   67.76%           
====================================================
  Files                     2517     2517           
  Lines                    72054    72054           
  Branches                  9276     9276           
====================================================
  Hits                     48828    48828           
  Misses                   21008    21008           
  Partials                  2218     2218           
Flag Coverage Δ
app 64.63% <ø> (ø)

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

Files Coverage Δ
...server/robot_server/protocols/protocol_analyzer.py 100.00% <ø> (ø)

@TamarZanzouri TamarZanzouri changed the title Rss 169 analysis internal error pending fix(robot-server): append error to analysis error list in case an internal error occurred Feb 16, 2024
@TamarZanzouri TamarZanzouri marked this pull request as ready for review February 16, 2024 02:28
@TamarZanzouri TamarZanzouri requested a review from a team as a code owner February 16, 2024 02:28
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.

Looks good, just a few small suggestions. Thank you!

robot-server/robot_server/protocols/protocol_analyzer.py Outdated Show resolved Hide resolved
robot-server/robot_server/protocols/protocol_analyzer.py Outdated Show resolved Hide resolved
robot-server/tests/protocols/test_protocol_analyzer.py Outdated Show resolved Hide resolved
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, couple tiny fixes maybe

robot-server/robot_server/protocols/protocol_analyzer.py Outdated Show resolved Hide resolved
@TamarZanzouri TamarZanzouri changed the base branch from edge to chore_release-7.2.0 February 16, 2024 16:48
@TamarZanzouri TamarZanzouri requested review from a team as code owners February 16, 2024 16:48
@TamarZanzouri TamarZanzouri requested review from smb2268 and removed request for a team February 16, 2024 16:50
@TamarZanzouri TamarZanzouri force-pushed the RSS-169-analysis-internal-error-pending branch from 504d1b8 to ce66749 Compare February 16, 2024 16:53
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.

Looks like there was some rebasing mixup? Should rebase on top of chore_release-7.2.0.

@TamarZanzouri TamarZanzouri force-pushed the RSS-169-analysis-internal-error-pending branch 2 times, most recently from 240f196 to 2c6489f Compare February 16, 2024 18:04
progress

update on fail

added completed_with_error to update signature

added tests and fixed linting

fixed linting

added EnumeratedError to errors list and removed completed_with_error

create an ErrorOccurence from the internal error

monkeypatching not going so well

progress with mocking. still not complete

fixed mocking!need fo figure out datetime failing

fixed bug with datetime compare

remove unnecessary test

reverted changes and restructured

removed print

reverted changes

Update robot-server/robot_server/protocols/protocol_analyzer.py

Co-authored-by: Seth Foster <seth@opentrons.com>

pr fixes
@sanni-t sanni-t force-pushed the RSS-169-analysis-internal-error-pending branch from 2c6489f to 832b783 Compare February 16, 2024 18:24
@TamarZanzouri TamarZanzouri merged commit 18d99a7 into chore_release-7.2.0 Feb 16, 2024
17 checks passed
@TamarZanzouri TamarZanzouri deleted the RSS-169-analysis-internal-error-pending branch February 16, 2024 18:50
DerekMaggio added a commit that referenced this pull request Mar 7, 2024
# Overview

Add protocols pulled from v7.2.0 pull requests for Analysis Snapshot
testing. All work filed under
[RQA-2434](https://opentrons.atlassian.net/browse/RQA-2434)

# Test Plan

- [x] Add partial tip pickup protocols from Sanitti
- [x] Add ABR protocol from
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23)
- [x] Run new protocols locally and verify results
- [x] Run in workflow dispatch action and verify success
[[link]](https://github.com/Opentrons/opentrons/actions/runs/8160792870/job/22308167177)

# Changelog

Here are the protocols added and where I found them:

- [RQA-2098](https://opentrons.atlassian.net/browse/RQA-2098)
-
Flex_P1000_96_Gripper_TC_TM_HS_AnalysisError_GripperCollisionWithTips.json
- #14491
  - Flex_None_None_TC_2_14_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_15_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_16_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_17_verifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_14_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_15_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_16_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_17_VerifyThermocyclerLoadedSlots.py
- #14475
-
Flex_None_None_TC_2_16_AnalysisError_TrashBinAndThermocyclerConflict.py
  - OT2_None_None_2_16_verifyDoesNotDeadlock.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin1.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin2.py
- #14547
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLid.py
- #14522
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLidClips.py
- Dispense functionality validation
  - OT2_P300M_P20S_TC_HS_TM_2_15_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_16_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_17_dispense_changes.py
- #14253
  - OT2_P300S_None_2_16_verifyNoFloatingPointErrorInPipetting.py

# Review requests

There are a few pull requests that I had questions about. I will @ the
people that worked on them to answer the questions

- #14437
- @sfoster1, Can I use the protocol found in
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23) to get this to
happen? Is there another way?
- #14509
- @SyntaxColoring, can analysis capture this change or is this only
relavant during actual protocol runtime?
- #14510
- @TamarZanzouri or @SyntaxColoring, can I raise a generic python
exception inside the protocol to trigger this functionality?
- #14512
- @Laura-Danielle or @SyntaxColoring, Iant to validate my logic. This
not a good canidate for snapshot testing because you have to run LPC and
Calibration which is not taken into account during analysis. Can you
confirm?

# Risk assessment

None


[RQA-2434]:
https://opentrons.atlassian.net/browse/RQA-2434?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RQA-2098]:
https://opentrons.atlassian.net/browse/RQA-2098?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RABR-23]:
https://opentrons.atlassian.net/browse/RABR-23?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
Add protocols pulled from v7.2.0 pull requests for Analysis Snapshot
testing. All work filed under
[RQA-2434](https://opentrons.atlassian.net/browse/RQA-2434)

- [x] Add partial tip pickup protocols from Sanitti
- [x] Add ABR protocol from
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23)
- [x] Run new protocols locally and verify results
- [x] Run in workflow dispatch action and verify success
[[link]](https://github.com/Opentrons/opentrons/actions/runs/8160792870/job/22308167177)

Here are the protocols added and where I found them:

- [RQA-2098](https://opentrons.atlassian.net/browse/RQA-2098)
-
Flex_P1000_96_Gripper_TC_TM_HS_AnalysisError_GripperCollisionWithTips.json
- #14491
  - Flex_None_None_TC_2_14_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_15_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_16_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_17_verifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_14_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_15_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_16_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_17_VerifyThermocyclerLoadedSlots.py
- #14475
-
Flex_None_None_TC_2_16_AnalysisError_TrashBinAndThermocyclerConflict.py
  - OT2_None_None_2_16_verifyDoesNotDeadlock.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin1.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin2.py
- #14547
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLid.py
- #14522
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLidClips.py
- Dispense functionality validation
  - OT2_P300M_P20S_TC_HS_TM_2_15_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_16_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_17_dispense_changes.py
- #14253
  - OT2_P300S_None_2_16_verifyNoFloatingPointErrorInPipetting.py

There are a few pull requests that I had questions about. I will @ the
people that worked on them to answer the questions

- #14437
- @sfoster1, Can I use the protocol found in
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23) to get this to
happen? Is there another way?
- #14509
- @SyntaxColoring, can analysis capture this change or is this only
relavant during actual protocol runtime?
- #14510
- @TamarZanzouri or @SyntaxColoring, can I raise a generic python
exception inside the protocol to trigger this functionality?
- #14512
- @Laura-Danielle or @SyntaxColoring, Iant to validate my logic. This
not a good canidate for snapshot testing because you have to run LPC and
Calibration which is not taken into account during analysis. Can you
confirm?

None

[RQA-2434]:
https://opentrons.atlassian.net/browse/RQA-2434?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RQA-2098]:
https://opentrons.atlassian.net/browse/RQA-2098?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RABR-23]:
https://opentrons.atlassian.net/browse/RABR-23?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Carlos-fernandez pushed a commit that referenced this pull request Jun 3, 2024
Add protocols pulled from v7.2.0 pull requests for Analysis Snapshot
testing. All work filed under
[RQA-2434](https://opentrons.atlassian.net/browse/RQA-2434)

- [x] Add partial tip pickup protocols from Sanitti
- [x] Add ABR protocol from
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23)
- [x] Run new protocols locally and verify results
- [x] Run in workflow dispatch action and verify success
[[link]](https://github.com/Opentrons/opentrons/actions/runs/8160792870/job/22308167177)

Here are the protocols added and where I found them:

- [RQA-2098](https://opentrons.atlassian.net/browse/RQA-2098)
-
Flex_P1000_96_Gripper_TC_TM_HS_AnalysisError_GripperCollisionWithTips.json
- #14491
  - Flex_None_None_TC_2_14_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_15_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_16_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_17_verifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_14_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_15_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_16_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_17_VerifyThermocyclerLoadedSlots.py
- #14475
-
Flex_None_None_TC_2_16_AnalysisError_TrashBinAndThermocyclerConflict.py
  - OT2_None_None_2_16_verifyDoesNotDeadlock.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin1.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin2.py
- #14547
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLid.py
- #14522
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLidClips.py
- Dispense functionality validation
  - OT2_P300M_P20S_TC_HS_TM_2_15_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_16_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_17_dispense_changes.py
- #14253
  - OT2_P300S_None_2_16_verifyNoFloatingPointErrorInPipetting.py

There are a few pull requests that I had questions about. I will @ the
people that worked on them to answer the questions

- #14437
- @sfoster1, Can I use the protocol found in
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23) to get this to
happen? Is there another way?
- #14509
- @SyntaxColoring, can analysis capture this change or is this only
relavant during actual protocol runtime?
- #14510
- @TamarZanzouri or @SyntaxColoring, can I raise a generic python
exception inside the protocol to trigger this functionality?
- #14512
- @Laura-Danielle or @SyntaxColoring, Iant to validate my logic. This
not a good canidate for snapshot testing because you have to run LPC and
Calibration which is not taken into account during analysis. Can you
confirm?

None

[RQA-2434]:
https://opentrons.atlassian.net/browse/RQA-2434?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RQA-2098]:
https://opentrons.atlassian.net/browse/RQA-2098?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RABR-23]:
https://opentrons.atlassian.net/browse/RABR-23?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
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.

4 participants