fix(api): filter get_run_commands #16001

TamarZanzouri · 2024-08-14T19:21:54Z

Overview

closes EXEC-654.
get filtered command list

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.

Changelog

added an arg allCommands to get_run_commands.
add logic to CommandState and CommandHistory to filter based on the protocol intent.

Review requests

changes make sense? how ugly is this?

Risk assessment

Medium. need to make sure nothing that uses run commands changed the default behavior.

TamarZanzouri · 2024-08-14T19:23:01Z

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

@@ -127,6 +130,28 @@ def get_running_command(self) -> Optional[CommandEntry]:
        else:
            return self._commands_by_id[self._running_command_id]

+    def get_filtered_command_ids(


should we add protocol intent filter? probably?

TamarZanzouri · 2024-08-14T19:23:37Z

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

-        total_length = self._state.command_history.length()
+        command_ids = self._state.command_history.get_filtered_command_ids(
+            command_intents=[
+                CommandIntent.PROTOCOL,


kind of hate this. should we just change this to a list of intents in robot server?

I'm totally neutral about it, but I think this also depends on the performance questions in CommandHistory (see my other comment). So let's come back to this.

TamarZanzouri · 2024-08-14T20:04:17Z

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

@@ -127,6 +130,28 @@ def get_running_command(self) -> Optional[CommandEntry]:
        else:
            return self._commands_by_id[self._running_command_id]

+    def get_filtered_command_ids(
+        self,
+        command_intents: list[CommandIntent] = [


should we change this to bool and add the logic within?

I think it depends on how the performance questions (in my other comment) get solved. You may find that it's too much work to get it to accept arbitrary CommandIntents, and we can only filter FIXIT for now.

So I guess let's focus on that question and then come back to this.

SyntaxColoring

This all makes sense to me from a correctness standpoint. I am concerned about performance, though, so let's test to make sure we're good there, before merging this.

robot-server/robot_server/runs/router/commands_router.py

robot-server/robot_server/maintenance_runs/maintenance_run_orchestrator_store.py

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

SyntaxColoring · 2024-08-14T20:18:16Z

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

@@ -127,6 +130,28 @@ def get_running_command(self) -> Optional[CommandEntry]:
        else:
            return self._commands_by_id[self._running_command_id]

+    def get_filtered_command_ids(
+        self,
+        command_intents: list[CommandIntent] = [


I think it depends on how the performance questions (in my other comment) get solved. You may find that it's too much work to get it to accept arbitrary CommandIntents, and we can only filter FIXIT for now.

So I guess let's focus on that question and then come back to this.

SyntaxColoring · 2024-08-14T20:29:08Z

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

+        filtered_command = self._commands_by_id
+        if CommandIntent.FIXIT not in command_intents:
+            filtered_command = {
+                key: val
+                for key, val in self._commands_by_id.items()
+                if val.command.intent != CommandIntent.FIXIT
+            }
+        if CommandIntent.SETUP not in command_intents:
+            filtered_command = {
+                key: val
+                for key, val in filtered_command.items()
+                if val.command.intent != CommandIntent.SETUP
+            }
+        return list(filtered_command.keys())


I'm concerned about the performance of this implementation.

Formerly, if an HTTP client asked to retrieve n commands, we could do it in average O(n) time. Now, if a client asks to retrieve n filtered commands, it will take O(m) time, where m is the number of total commands in the run. This could be quite bad if the run is long and the client is polling us for the run log.

I think before merging, we need to confirm with testing that response times will stay acceptable even with, say, a 10,000-command protocol. (I'm getting the number 10,000 from here—it's pessimistic but realistic.)

A more performant way to implement this will probably involve some kind of speedup index, like we do for _queued_setup_command_ids, etc. It may also involve combining the filtering and slicing steps.

I agree. I told Jamey that I am concerned from the performance as well. The problem is that we have a fixit_queued_commad_ids but once it's executed it's not in the queue anymore, it's just in the commands queue. This was my first implementation but found bugs related to that. I will explore other options.

SyntaxColoring · 2024-08-14T20:34:52Z

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

-        total_length = self._state.command_history.length()
+        command_ids = self._state.command_history.get_filtered_command_ids(
+            command_intents=[
+                CommandIntent.PROTOCOL,


I'm totally neutral about it, but I think this also depends on the performance questions in CommandHistory (see my other comment). So let's come back to this.

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

api/tests/opentrons/protocol_engine/state/test_command_history.py

SyntaxColoring · 2024-08-14T20:45:45Z

api/tests/opentrons/protocol_engine/state/test_command_view_old.py

@@ -1006,6 +1006,51 @@ def test_get_slice_default_cursor_running() -> None:
    )


+def test_get_slice_without_fixit() -> None:


Can we add this in test_command_state instead, if you have time? For the reasons described at the top of this file.

This is a lower-priority request because the CommandView.get_slice() tests have a lot of confusing overlap with the ones for CommandHistory and I would not mind if we just ignored it for now. :^)

Co-authored-by: Max Marrone <[email protected]>

TamarZanzouri · 2024-08-16T14:27:51Z

Decided to go with a FE change for now. I backend change will involve DB changes. Decided to punt on this for now.
see https://opentrons.slack.com/archives/C06M9F26YGH/p1723753502607629 for context.

…ist (#16075) # Overview reference #16001 for previous pr comments. closes [EXEC-654](https://opentrons.atlassian.net/browse/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. [EXEC-654]: https://opentrons.atlassian.net/browse/EXEC-654?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: Seth Foster <[email protected]>

TamarZanzouri added 6 commits August 12, 2024 16:34

robot server get all commands WIP

77dc730

get filtered list

27ae9bd

get filtered list and tests

58703ad

get filtered slice and_current for cursor

4f29706

cleanup and logic fix

e93f539

added tests

cbf3882

TamarZanzouri commented Aug 14, 2024

View reviewed changes

reverted get_current

2b1fc36

TamarZanzouri changed the title ~~Exec 654 unsafe drop in place appears in run preview~~ fix(api): filter get_run_commands Aug 14, 2024

lint

27d4dcb

TamarZanzouri marked this pull request as ready for review August 14, 2024 20:03

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

TamarZanzouri requested review from mjhuff and SyntaxColoring and removed request for a team August 14, 2024 20:03

TamarZanzouri commented Aug 14, 2024

View reviewed changes

SyntaxColoring reviewed Aug 14, 2024

View reviewed changes

TamarZanzouri and others added 5 commits August 15, 2024 13:46

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

cefd851

Co-authored-by: Max Marrone <[email protected]>

include_fixit_commands

5e80365

includeFixitCommands

1e1c08a

removed todo

b6962ee

change test to not use private methods. cursor rename

c1d580c

TamarZanzouri closed this Aug 16, 2024

TamarZanzouri mentioned this pull request Aug 20, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): filter get_run_commands #16001

fix(api): filter get_run_commands #16001

TamarZanzouri commented Aug 14, 2024 •

edited by jira bot

Loading

TamarZanzouri Aug 14, 2024

TamarZanzouri Aug 14, 2024

SyntaxColoring Aug 14, 2024

TamarZanzouri Aug 14, 2024

SyntaxColoring Aug 14, 2024

SyntaxColoring left a comment

SyntaxColoring Aug 14, 2024

SyntaxColoring Aug 14, 2024 •

edited

Loading

TamarZanzouri Aug 14, 2024

SyntaxColoring Aug 14, 2024

SyntaxColoring Aug 14, 2024 •

edited

Loading

TamarZanzouri commented Aug 16, 2024

		@@ -1006,6 +1006,51 @@ def test_get_slice_default_cursor_running() -> None:
		)


		def test_get_slice_without_fixit() -> None:

fix(api): filter get_run_commands #16001

fix(api): filter get_run_commands #16001

Conversation

TamarZanzouri commented Aug 14, 2024 • edited by jira bot Loading

Overview

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

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

SyntaxColoring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxColoring Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxColoring Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

TamarZanzouri commented Aug 16, 2024

TamarZanzouri commented Aug 14, 2024 •

edited by jira bot

Loading

SyntaxColoring Aug 14, 2024 •

edited

Loading

SyntaxColoring Aug 14, 2024 •

edited

Loading