resume procedure endpoint #253

imaitland · 2025-02-06T18:55:52Z

feature - when a calibration procedure state has been saved (persisted to fs), it can be 'resumed' by passing the "resume" flag to the /start endpoint.

imaitland · 2025-02-06T18:56:56Z

evolver/app/routers/hardware.py

@@ -135,7 +140,7 @@ def dispatch_calibrator_action(request: Request, hardware_name: str = Path(...),

    payload = action.get("payload", {})

-    return {**calibration_procedure.dispatch(action_to_dispatch, payload), "started": True}


Refactored "started" atrribute to be common state for all procedures. It's needed as the procedure controls (UI) are distinct for an uninitialized (not started) and initialized (started) procedure.

codecov · 2025-02-06T18:57:05Z

Codecov Report

Attention: Patch coverage is 97.77778% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
evolver/calibration/action.py	66.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

imaitland · 2025-02-06T18:57:40Z

evolver/app/tests/test_calibration.py

@@ -44,7 +44,7 @@ class TestCalibration:
    def test_get_calibration_status(self):
        _, client = setup_evolver_with_calibrator(NoOpCalibrator)

-        response = client.post("/hardware/test/calibrator/procedure/start")
+        response = client.post("/hardware/test/calibrator/procedure/start", params={"resume": False})


Now the user must be explicit about their intention to attempt to resume a procedure or start a new / fresh one.

evolver/app/tests/test_calibration.py

imaitland · 2025-02-06T18:59:20Z

evolver/app/tests/test_calibration.py


    raw_dispatch_response = dispatch_action(client, "test", "read_vial_0_raw_output")

    assert raw_dispatch_response.status_code == 200
    assert raw_dispatch_response.json() == {
        "0": {"raw": [1.23], "reference": []},
        "completed_actions": ["read_vial_0_raw_output"],
-        "history": [{"completed_actions": [], "history": []}],
+        "history": [{"completed_actions": [], "history": [], "started": True}],


only downside to adding "started" to common procedure state is it appears in history.

…ns and calibration data

imaitland · 2025-02-06T20:30:58Z

evolver/calibration/interface.py

+    class CalibrationData(Transformer.Config):
+        """Stores calibration data, including the procedure_state from the calibration_procedure."""
+
+        procedure_state: CalibrationStateModel = Field(


CalibrationStateModel is now common model shared across the Procedure, Actions and CalibrationData

I like the common model, but given that we now expect the measure data to be under CalibrationData.procedure_state.measured, I'm not sure what else would ever need to go in this structure at the base level, so is there even a need to have the nesting? In other words, do we need both CalibrationData and CalibrationStateModel or are they really the same thing?

evolver/calibration/procedure.py

imaitland · 2025-02-06T20:32:12Z

evolver/calibration/procedure.py

@@ -75,29 +55,31 @@ def undo(self):
        """
        Undo the last action that was dispatched in the calibration procedure.
        """
-        if len(self.state["history"]) > 0:
-            self.state = self.state["history"].pop()
+        if len(self.state.history) > 0:


since state remains a pydantic model now across the system, we can get typed property access.

amitschang

overall looks good. Just minor comments and maybe opportunity to simplify

amitschang · 2025-02-06T21:53:52Z

evolver/calibration/interface.py

+    class CalibrationData(Transformer.Config):
+        """Stores calibration data, including the procedure_state from the calibration_procedure."""
+
+        procedure_state: CalibrationStateModel = Field(


I like the common model, but given that we now expect the measure data to be under CalibrationData.procedure_state.measured, I'm not sure what else would ever need to go in this structure at the base level, so is there even a need to have the nesting? In other words, do we need both CalibrationData and CalibrationStateModel or are they really the same thing?

amitschang · 2025-02-06T22:03:24Z

evolver/calibration/procedure.py

@@ -54,7 +33,8 @@ def __init__(self, hardware, state=None, *args, **kwargs):
        """
        super().__init__(*args, **kwargs)
        self.actions = []
-        self.state = CalibrationStateModel(**(state or {})).model_dump()
+        self.state = CalibrationStateModel(**(state or {}))


It's not wrong, but since we are working with an object, might it be cleaner to

CalibrationStateModel.model_validate(state or {})

And I didn't really catch earlier, but since the init takes state as a parameter we could default state={} there, and can pass just state to the above - unless of course we specifically need to support None being passed in.

yep will fix

amitschang · 2025-02-06T22:05:47Z

evolver/calibration/standard/calibrators/temperature.py

    class CalibrationData(Calibrator.CalibrationData):
-        measured: Dict[int, Dict[str, List[float]]] = {}  # {vial_index: {"reference": [], "raw": []}}
+        procedure_state: CalibrationStateModel = {}


in this case we actually don't even need to overload anymore, so this whole class can (and should) be removed.

yep will fix

imaitland · 2025-02-12T14:24:03Z

evolver/app/tests/test_calibration.py

@@ -212,6 +275,22 @@ def test_dispatch_temperature_calibration_calculate_fit_action():
    assert output_transformer_response.json()["0"]["parameters"] == [0.6149999999999997, 0.024599999999999997]


+def get_stable_state_subset(state):


Having to do this reveals an area for improvement in the history data structure. Since history is part of the state persisted to the history, the entire history is actually already available in the history prop of the last added item to the history stack...

Soooo...
To fix, update the history stack data-structure update methods (push and pop) to only need the last state in the stack, and update the undo method, to essentially walk the history attr of the single state that's now in the history stack.

See issue: #256

amitschang

Thanks! I think it can go, but a couple comments for your perusal

amitschang · 2025-02-12T16:28:01Z

evolver/calibration/interface.py

+            if self.calibration_file:
+                self.calibration_data = CalibrationStateModel.load(file_path=self.calibration_file)
+            else:
+                self.calibration_data = CalibrationStateModel()


I think this is not quite right. Since this is init and we are passed the calibration_file to input, we shouldn't have the self.calibration_file unless set here, so the above condition (if calibration_file) should be sufficient? Meaning here is just to create a default calibration state

Yeah makes sense. Fixed.

amitschang · 2025-02-12T16:39:02Z

evolver/calibration/standard/actions/temperature.py

+        if not state.measured:
+            state.measured = {}
+        if self.vial_idx not in state.measured:
+            state.measured[self.vial_idx] = {"reference": [], "raw": []}
+        state.measured[self.vial_idx]["reference"].append(payload.temperature)


totally fine, but in case you like: collections.defaultdict and/or setdefault is useful here:

state.measured = state.measured or defaultdict(lambda: {"referece": [], "raw": []}) state.measured[self.vial_idx]["reference"].append(payload.temperature)

where all elements are same kind, no need to check and initialize yourself.

imaitland added 6 commits February 6, 2025 12:40

working procedure resume

7795176

format

e1fc9c0

update comment

3c809c5

update comments

ff49bad

refactor

2fe2463

refactor started to be part of common procedure state

3ba2b3c

imaitland requested a review from amitschang February 6, 2025 18:55

imaitland requested a review from a team as a code owner February 6, 2025 18:55

imaitland commented Feb 6, 2025

View reviewed changes

evolver/app/tests/test_calibration.py Outdated Show resolved Hide resolved

imaitland commented Feb 6, 2025

View reviewed changes

imaitland added 3 commits February 6, 2025 14:56

refactor procedure ds

54a4936

refactor to use pydantic all through the procedure state across actio…

8dc2cdc

…ns and calibration data

format

cdeac44

imaitland commented Feb 6, 2025

View reviewed changes

evolver/calibration/procedure.py Show resolved Hide resolved

imaitland commented Feb 6, 2025

View reviewed changes

typo

f307906

amitschang reviewed Feb 6, 2025

View reviewed changes

imaitland added 4 commits February 11, 2025 16:07

refactor CalibrationData, use CalibrationStateModel

6c5f66a

working refactor

8d0157b

cleanup

7fc92f9

format

e322e3d

imaitland requested a review from amitschang February 11, 2025 21:43

imaitland commented Feb 12, 2025

View reviewed changes

amitschang previously approved these changes Feb 12, 2025

View reviewed changes

imaitland added 2 commits February 12, 2025 12:04

use defaultdict in actions and respond to arik's comment

be641c4

format

33b70b1

imaitland dismissed amitschang’s stale review via 33b70b1 February 12, 2025 17:10

imaitland requested a review from amitschang February 12, 2025 17:11

amitschang approved these changes Feb 12, 2025

View reviewed changes

imaitland merged commit 89e9ee7 into main Feb 12, 2025
8 checks passed

imaitland deleted the iainland/resume-procedure-2 branch February 12, 2025 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resume procedure endpoint #253

resume procedure endpoint #253

imaitland commented Feb 6, 2025

imaitland Feb 6, 2025

codecov bot commented Feb 6, 2025 •

edited

Loading

imaitland Feb 6, 2025

imaitland Feb 6, 2025

imaitland Feb 6, 2025

amitschang Feb 6, 2025

imaitland Feb 6, 2025

amitschang left a comment

amitschang Feb 6, 2025

amitschang Feb 6, 2025

imaitland Feb 11, 2025

amitschang Feb 6, 2025

imaitland Feb 11, 2025

imaitland Feb 12, 2025

amitschang left a comment

amitschang Feb 12, 2025

imaitland Feb 12, 2025

amitschang Feb 12, 2025

		@@ -135,7 +140,7 @@ def dispatch_calibrator_action(request: Request, hardware_name: str = Path(...),

		payload = action.get("payload", {})

		return {**calibration_procedure.dispatch(action_to_dispatch, payload), "started": True}

		@@ -212,6 +275,22 @@ def test_dispatch_temperature_calibration_calculate_fit_action():
		assert output_transformer_response.json()["0"]["parameters"] == [0.6149999999999997, 0.024599999999999997]


		def get_stable_state_subset(state):

resume procedure endpoint #253

resume procedure endpoint #253

Conversation

imaitland commented Feb 6, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitschang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitschang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2025 •

edited

Loading