feat(robot-server): start logic for dynamic error recovery policy #15707
Conversation
Create robot-server function that turns a List of ErrorRecoveryRules into a full ErrorRecovery policy. A future PR will implement the HTTP calls that actually create the list of rules. EXEC-589
| from typing import Optional | ||
|
|
||
|
|
||
| def create_error_recovery_policy_from_rules( |
There was a problem hiding this comment.
Looks good, but can we add a unit test for this?
Fortunately it's the fun (to me) kind of unit test. No mocks, just lots of black-box input and output, living in the moment, aah.
There was a problem hiding this comment.
Unfortunately I had to resort to mocks to create fake errors/commands/etc. If you think there's a cleaner or better way to do it just let me know
There was a problem hiding this comment.
I would just construct actual full errors/command/etc. objects. Possibly with helper functions to make it more convenient.
This is a good guideline, in general:
| Object type | Usage in tests |
|---|---|
| "Behavioral" (it exists for its methods) | Replace the object and its methods with test doubles (mocks) |
| "Data" (it exists to group stuff together) | Actually construct actual objects, in full |
But, you know, this only holds up to a certain point. There's a threshold where a data object gets so big and annoying to construct, that it clutters up the tests, and you would rather just create the parts that you really need, using something like a mock. I feel like our JSON protocol and labware definition models are like that, for example. Personally, the objects in this PR don't meet my threshold for that, but I can certainly see how they would.
But I think this works, no need to change it.
There was a problem hiding this comment.
Looks good, thanks.
We can update the NotImplementedError comment and I think there's a missing == on one if the assert statements, but other than that, this looks good to me to merge. Feel free to merge after fixing those and checking that all the CI is still passing.
| from typing import Optional | ||
|
|
||
|
|
||
| def create_error_recovery_policy_from_rules( |
There was a problem hiding this comment.
I would just construct actual full errors/command/etc. objects. Possibly with helper functions to make it more convenient.
This is a good guideline, in general:
| Object type | Usage in tests |
|---|---|
| "Behavioral" (it exists for its methods) | Replace the object and its methods with test doubles (mocks) |
| "Data" (it exists to group stuff together) | Actually construct actual objects, in full |
But, you know, this only holds up to a certain point. There's a threshold where a data object gets so big and annoying to construct, that it clutters up the tests, and you would rather just create the parts that you really need, using something like a mock. I feel like our JSON protocol and labware definition models are like that, for example. Personally, the objects in this PR don't meet my threshold for that, but I can certainly see how they would.
But I think this works, no need to change it.
| class ErrorRecoveryRule(BaseModel): | ||
| """Request/Response model for new error recovery rule creation.""" | ||
|
|
||
| matchCriteria: list[MatchCriteria] = Field( | ||
| default_factory=list, | ||
| description="The criteria that must be met for this rule to be applied.", | ||
| ) | ||
| ifMatch: list[ReactionIfMatch] = Field( | ||
| default_factory=list, | ||
| description="The specific recovery setting that will be in use if the type parameters match.", | ||
| ) |
There was a problem hiding this comment.
OK so it looks like this is modeling a structure that's like this (in loose sketchy form):
rules
matchCriteria
criteria 1 (with one commandType and one errorType)
criteria 2 (with one commandType and one errorType)
criteria 3 (with one commandType and one errorType)
ifMatch
reaction 1
reaction 2
reaction 3
Where criteria 1 gets paired with reaction 1, criteria 2 gets paired with reaction 2, etc.
I think instead, we want:
rules
rule 1
matchCriteria (with one commandType and one errorType)
ifMatch reaction
rule 2
matchCriteria (with one commandType and one errorType)
ifMatch reaction
But I think we can merge this as-is and address it separately. On its own terms, this looks correctly-implemented, and this PR nicely unblocks both the HTTP API work and the internal opentrons.protocol_engine work.
|
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
Overview
Create robot-server function that turns a List of ErrorRecoveryRules into a full ErrorRecoveryPolicy. A future PR will implement the HTTP calls that actually create the list of rules.
EXEC-589
Test Plan
Changelog
Review requests
Risk assessment