feat(api, robot-server): get historical and current command errors #16697
Conversation
robot-server/robot_server/persistence/file_and_directory_names.py
Outdated
Show resolved
Hide resolved
| CREATE INDEX ix_run_command_command_error ON run_command (command_error) | ||
| """, |
There was a problem hiding this comment.
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), wherehas_erroris a true/false value computed fromcommand_errorbeing 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), wherecommand_statusis a new column storing the command'ssucceeded/failedstatus. And then theSELECTstatement filters based oncommand_status == "failed".
There was a problem hiding this comment.
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.
This reverts commit 3a29bfd.
Ensure it's based on a copy-paste of the latest (post-merge) schema_7.py.
Ensure EXPECTED_STATEMENTS_V8 is based on a copy-paste of the latest EXPECTED_STATEMENTS_V7.
SyntaxColoring
force-pushed
the
EXEC-655-store-commands-error-list-in-db
branch
from
bf9b701
to
5af324e
Compare
| add_column( | ||
| dest_engine, | ||
| schema_8.run_command_table.name, | ||
| schema_8.run_command_table.c.command_error, | ||
| ) | ||
|
|
||
| add_column( | ||
| dest_engine, | ||
| schema_8.run_command_table.name, | ||
| schema_8.run_command_table.c.command_status, | ||
| ) | ||
|
|
||
| _add_missing_indexes(dest_transaction=dest_transaction) | ||
|
|
||
| _migrate_command_table_with_new_command_error_col_and_command_status( | ||
| dest_transaction=dest_transaction | ||
| ) |
There was a problem hiding this comment.
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.
- We have to manually create indexes.
- We have to manually create constraints. (And we're not doing that right now—see my
# todocomment inschema_8.py). - The
command_statuscolumn 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?
There was a problem hiding this comment.
the other way means creating all tables from scratch? we still need to insert the data.
| # todo(2024-11-20): Probably add the indexes missing from prior migrations here. | ||
| # https://opentrons.atlassian.net/browse/EXEC-827 |
There was a problem hiding this comment.
@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?
| # nullable=True because it was easier for the migration to add the column | ||
| # this way. This is not intended to ever be null in practice. | ||
| nullable=True, | ||
| # todo(mm, 2024-11-20): We want create_constraint=True here. Something | ||
| # about the way we compare SQL in test_tables.py is making that difficult-- | ||
| # even when we correctly add the constraint in the migration, the SQL | ||
| # doesn't compare equal to what create_constraint=True here would emit. | ||
| create_constraint=False, |
There was a problem hiding this comment.
@TamarZanzouri FYI, I made these changes when resolving merge conflicts:
- Made
command_statusnullable to match theALTER TABLEstatement that you were doing in the migration. - Removed the
command_statusenum 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.
There was a problem hiding this comment.
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?
Co-authored-by: Max Marrone <[email protected]>
Co-authored-by: Max Marrone <[email protected]>
|
tested the migration on a FLEX and it works as expected. @SyntaxColoring |
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.
/runs/{runId}/commandsErroswhile the run is active and make sure you get the failed commands errors./runs/{runId}/commandsErroswhen the run is finished and make sure you get the failed commands errors.Changelog
command_error,command_statusfield tocommands_table.command_error,command_statuswhen inserting a command to commands table.get_commands_errors_sliceReview requests
changes make sense?
Risk assessment
low.