-
Notifications
You must be signed in to change notification settings - Fork 180
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.
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
fix(api): filter get_run_commands #16001
Changes from all commits
77dc730
27ae9bd
58703ad
4f29706
e93f539
cbf3882
2b1fc36
27d4dcb
cefd851
5e80365
1e1c08a
b6962ee
c1d580c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,9 +101,14 @@ def get_all_ids(self) -> List[str]: | |
"""Get all command IDs.""" | ||
return self._all_command_ids | ||
|
||
def get_slice(self, start: int, stop: int) -> List[Command]: | ||
def get_slice( | ||
self, start: int, stop: int, command_ids: Optional[list[str]] = None | ||
) -> List[Command]: | ||
"""Get a list of commands between start and stop.""" | ||
commands = self._all_command_ids[start:stop] | ||
selected_command_ids = ( | ||
command_ids if command_ids is not None else self._all_command_ids | ||
) | ||
commands = selected_command_ids[start:stop] | ||
return [self._commands_by_id[command].command for command in commands] | ||
|
||
def get_tail_command(self) -> Optional[CommandEntry]: | ||
|
@@ -127,6 +132,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] = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we change this to bool and add the logic within? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 So I guess let's focus on that question and then come back to this. |
||
CommandIntent.PROTOCOL, | ||
CommandIntent.SETUP, | ||
], | ||
) -> List[str]: | ||
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()) | ||
Comment on lines
+142
to
+155
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
def get_queue_ids(self) -> OrderedSet[str]: | ||
"""Get the IDs of all queued protocol commands, in FIFO order.""" | ||
return self._queued_command_ids | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -580,18 +580,25 @@ def get_all(self) -> List[Command]: | |
return self._state.command_history.get_all_commands() | ||
|
||
def get_slice( | ||
self, | ||
cursor: Optional[int], | ||
length: int, | ||
self, cursor: Optional[int], length: int, include_fixit_commands: bool | ||
) -> CommandSlice: | ||
"""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. | ||
""" | ||
command_ids = self._state.command_history.get_filtered_command_ids( | ||
command_intents=[ | ||
CommandIntent.PROTOCOL, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. kind of hate this. should we just change this to a list of intents in robot server? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm totally neutral about it, but I think this also depends on the performance questions in |
||
CommandIntent.SETUP, | ||
CommandIntent.FIXIT, | ||
] | ||
if include_fixit_commands | ||
else [CommandIntent.PROTOCOL, CommandIntent.SETUP] | ||
) | ||
running_command = self._state.command_history.get_running_command() | ||
queued_command_ids = self._state.command_history.get_queue_ids() | ||
total_length = self._state.command_history.length() | ||
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? | ||
|
@@ -620,7 +627,10 @@ def get_slice( | |
# start is inclusive, stop is exclusive | ||
actual_cursor = max(0, min(cursor, total_length - 1)) | ||
stop = min(total_length, actual_cursor + length) | ||
commands = self._state.command_history.get_slice(start=actual_cursor, stop=stop) | ||
|
||
commands = self._state.command_history.get_slice( | ||
start=actual_cursor, stop=stop, command_ids=command_ids | ||
) | ||
|
||
return CommandSlice( | ||
commands=commands, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -904,7 +904,7 @@ def test_get_current() -> None: | |
def test_get_slice_empty() -> None: | ||
"""It should return a slice from the tail if no current command.""" | ||
subject = get_command_view(commands=[]) | ||
result = subject.get_slice(cursor=0, length=2) | ||
result = subject.get_slice(cursor=0, length=2, include_fixit_commands=True) | ||
|
||
assert result == CommandSlice(commands=[], cursor=0, total_length=0) | ||
|
||
|
@@ -918,15 +918,15 @@ def test_get_slice() -> None: | |
|
||
subject = get_command_view(commands=[command_1, command_2, command_3, command_4]) | ||
|
||
result = subject.get_slice(cursor=1, length=3) | ||
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=4, | ||
) | ||
|
||
result = subject.get_slice(cursor=-3, length=10) | ||
result = subject.get_slice(cursor=-3, length=10, include_fixit_commands=True) | ||
|
||
assert result == CommandSlice( | ||
commands=[command_1, command_2, command_3, command_4], | ||
|
@@ -944,7 +944,7 @@ def test_get_slice_default_cursor_no_current() -> None: | |
|
||
subject = get_command_view(commands=[command_1, command_2, command_3, command_4]) | ||
|
||
result = subject.get_slice(cursor=None, length=3) | ||
result = subject.get_slice(cursor=None, length=3, include_fixit_commands=True) | ||
|
||
assert result == CommandSlice( | ||
commands=[command_2, command_3, command_4], | ||
|
@@ -975,7 +975,7 @@ def test_get_slice_default_cursor_failed_command() -> None: | |
failed_command=CommandEntry(index=2, command=command_3), | ||
) | ||
|
||
result = subject.get_slice(cursor=None, length=3) | ||
result = subject.get_slice(cursor=None, length=3, include_fixit_commands=True) | ||
|
||
assert result == CommandSlice( | ||
commands=[command_3, command_4], | ||
|
@@ -997,7 +997,7 @@ def test_get_slice_default_cursor_running() -> None: | |
running_command_id="command-id-3", | ||
) | ||
|
||
result = subject.get_slice(cursor=None, length=2) | ||
result = subject.get_slice(cursor=None, length=2, include_fixit_commands=True) | ||
|
||
assert result == CommandSlice( | ||
commands=[command_3, command_4], | ||
|
@@ -1006,6 +1006,51 @@ def test_get_slice_default_cursor_running() -> None: | |
) | ||
|
||
|
||
def test_get_slice_without_fixit() -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add this in This is a lower-priority request because the |
||
"""It should select a cursor based on the running command, if present.""" | ||
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") | ||
command_4 = create_queued_command(command_id="command-id-4") | ||
command_5 = create_queued_command(command_id="command-id-5") | ||
command_6 = create_queued_command( | ||
command_id="fixit-id-1", intent=cmd.CommandIntent.FIXIT | ||
) | ||
command_7 = create_queued_command( | ||
command_id="fixit-id-2", intent=cmd.CommandIntent.FIXIT | ||
) | ||
|
||
subject = get_command_view( | ||
commands=[ | ||
command_1, | ||
command_2, | ||
command_3, | ||
command_4, | ||
command_5, | ||
command_6, | ||
command_7, | ||
], | ||
queued_command_ids=[ | ||
"command-id-1", | ||
"command-id-2", | ||
"command-id-3", | ||
"command-id-4", | ||
"command-id-5", | ||
"fixit-id-1", | ||
"fixit-id-2", | ||
], | ||
queued_fixit_command_ids=["fixit-id-1", "fixit-id-2"], | ||
) | ||
|
||
result = subject.get_slice(cursor=None, length=7, include_fixit_commands=False) | ||
|
||
assert result == CommandSlice( | ||
commands=[command_1, command_2, command_3, command_4, command_5], | ||
cursor=0, | ||
total_length=5, | ||
) | ||
|
||
|
||
def test_get_errors_slice_empty() -> None: | ||
"""It should return a slice from the tail if no current command.""" | ||
subject = get_command_view(failed_command_errors=[]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add protocol intent filter? probably?