chore(robot-server, api): filter fixit commands out of the commands list #16075

TamarZanzouri · 2024-08-20T20:03:07Z

Overview

reference #16001 for previous pr comments.
closes EXEC-654.
get filtered command list for historical runs and for current runs.

Test Plan and Hands on Testing

upload a protocol that will enter ER.
add some fixit commands.
make sure you dont see the fixit commands in the run log.
when the run is completed and un-current make sure you still dont see fixit commands.

Changelog

added an arg includeFixitCommands to get_run_commands route.
add logic to CommandState and CommandHistory to filter based on the protocol intent.
added column command_intent to run_command_table and migration scripts.
fixed but with copying over enum columns in the DB.
added filtering in the sql query for command intent.

Review requests

changes make sense? how ugly is this?

Risk assessment

Medium. need to make sure run commands did not changed the default behavior.

api/src/opentrons/protocol_runner/run_orchestrator.py

codecov · 2024-08-20T20:11:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (3f48d03) to head (1d4d990).
Report is 201 commits behind head on edge.

Additional details and impacted files

@@             Coverage Diff             @@
##             edge   #16075       +/-   ##
===========================================
+ Coverage   56.21%   92.43%   +36.22%     
===========================================
  Files          32       77       +45     
  Lines        1681     1283      -398     
===========================================
+ Hits          945     1186      +241     
+ Misses        736       97      -639

Flag	Coverage Δ
g-code-testing	`92.43% <ø> (?)`
update-server	`?`
usb-bridge	`?`

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

see 109 files with indirect coverage changes

robot-server/robot_server/runs/run_data_manager.py

SyntaxColoring

I know this is a work in progress, just some preliminary comments:

robot-server/robot_server/persistence/tables/schema_7.py

api/src/opentrons/protocol_engine/state/command_history.py

… - using intent instead of command_intent

sfoster1

Looks good, need to remove all that commented out test code though.

Do we need both those command_intent != 'fixit' OR command_intent == None things there? shouldn't fixit != none? isn't that column non-nullable anyway?

robot-server/robot_server/persistence/_migrations/v6_to_v7.py

robot-server/robot_server/runs/run_store.py

robot-server/tests/integration/http_api/persistence/test_compatibility.py

Co-authored-by: Seth Foster <[email protected]>

sfoster1

Sweet!

…mmands-list

vegano1 · 2024-09-10T20:25:44Z

robot-server/robot_server/persistence/_migrations/v6_to_v7.py

+    for old_row in source_transaction.execute(select_old_protocols).all():
+        new_protocol_kind = (
+            schema_6.ProtocolKindSQLEnum.STANDARD.value
+            if old_row.protocol_kind == "<ProtocolKindSQLEnum.STANDARD: 'standard'>"
+            else schema_6.ProtocolKindSQLEnum.QUICK_TRANSFER.value
+        )


We spoke a bit about this yesterday, can you go over why we need to do this?

The change that added ProtocolKindSQLEnum converts the stored values for protocol_kind from plain strings to SQLEnums like so "<ProtocolKindSQLEnum.STANDARD: 'standard'>". While this is stored on the database in the above format, when you use sqlalchemy to query it returns an actual enum object.

So we don't want to store the plain string values "quick-transfer" and "standard" but <ProtocolKindSQLEnum.QUICK_TRANSFER: 'quick-transfer'> and <ProtocolKindSQLEnum.STANDARD: 'standard'> as we have now.

Furthermore, we added this migration step to account for NULL protocol_kind values, which are semantically equal to the "standard" protocol kind in DB schema V5. This change would revert that.

What @vegano1 said. The protocol table should be copied unmodified. There is nothing wrong with the protocol kind column's contents in v6 database.

if we store the string values we are still able to parse as ProtocolKindSQLEnum. if you will take a look at the code lambda obj: [e.value for e in obj] should insert the value of the enum in the DB but instead its storing the member. The problem I encountered is that bc we store <ProtocolKindSQLEnum.QUICK_TRANSFER: 'quick-transfer'> when migrating and copying over the row's ProtocolKindSQLEnum does not know how to parse this value. its expecting quick-transfer. @sanni-t @vegano1

values_callable –
A callable which will be passed the PEP-435 compliant enumerated type, which should then return a list of string
values to be persisted. This allows for alternate usages such as using the string value of an enum to be persisted to the database instead of its name.

from documentation. https://docs.sqlalchemy.org/en/14/core/type_basics.html#sqlalchemy.types.Enum

vegano1

Left a comment regarding quick transfers protol_kind key in the database.

sanni-t

Should definitely change the protocol table migration to copy it unmodified.

Other than that I think there has to be an easier way to migrate when only adding a column to the table (like what we do in v4 to v5 migration) but it's a soft suggestion. If this is blocking something then we don't have to make that optimization in this PR.

sanni-t · 2024-09-11T16:43:49Z

robot-server/robot_server/persistence/_migrations/v6_to_v7.py

+    for old_row in source_transaction.execute(select_old_protocols).all():
+        new_protocol_kind = (
+            schema_6.ProtocolKindSQLEnum.STANDARD.value
+            if old_row.protocol_kind == "<ProtocolKindSQLEnum.STANDARD: 'standard'>"
+            else schema_6.ProtocolKindSQLEnum.QUICK_TRANSFER.value
+        )


What @vegano1 said. The protocol table should be copied unmodified. There is nothing wrong with the protocol kind column's contents in v6 database.

sanni-t · 2024-09-11T17:24:43Z

robot-server/robot_server/persistence/_migrations/v6_to_v7.py

+            source_transaction = exit_stack.enter_context(source_engine.begin())
+            dest_transaction = exit_stack.enter_context(dest_engine.begin())
+
+            _migrate_db_with_changes(source_transaction, dest_transaction)


Copying over unmodified tables individually work, but I wonder if there's an easier way of doing what you want. You are just adding a column to a table and inserting its contents. There should be a way to just copy over the entire .db file, then open it up with the new schema, add a column and insert contents for the new column.

The method in this PR is not wrong, but feels like we are doing too much work for just adding a column. Also, it's easy to forget to copy over tables and files when doing such individual copying.

I agree but its not just adding a column. Its selecting the command as a json and extracting data from it. I agree, if it was just a simple SQL server I would have probably just ALTER and UPDATE, but its a bit more complicated when using json objects.

sanni-t · 2024-09-11T17:34:56Z

robot-server/robot_server/persistence/tables/schema_7.py

+    sqlalchemy.Column("index_in_run", sqlalchemy.Integer, nullable=False),
+    sqlalchemy.Column("command_id", sqlalchemy.String, nullable=False),
+    sqlalchemy.Column("command", sqlalchemy.String, nullable=False),
+    sqlalchemy.Column("command_intent", sqlalchemy.String, nullable=False, index=True),


I can see the usefulness of creating an index on this, but it doesn't look like we are using it when selecting commands, are we? Not saying that this column shouldn't be indexed, just want to check if we are planning on using the index.

we are using command_intent in the where clause when slicing historical run commands

TamarZanzouri · 2024-09-12T18:29:27Z

tested migrations! works perfectly! tested changes in route - works as expected

sanni-t

Yay! I'm glad we were able to figure out the enum weirdness and reduce the steps needed to do the migration.

Have a nitpick and a question. Otherwise the db changes lgtm!

robot-server/robot_server/persistence/_migrations/v6_to_v7.py

sanni-t · 2024-09-12T20:46:45Z

robot-server/robot_server/persistence/_migrations/v6_to_v7.py

+                engine.execute(
+                    f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}"
+                )


Does this get committed in the same transaction as the table update? I get confused with the different ways execute can be called and what it implies. Just want to be sure that it's one transaction.

Should be the same transaction

PTO

TamarZanzouri added 3 commits August 20, 2024 14:38

cherry picked prev branch changes onto edge

0f2dc73

cherry pick fixes

be7bd0b

fixed failing tests

09b8a88

TamarZanzouri requested a review from a team as a code owner August 20, 2024 20:03

TamarZanzouri marked this pull request as draft August 20, 2024 20:03

TamarZanzouri commented Aug 20, 2024

View reviewed changes

api/src/opentrons/protocol_runner/run_orchestrator.py Outdated Show resolved Hide resolved

TamarZanzouri added 7 commits August 20, 2024 16:25

added missing tests

7fe8fdc

refactor so that complexity is better

96e8743

changed logic to append to a non fixit list

9aa3fa3

set _all_command_ids_but_fixit_command_ids in test

0c8110c

insert, update, get WIP include_fixit

4f3df72

preserialized and comments

272ebf4

moved insert to non fixit into _add

0c99a4f

TamarZanzouri commented Aug 23, 2024

View reviewed changes

robot-server/robot_server/runs/run_data_manager.py Outdated Show resolved Hide resolved

Update robot-server/robot_server/runs/run_data_manager.py

eaea919

SyntaxColoring previously requested changes Aug 28, 2024

View reviewed changes

robot-server/robot_server/persistence/tables/schema_7.py Outdated Show resolved Hide resolved

api/src/opentrons/protocol_engine/state/command_history.py Show resolved Hide resolved

TamarZanzouri and others added 11 commits September 6, 2024 12:11

added non fixit commands to test from DB

2b97128

changed db schema to 7, WIP - why is select without fixit not working…

2b8b711

… - using intent instead of command_intent

fixed selecting without fixit

86f8d56

migration script

696833a

migration tests

f70d456

persistance settings

82d5c69

fixed migration problem with protocol kind

a0db26c

validate strings

72ce75c

command_intent not null

3a98b49

added indexing

b9555dd

Update api/src/opentrons/protocol_runner/run_orchestrator.py

20dcd53

TamarZanzouri changed the title ~~Exec 686 filter fixit commands out of the commands list~~ chore(robot-server, api): filter fixit commands out of the commands list Sep 10, 2024

TamarZanzouri marked this pull request as ready for review September 10, 2024 17:38

TamarZanzouri requested a review from sfoster1 September 10, 2024 17:38

sfoster1 requested changes Sep 10, 2024

View reviewed changes

robot-server/robot_server/persistence/_migrations/v6_to_v7.py Show resolved Hide resolved

robot-server/robot_server/runs/run_store.py Outdated Show resolved Hide resolved

robot-server/tests/integration/http_api/persistence/test_compatibility.py Outdated Show resolved Hide resolved

TamarZanzouri and others added 4 commits September 10, 2024 13:53

lint

1d4d990

Update robot-server/robot_server/persistence/_migrations/v6_to_v7.py

8e729f2

Co-authored-by: Seth Foster <[email protected]>

fix non null insert and pr feedback

0106301

remove null check

2d7113b

TamarZanzouri requested review from sfoster1 and SyntaxColoring September 10, 2024 19:24

removed comment

bc71142

sfoster1 approved these changes Sep 10, 2024

View reviewed changes

Merge branch 'edge' into EXEC-686-filter-fixit-commands-out-of-the-co…

2e45299

…mmands-list

vegano1 reviewed Sep 10, 2024

View reviewed changes

load json

7355843

vegano1 reviewed Sep 11, 2024

View reviewed changes

added missing tables in migration

cb1b6c5

TamarZanzouri requested a review from vegano1 September 11, 2024 17:01

sanni-t requested changes Sep 11, 2024

View reviewed changes

TamarZanzouri requested a review from sanni-t September 11, 2024 18:09

copy whole db, add column and update value

0f9382d

lint

85ee3f6

sanni-t approved these changes Sep 12, 2024

View reviewed changes

TamarZanzouri added 2 commits September 13, 2024 11:52

remove source table

7722510

typo

f15ae9c

TamarZanzouri merged commit b2ba38e into edge Sep 13, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(robot-server, api): filter fixit commands out of the commands list #16075

chore(robot-server, api): filter fixit commands out of the commands list #16075

TamarZanzouri commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

SyntaxColoring left a comment

sfoster1 left a comment

sfoster1 left a comment

vegano1 Sep 10, 2024

sanni-t Sep 11, 2024

TamarZanzouri Sep 11, 2024 •

edited

Loading

TamarZanzouri Sep 11, 2024 •

edited

Loading

vegano1 left a comment

sanni-t left a comment

sanni-t Sep 11, 2024

sanni-t Sep 11, 2024

TamarZanzouri Sep 11, 2024

sanni-t Sep 11, 2024

TamarZanzouri Sep 11, 2024

TamarZanzouri commented Sep 12, 2024

sanni-t left a comment

sanni-t Sep 12, 2024

TamarZanzouri Sep 13, 2024

chore(robot-server, api): filter fixit commands out of the commands list #16075

chore(robot-server, api): filter fixit commands out of the commands list #16075

Conversation

TamarZanzouri commented Aug 20, 2024 • edited Loading

Overview

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

SyntaxColoring left a comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TamarZanzouri Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

TamarZanzouri Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

vegano1 left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TamarZanzouri commented Sep 12, 2024

sanni-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TamarZanzouri commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

TamarZanzouri Sep 11, 2024 •

edited

Loading

TamarZanzouri Sep 11, 2024 •

edited

Loading