feat(api, robot-server): get historical and current command errors

[TamarZanzouri]

Overview

closes https://opentrons.atlassian.net/browse/EXEC-655.
GET historical command errors and refactor current command errors.

Test Plan and Hands on Testing

run a protocol with ER failed commands.

• GET /runs/{runId}/commandsErros while the run is active and make sure you get the failed commands errors.
• GET /runs/{runId}/commandsErros when the run is finished and make sure you get the failed commands errors.

Changelog

• added db schema 8.
• added command_error, command_status field to commands_table.
• insert command_error, command_status when inserting a command to commands table.
• get_commands_errors_slice
• added missing indexes from previous migrations.

Review requests

changes make sense?

Risk assessment

low.

[SyntaxColoring]

Thank you!

[SyntaxColoring]

I have not tested this, but I don't think this index is what we want. (This index is what happens when you just do index=True on the new column declaration.)

This will, across all commands from all runs, maintain an index that is a lexicographic sort of the errors' JSON strings.

That does not help with efficiently serving the SELECT statement that you've written in RunStore, which is something like, "get all the errors from a single run, in order."

So I think what we want instead is either:

• A compound index on (run_id, has_error, index_in_run), where has_error is a true/false value computed from command_error being non-NULL. I don't know how to do this off the top of my head with SQLAlchemy, but here is SQLite documentation that looks relevant.
• A compound index on (run_id, command_status, index_in_run), where command_status is a new column storing the command's succeeded/failed status. And then the SELECT statement filters based on command_status == "failed".

[SyntaxColoring]

We could also test whether it's fast enough to just use the existing (run_id, index_in_run) compound index. From there, filtering for just the failed commands will be an O(n) linear scan, but...it's a linear scan implemented by optimized C, so maybe it's good enough.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (ec7641c) to head (ec64c12).
Report is 140 commits behind head on edge.

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #16697   +/-   ##
=======================================
  Coverage   92.43%   92.43%           
=======================================
  Files          77       77           
  Lines        1283     1283           
=======================================
  Hits         1186     1186           
  Misses         97       97           

Flag	Coverage Δ
g-code-testing	92.43% <ø> (ø)

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

[SyntaxColoring]

This is pending some merge conflict resolution with edge. @TamarZanzouri and I will pick this up again when 8.2.0 release stuff chills out. Re-drafting until then.

[SyntaxColoring]

Mostly looks good, but I'm not sure about the query.

[SyntaxColoring]

Nitpick: Does "_id" really need to have that underscore? Or is it only there because that's how the column was named in the example code?

[TamarZanzouri]

i think that as long as we dont use a defined prop name we are good

[SyntaxColoring]

No action needed, just some thoughts.

Early on in this PR, we made the decision to add the new column with ALTER TABLE, instead of letting SQLAlchemy create the new one from scratch.

I don't think that decision is panning out well.

1. We have to manually create indexes.
2. We have to manually create constraints. (And we're not doing that right now—see my # todo comment in schema_8.py).
3. The command_status column needs to be nullable even when that's not a good match for the data we're storing in it.

1 and 2 are thankfully caught by tests now, and perhaps we can improve them by fleshing out the add_column() function. But 3 is fundamental to SQLite. We talked about fixing 3 with Alembic, but I've looked into it and I'm skeptical that that will be easy.

Meanwhile, is ALTER TABLE really saving that much overhead compared to creating the new table from scratch? We're still iterating over every single command and parsing it as JSON, and I imagine that cost will dominate the savings from not having to copy the data.

I think we should stick with what you have now, but in the future, let's try the other way?

[TamarZanzouri]

the other way means creating all tables from scratch? we still need to insert the data.

[SyntaxColoring]

@TamarZanzouri FYI I think that it was correct for you to have had the other fixes in here, but when I was resolving merge conflicts, it was feeling like a bit too much to try to do at once, so I removed them. Let's do it in a follow-up?

[SyntaxColoring]

@TamarZanzouri FYI, I made these changes when resolving merge conflicts:

1. Made command_status nullable to match the ALTER TABLE statement that you were doing in the migration.
2. Removed the command_status enum constraint because I couldn't figure out how to get the migration to match it in a way that would pass the tests. We should figure that out.

[TamarZanzouri]

tried setting create_constraint=True but its acting weird in the test as you mentioned. maybe we should add a CREATE CONSTRAINT to the migration script?

[TamarZanzouri]

tested the migration on a FLEX and it works as expected. @SyntaxColoring