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,robot-server): Account for failed commands not having a pipetteId #16859

Merged
merged 10 commits into from
Nov 18, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Nov 15, 2024

Overview

Closes RQA-3599.

Test Plan and Hands on Testing

  • Manually do some GET requests to /runs/{id}/currentState throughout a run and watch the new tipState field change over time. Make sure it's correct.
  • Follow the steps to reproduce in RQA-3599 and make sure the user-observable behavior is fixed.
  • "There is absolutely no way there is a regression" – @mjhuff

Changelog

  • Expose the robot's idea of whether there's a tip logically attached, via the GET /runs/{id}/currentState endpoint.

  • Use that to replace some code in the frontend that was reading command-level information to try to get the pipetteId involved in an error.

    That code could not account for gripper failures, where the failed moveLabware command does not have a pipetteId. That's what was causing RQA-3599.

Review requests

See comments below.

Risk assessment

Low?

@SyntaxColoring SyntaxColoring changed the base branch from edge to chore_release-8.2.0 November 15, 2024 20:19
Comment on lines +298 to +300
# todo(mm, 2024-11-15): Are all of these pass-through methods helpful?
# Can we delete them and make callers just call .run_orchestrator.play(), etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TamarZanzouri Is this a good idea, or does this conflict with any future vision you had for the orchestrator stuff?

Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Nov 15, 2024

Choose a reason for hiding this comment

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

DIscussed in-person. I think we're both a little confused about this, but:

RunOrchestratorStore is largely a glorified RunOrchestrator | None variable. And from that perspective, yes, all of these methods are redundant boilerplate, not offering any abstraction. We can replace RunOrchestrator.foo() with RunOrchestrator.run_orchestrator.foo() without losing anything.

There is then a question of, well, why even have RunOrchestratorStore. The answer is that other than all of these wrapper methods, it also provides access to a "default RunOrchestrator" for the "stateless command" endpoints, and it gates that access behind there not already being some other, run-controlled, RunOrchestrator already active. That is a good thing for the server to do, but I wonder if it still makes sense at this layer, now that we have maintenance runs. We might want one higher-level thing mediating between runs, maintenance runs, and stateless commands.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah stateless commands are not a thing we use anymore I'm pretty sure - we replaced them with maintenance runs because it turns out we need state for most stuff we want to do.

@SyntaxColoring SyntaxColoring marked this pull request as ready for review November 15, 2024 21:01
@SyntaxColoring SyntaxColoring requested review from a team as code owners November 15, 2024 21:01
@SyntaxColoring SyntaxColoring requested review from TamarZanzouri and removed request for a team November 15, 2024 21:01
@SyntaxColoring SyntaxColoring changed the title feat(robot-server): Expose current logical tip state through the HTTP API fix(robot-server): Account for failed commands not having a pipetteId Nov 15, 2024
@SyntaxColoring SyntaxColoring changed the title fix(robot-server): Account for failed commands not having a pipetteId fix(app,robot-server): Account for failed commands not having a pipetteId Nov 15, 2024
@mjhuff mjhuff requested review from sfoster1, smb2268 and a team November 15, 2024 21:26
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 and makes sense.

Comment on lines +298 to +300
# todo(mm, 2024-11-15): Are all of these pass-through methods helpful?
# Can we delete them and make callers just call .run_orchestrator.play(), etc.?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah stateless commands are not a thing we use anymore I'm pretty sure - we replaced them with maintenance runs because it turns out we need state for most stuff we want to do.

@SyntaxColoring SyntaxColoring merged commit 29e03ae into chore_release-8.2.0 Nov 18, 2024
49 checks passed
@SyntaxColoring SyntaxColoring deleted the current_tip_state branch November 18, 2024 15:20
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.

3 participants