Skip to content

fix(robot-server, api): Standardize command slice behavior #18203

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 29, 2025
Merged

fix(robot-server, api): Standardize command slice behavior #18203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a58ce6b
refactor(api): standardize command slice behavior
mjhuff Apr 29, 2025
3e13c38
refactor(robot-server): standardize error slicing behavior for curren…
mjhuff Apr 29, 2025
7b61fdd
T A V E R N
mjhuff Apr 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 14 additions & 28 deletions api/src/opentrons/protocol_engine/state/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,49 +619,35 @@ def get_slice(
"""Get a subset of commands around a given cursor.

If the cursor is omitted, a cursor will be selected automatically
based on the currently running or most recently executed command.
based on the currently running or most recently executed command,
and the slice of commands returned is the previous `length` commands
inclusive of the currently running or most recently executed command.
"""
command_ids = self._state.command_history.get_filtered_command_ids(
include_fixit_commands=include_fixit_commands
)
running_command = self._state.command_history.get_running_command()
queued_command_ids = self._state.command_history.get_queue_ids()
total_length = len(command_ids)

# TODO(mm, 2024-05-17): This looks like it's attempting to do the same thing
# as self.get_current(), but in a different way. Can we unify them?
if cursor is None:
if running_command is not None:
cursor = running_command.index
elif len(queued_command_ids) > 0:
# Get the most recently executed command,
# which we can find just before the first queued command.
cursor = (
self._state.command_history.get(queued_command_ids.head()).index - 1
)
elif (
self._state.run_result
and self._state.run_result == RunResult.FAILED
and self._state.failed_command
):
# Currently, if the run fails, we mark all the commands we didn't
# reach as failed. This makes command status alone insufficient to
# find the most recent command that actually executed, so we need to
# store that separately.
cursor = self._state.failed_command.index
current_pointer = self.get_current()
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember we wanted to do this for a while but we avoided it bc the logic was different? I dont remember why @SyntaxColoring do you remember?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is fundamentally different, at least from my testing and reading, but I've been wrong on at least a couple occasions 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at the implementation again and it does seem correct. I remember wanting to do this change but there was a difference I cannot remember :-) if this was tested then should be fine.

Copy link
Contributor

@TamarZanzouri TamarZanzouri Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the change I am seeing is in case the last command is a failed command and the queue is empty.
We use the failed_command prop for this case, where with get_current() we loop through the command list to get the last command executed -> they will probably be the same value. also, we are looping though command ids while using get_current and queued commands in the old slice. I think this change is fine though bc the results should be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool cool, thank you for double checking!

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing that was always holding me back was the behavioral changes revealed in the test diffs here. It wasn't clear to me that those changes were safe to make, with respect to client expectations. If you've audited the client and it all looks good to you, then yeah, this is great!


if current_pointer is not None:
cursor = current_pointer.index
else:
cursor = total_length - length
cursor = total_length - 1

cursor = max(cursor - length + 1, 0)

# start is inclusive, stop is exclusive
actual_cursor = max(0, min(cursor, total_length - 1))
stop = min(total_length, actual_cursor + length)
start = max(0, min(cursor, total_length - 1))
stop = min(total_length, start + length)
commands = self._state.command_history.get_slice(
start=actual_cursor, stop=stop, command_ids=command_ids
start=start, stop=stop, command_ids=command_ids
)

return CommandSlice(
commands=commands,
cursor=actual_cursor,
cursor=start,
total_length=total_length,
)

Expand Down
51 changes: 46 additions & 5 deletions api/tests/opentrons/protocol_engine/state/test_command_view_old.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,8 +1000,8 @@ def test_get_slice_default_cursor_failed_command() -> None:
result = subject.get_slice(cursor=None, length=3, include_fixit_commands=True)

assert result == CommandSlice(
commands=[command_3, command_4],
cursor=2,
commands=[command_2, command_3, command_4],
cursor=1,
total_length=4,
)

Expand All @@ -1022,14 +1022,14 @@ def test_get_slice_default_cursor_running() -> None:
result = subject.get_slice(cursor=None, length=2, include_fixit_commands=True)

assert result == CommandSlice(
commands=[command_3, command_4],
cursor=2,
commands=[command_2, command_3],
cursor=1,
total_length=5,
)


def test_get_slice_without_fixit() -> None:
"""It should select a cursor based on the running command, if present."""
"""It should filter out fixit commands when requested."""
command_1 = create_succeeded_command(command_id="command-id-1")
command_2 = create_succeeded_command(command_id="command-id-2")
command_3 = create_running_command(command_id="command-id-3")
Expand Down Expand Up @@ -1071,3 +1071,44 @@ def test_get_slice_without_fixit() -> None:
cursor=0,
total_length=5,
)


def test_get_slice_large_length() -> None:
"""It should handle cases where length is larger than available commands."""
command_1 = create_succeeded_command(command_id="command-id-1")
command_2 = create_succeeded_command(command_id="command-id-2")
command_3 = create_running_command(command_id="command-id-3")

subject = get_command_view(
commands=[command_1, command_2, command_3],
running_command_id="command-id-3",
)

result = subject.get_slice(cursor=None, length=10, include_fixit_commands=True)

assert result == CommandSlice(
commands=[command_1, command_2, command_3],
cursor=0,
total_length=3,
)


def test_get_slice_explicit_cursor_with_length() -> None:
"""It should use the cursor as the start position when explicitly provided."""
command_1 = create_succeeded_command(command_id="command-id-1")
command_2 = create_succeeded_command(command_id="command-id-2")
command_3 = create_succeeded_command(command_id="command-id-3")
command_4 = create_succeeded_command(command_id="command-id-4")
command_5 = create_succeeded_command(command_id="command-id-5")

subject = get_command_view(
commands=[command_1, command_2, command_3, command_4, command_5],
)

result = subject.get_slice(cursor=1, length=3, include_fixit_commands=True)

assert result == CommandSlice(
commands=[command_2, command_3, command_4],
cursor=1,
total_length=5,
)
Comment on lines +1074 to +1114
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the new tests!

4 changes: 3 additions & 1 deletion robot-server/robot_server/commands/router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ async def get_commands_list(
description=(
"The starting index of the desired first command in the list."
" If unspecified, a cursor will be selected automatically"
" based on the currently running or most recently executed command."
" based on the currently running or most recently executed command, "
" and the slice of commands returned is the previous `pageLength` commands"
" inclusive of the currently running or most recently executed command."
),
),
] = None,
Expand Down
4 changes: 3 additions & 1 deletion robot-server/robot_server/maintenance_runs/router/commands_router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,9 @@ async def get_run_commands(
description=(
"The starting index of the desired first command in the list."
" If unspecified, a cursor will be selected automatically"
" based on the currently running or most recently executed command."
" based on the currently running or most recently executed command, "
" and the slice of commands returned is the previous `length` commands"
" inclusive of the currently running or most recently executed command."
),
),
] = None,
Expand Down
12 changes: 5 additions & 7 deletions robot-server/robot_server/runs/router/base_router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,8 @@ async def get_run_commands_error(
description=(
"The starting index of the desired first command error in the list."
" If unspecified, a cursor will be selected automatically"
" based on the last error added."
" based on the last error added, and the slice of errors returned "
" is the previous `pageLength` errors."
),
),
] = None,
Expand All @@ -490,12 +491,9 @@ async def get_run_commands_error(
all_errors_count = run_data_manager.get_command_errors_count(run_id=runId)

if cursor is None:
if all_errors_count > 0:
# Get the most recent error,
# which we can find just at the end of the list.
cursor = all_errors_count - 1
else:
cursor = 0
cursor = max(all_errors_count - 1, 0)
cursor = max(cursor - pageLength + 1, 0)
cursor = min(cursor, all_errors_count)

command_error_slice = run_data_manager.get_command_error_slice(
run_id=runId,
Expand Down
4 changes: 3 additions & 1 deletion robot-server/robot_server/runs/router/commands_router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,9 @@ async def get_run_commands(
description=(
"The starting index of the desired first command in the list."
" If unspecified, a cursor will be selected automatically"
" based on the currently running or most recently executed command."
" based on the currently running or most recently executed command, "
" and the slice of commands returned is the previous `pageLength` commands"
" inclusive of the currently running or most recently executed command."
),
),
] = None,
Expand Down
41 changes: 31 additions & 10 deletions robot-server/tests/integration/http_api/runs/test_json_v6_run_failure.tavern.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,36 @@ stages:
key: !anystr
createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
meta:
cursor: 3
cursor: 0
totalLength: 4
data:
- id: !anystr
key: !anystr
commandType: home
createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
status: succeeded
params: !anydict
notes: !anylist
- id: !anystr
key: !anystr
commandType: loadLabware
createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
status: succeeded
params: !anydict
notes: !anylist
- id: !anystr
key: !anystr
commandType: loadPipette
createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
status: succeeded
params: !anydict
notes: !anylist
- id: !anystr
key: !anystr
commandType: aspirate
Expand All @@ -115,12 +142,6 @@ stages:
pipetteId: pipetteId
labwareId: tipRackId
wellName: A1
wellLocation:
origin: bottom
offset:
x: 0
y: 0
z: 1
volumeOffset: 0
flowRate: 3.78
volume: 100
wellLocation: !anydict
flowRate: !anyfloat
volume: !anyfloat
50 changes: 42 additions & 8 deletions robot-server/tests/integration/http_api/runs/test_papi_v2_run_failure.tavern.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,51 @@ stages:
key: !anystr
createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
meta:
cursor: 3
cursor: 0
totalLength: 4
data:
- id: !anystr
key: !anystr
commandType: home
createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
status: succeeded
params: !anydict
notes: !anylist
- id: !re_fullmatch "commands\\.LOAD_LABWARE-\\d+"
key: !re_fullmatch "commands\\.LOAD_LABWARE-\\d+"
commandType: loadLabware
createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
status: succeeded
params:
location:
slotName: "1"
loadName: opentrons_96_tiprack_300ul
namespace: opentrons
version: 1
notes: !anylist
- id: !re_fullmatch "commands\\.LOAD_PIPETTE-\\d+"
key: !re_fullmatch "commands\\.LOAD_PIPETTE-\\d+"
commandType: loadPipette
createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
status: succeeded
params:
pipetteName: p300_single
mount: right
notes: !anylist
- id: !re_fullmatch "command\\.ASPIRATE-\\d+"
key: !re_fullmatch "command\\.ASPIRATE-\\d+"
commandType: aspirate
createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
status: failed
notes: []
notes: !anylist
error:
id: !anystr
errorType: LegacyContextCommandError
Expand All @@ -119,9 +153,9 @@ stages:
wellLocation:
origin: top
offset:
x: 0
y: 0
z: 0
volumeOffset: 0
flowRate: 150
volume: 100
x: !anyfloat
y: !anyfloat
z: !anyfloat
volumeOffset: !anyfloat
flowRate: !anyfloat
volume: !anyfloat