Skip to content

Commit

Permalink
set instead of create, remove async and more pr feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
TamarZanzouri committed Jul 26, 2024
1 parent fe6bc3b commit 04dd5c8
Show file tree
Hide file tree
Showing 8 changed files with 29 additions and 45 deletions.
2 changes: 1 addition & 1 deletion api/src/opentrons/protocol_engine/protocol_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,7 @@ def set_and_start_queue_worker(
)
self._queue_worker.start()

async def set_error_recovery_policy(self, policy: ErrorRecoveryPolicy) -> None:
def set_error_recovery_policy(self, policy: ErrorRecoveryPolicy) -> None:
"""Set error recovery policy for run."""
raise NotImplementedError("set_error_recovery_policy is not implemented yet")

Expand Down
4 changes: 2 additions & 2 deletions api/src/opentrons/protocol_runner/run_orchestrator.py
Original file line number Diff line number Diff line change
Expand Up @@ -359,9 +359,9 @@ def get_deck_type(self) -> DeckType:
"""Get engine deck type."""
return self._protocol_engine.state_view.config.deck_type

async def create_error_recovery_policy(self, policy: ErrorRecoveryPolicy) -> None:
def set_error_recovery_policy(self, policy: ErrorRecoveryPolicy) -> None:
"""Create error recovery policy for the run."""
await self._protocol_engine.set_error_recovery_policy(policy)
self._protocol_engine.set_error_recovery_policy(policy)

async def command_generator(self) -> AsyncGenerator[str, None]:
"""Yield next command to execute."""
Expand Down
4 changes: 2 additions & 2 deletions api/tests/opentrons/protocol_runner/test_run_orchestrator.py
Original file line number Diff line number Diff line change
Expand Up @@ -528,5 +528,5 @@ async def test_create_error_recovery_policy(
) -> None:
"""Should call PE set_error_recovery_policy."""
policy = decoy.mock(cls=ErrorRecoveryPolicy)
await live_protocol_subject.create_error_recovery_policy(policy)
decoy.verify(await mock_protocol_engine.set_error_recovery_policy(policy))
live_protocol_subject.set_error_recovery_policy(policy)
decoy.verify(mock_protocol_engine.set_error_recovery_policy(policy))
19 changes: 10 additions & 9 deletions robot-server/robot_server/runs/router/base_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -366,12 +366,12 @@ async def update_run(


@PydanticResponse.wrap_route(
base_router.post,
path="/runs/{runId}/policies",
summary="Create run policies",
base_router.put,
path="/runs/{runId}/errorRecoveryPolicies",
summary="Set run policies",
description=dedent(
"""
Create run policies for error recovery.
Set run policies for error recovery.
"""
),
status_code=status.HTTP_201_CREATED,
Expand All @@ -380,22 +380,23 @@ async def update_run(
status.HTTP_409_CONFLICT: {"model": ErrorBody[RunStopped]},
},
)
async def create_run_policies(
async def set_run_policies(
runId: str,
request_body: Optional[RequestModel[List[ErrorRecoveryRule]]] = None,
request_body: RequestModel[List[ErrorRecoveryRule]],
run_data_manager: RunDataManager = Depends(get_run_data_manager),
) -> PydanticResponse[SimpleEmptyBody]:
"""Create run polices.
Arguments:
runId: Run ID pulled from URL.
request_body: Optional request body with run creation data.
request_body: Request body with run policies data.
run_data_manager: Current and historical run data management.
"""
policies = request_body.data if request_body is not None else None
policies = request_body.data
print(policies)
if policies:
try:
await run_data_manager.create_policies(run_id=runId, policies=policies)
run_data_manager.set_policies(run_id=runId, policies=policies)
except RunNotCurrentError as e:
raise RunStopped(detail=str(e)).as_error(status.HTTP_409_CONFLICT) from e

Expand Down
6 changes: 2 additions & 4 deletions robot-server/robot_server/runs/run_data_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -435,9 +435,7 @@ def get_all_commands_as_preserialized_list(self, run_id: str) -> List[str]:
)
return self._run_store.get_all_commands_as_preserialized_list(run_id)

async def create_policies(
self, run_id: str, policies: List[ErrorRecoveryRule]
) -> None:
def set_policies(self, run_id: str, policies: List[ErrorRecoveryRule]) -> None:
"""Create run policy rules for error recovery."""
if run_id != self._run_orchestrator_store.current_run_id:
raise RunNotCurrentError(
Expand All @@ -446,7 +444,7 @@ async def create_policies(
policy = error_recovery_mapping.create_error_recovery_policy_from_rules(
policies
)
await self._run_orchestrator_store.create_error_recovery_policy(policy=policy)
self._run_orchestrator_store.set_error_recovery_policy(policy=policy)

def _get_state_summary(self, run_id: str) -> Union[StateSummary, BadStateSummary]:
if run_id == self._run_orchestrator_store.current_run_id:
Expand Down
4 changes: 2 additions & 2 deletions robot-server/robot_server/runs/run_orchestrator_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -360,9 +360,9 @@ def add_labware_definition(self, definition: LabwareDefinition) -> LabwareUri:
"""Add a new labware definition to state."""
return self.run_orchestrator.add_labware_definition(definition)

async def create_error_recovery_policy(self, policy: ErrorRecoveryPolicy) -> None:
def set_error_recovery_policy(self, policy: ErrorRecoveryPolicy) -> None:
"""Create run policy rules for error recovery."""
await self.run_orchestrator.create_error_recovery_policy(policy)
self.run_orchestrator.set_error_recovery_policy(policy)

async def add_command_and_wait_for_interval(
self,
Expand Down
27 changes: 7 additions & 20 deletions robot-server/tests/runs/router/test_base_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
get_runs,
remove_run,
update_run,
create_run_policies,
set_run_policies,
)

from robot_server.deck_configuration.store import DeckConfigurationStore
Expand Down Expand Up @@ -580,25 +580,12 @@ async def test_create_policies(
) -> None:
"""It should call RunDataManager create run policies."""
policies = decoy.mock(cls=List[ErrorRecoveryRule])
await create_run_policies(
await set_run_policies(
runId="rud-id",
run_data_manager=mock_run_data_manager,
request_body=RequestModel(data=policies),
run_data_manager=mock_run_data_manager,
)
decoy.verify(
await mock_run_data_manager.create_policies(run_id="rud-id", policies=policies)
)


# async def test_create_policies_raises_no_policies(
# decoy: Decoy, mock_run_data_manager: RunDataManager
# ) -> None:
# """It should raise that no policies were accepted."""
# policies = decoy.mock(cls=List[ErrorRecoveryRule])
# await create_run_policies(runId="rud-id", request_body=RequestModel(data=policies))
# decoy.verify(
# mock_run_data_manager.create_policies(run_id="rud-id", policies=policies)
# )
decoy.verify(mock_run_data_manager.set_policies(run_id="rud-id", policies=policies))


async def test_create_policies_raises_not_active_run(
Expand All @@ -607,13 +594,13 @@ async def test_create_policies_raises_not_active_run(
"""It should raise that the run is not current."""
policies = decoy.mock(cls=List[ErrorRecoveryRule])
decoy.when(
await mock_run_data_manager.create_policies(run_id="rud-id", policies=policies)
mock_run_data_manager.set_policies(run_id="rud-id", policies=policies)
).then_raise(RunNotCurrentError())
with pytest.raises(ApiError) as exc_info:
await create_run_policies(
await set_run_policies(
runId="rud-id",
run_data_manager=mock_run_data_manager,
request_body=RequestModel(data=policies),
run_data_manager=mock_run_data_manager,
)

assert exc_info.value.status_code == 409
Expand Down
8 changes: 3 additions & 5 deletions robot-server/tests/runs/test_run_data_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,7 @@ async def test_create_policies_raises_run_not_current(
"not-current-run-id"
)
with pytest.raises(RunNotCurrentError):
await subject.create_policies(
subject.set_policies(
run_id="run-id", policies=decoy.mock(cls=List[ErrorRecoveryRule])
)

Expand All @@ -1040,7 +1040,5 @@ async def test_create_policies_translates_and_calls_orchestrator(
error_recovery_mapping.create_error_recovery_policy_from_rules(input_rules)
).then_return(expected_output)
decoy.when(mock_run_orchestrator_store.current_run_id).then_return("run-id")
await subject.create_policies(run_id="run-id", policies=input_rules)
decoy.verify(
await mock_run_orchestrator_store.create_error_recovery_policy(expected_output)
)
subject.set_policies(run_id="run-id", policies=input_rules)
decoy.verify(mock_run_orchestrator_store.set_error_recovery_policy(expected_output))

0 comments on commit 04dd5c8

Please sign in to comment.