Add procedure completed actions and undo #228
Conversation
imaitland
commented
Dec 10, 2024
•
imaitland
commented
- The front end must display the current progress of the procedure. This PR adds a "completed_actions" property.
- The front end must support undoing actions. This PR adds an "undo" endpoint".
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
imaitland
force-pushed
the
iainland/add-procedure-completed-actions-and-undo
branch
from
b7c60a1 to
acac5bb
Compare
evolver/app/routers/hardware.py
Outdated
| calibrator = hardware_instance.calibrator | ||
| if not calibrator: | ||
| raise CalibratorNotFoundError |
There was a problem hiding this comment.
[minor] Would be a good place for walrus!
if not (calibrator := hardware_instance.calibrator):
raise CalibratorNotFoundError| dispatch_action(client, "test", "measure_vial_0_temperature", {"temperature": 25.0}) | ||
| dispatch_action(client, "test", "read_vial_0_raw_output") | ||
| dispatch_action(client, "test", "calculate_vial_0_fit") |
There was a problem hiding this comment.
I think this is fine for now, but we might want to make a note to create a mock procedure for the tests which cover action kinds (e.g. ones that need input, ones that don't, etc.), iso using the temperature one - just because this looks more generic
There was a problem hiding this comment.
yeah i agree, created an issue.#231
evolver/calibration/interface.py
Outdated
| @@ -193,5 +193,5 @@ class Config(Calibrator.Config): | |||
| input_transformer: dict[int, ConfigDescriptor | Transformer | None] | None = None | |||
| output_transformer: dict[int, ConfigDescriptor | Transformer | None] | None = None | |||
|
|
|||
| def create_calibration_procedure(self, selected_hardware, *args, **kwargs): | |||
| def create_calibration_procedure(self, selected_hardware, resume, *args, **kwargs): | |||
There was a problem hiding this comment.
I know this is not added in this PR, but I don't think we even need this method here. It is abstract in the base and we can leave it out to have consistent error (TypeError from the ABC) - plus satisfying to remove code!
There was a problem hiding this comment.
Ok, cool will fix.
| @@ -19,14 +19,26 @@ class TemperatureCalibrator(IndependentVialBasedCalibrator): | |||
|
|
|||
| class CalibrationData(Transformer.Config): | |||
| measured: Dict[int, Dict[str, List[float]]] = {} # {vial_index: {"reference": [], "raw": []}} | |||
| completed_actions: List[str] = [] | |||
There was a problem hiding this comment.
Since completed_actions looks like something we want to support at the interface, perhaps this should actually go in Transformer.Config (or actually I think better yet: this class should be based off Calibrator.CalibrationData and that should have completed_actions)
There was a problem hiding this comment.
makes sense, will fix.
| This can be done by defining a SaveProcedureState action in the procedure and dispatching it. | ||
| """ | ||
| super().__init__(*args, **kwargs) | ||
| self.actions = [] | ||
| self.state = {} | ||
| self.state = CalibrationStateModel(**(state or {})).model_dump() |
There was a problem hiding this comment.
looking at this and the setdefaults done below, I wonder if we rather could benefit from state being CalibrationStateModel iso Dict and make the model allow extra fields.
class CalibrationStateModel(BaseModel):
model_config = ConfigDict(extra='allow')
completed_actions: List[str] = Field(default_factory=list)This way instantiating will always make the default empty list if not supplied, and user doesn't have to state explicitly deal with that. Also, I think without the extra being "allow"ed, this part might drop the things in **state that are not completed_actions
…ract methods properly and explicitly communicate IndependentVialBasedCalibrator should be used as a base class, not by itself
imaitland
force-pushed
the
iainland/add-procedure-completed-actions-and-undo
branch
from
acac5bb to
40b7e50
Compare
imaitland
deleted the
iainland/add-procedure-completed-actions-and-undo
branch
