-
Notifications
You must be signed in to change notification settings - Fork 178
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
refactor(robot-server): Use last run command for /runs/:runId/currentState
links
#16557
Changes from 4 commits
25f152f
85e625b
d2778af
b26eff9
db5dda4
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 |
---|---|---|
|
@@ -456,10 +456,20 @@ def get_current_command(self, run_id: str) -> Optional[CommandPointer]: | |
if self._run_orchestrator_store.current_run_id == run_id: | ||
return self._run_orchestrator_store.get_current_command() | ||
else: | ||
# todo(mm, 2024-05-20): | ||
# For historical runs to behave consistently with the current run, | ||
# this should be the most recently completed command, not `None`. | ||
return None | ||
return self._get_historical_run_last_command(run_id=run_id) | ||
|
||
def get_last_completed_command(self, run_id: str) -> Optional[CommandPointer]: | ||
"""Get the "last" command, if any. | ||
|
||
See `ProtocolEngine.state_view.commands.get_most_recently_finalized_command()` for the definition of "last." | ||
|
||
Args: | ||
run_id: ID of the run. | ||
""" | ||
if self._run_orchestrator_store.current_run_id == run_id: | ||
return self._run_orchestrator_store.get_most_recently_finalized_command() | ||
else: | ||
return self._get_historical_run_last_command(run_id=run_id) | ||
|
||
def get_recovery_target_command(self, run_id: str) -> Optional[CommandPointer]: | ||
"""Get the current error recovery target. | ||
|
@@ -554,3 +564,22 @@ def _get_run_time_parameters(self, run_id: str) -> List[RunTimeParameter]: | |
return self._run_orchestrator_store.get_run_time_parameters() | ||
else: | ||
return self._run_store.get_run_time_parameters(run_id=run_id) | ||
|
||
def _get_historical_run_last_command(self, run_id: str) -> Optional[CommandPointer]: | ||
command_slice = self._run_store.get_commands_slice( | ||
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. Heads up that for older historical JSON runs, which enqueued all of their commands up-front instead of one-by-one as the run proceeded, this will return the last command in the whole protocol, not the last command that actually ran. I think this is close enough that we shouldn't worry about it. |
||
run_id=run_id, cursor=None, length=1, include_fixit_commands=True | ||
) | ||
if not command_slice.commands: | ||
return None | ||
command = command_slice.commands[-1] | ||
|
||
return ( | ||
CommandPointer( | ||
command_id=command.id, | ||
command_key=command.key, | ||
created_at=command.createdAt, | ||
index=command_slice.total_length - 1, | ||
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. It should be equivalent, but you might find it clearer to do 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. Ahhh yep, that is cleaner, thanks |
||
) | ||
if command | ||
else None | ||
) |
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.
I would consider naming this
lastCompleted
instead of justlast
, in order to avoid confusion with the last queued command, which hasn't necessarily executed yet, and so is a different thing.