Add abort api #194

amitschang · 2024-10-16T20:16:25Z

Sets up API for abort, which would put all devices into a stopped state (whatever that means for a particular device still in the powered-on state). The device-level action both stops the control components of loop and also the all devices, while the web-app (application-level) aborts the device and writes out the control disablement to config, such that a restart would not inadvertently continue normal operation without user intervention (given the current mainline behavior of the system)

Resolves #121

codecov · 2024-10-16T20:19:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

jamienoss · 2024-10-17T14:06:03Z

evolver/app/main.py

+    app.state.evolver.abort()
+    # Disable commit also in persistent config in case application needs to restart
+    app.state.evolver.config_model.enable_commit = False
+    app.state.evolver.config_model.save(app_settings.CONFIG_FILE)


I think we'll need to repon that convo about stashing state vs configuration, as in #185.

There seems to be some flip-flopping on what app_settings.CONFIG_FILE is for. I was initially led to believe that it was for state, however, during #185 it was argued that it very much isn't for state, but for updating the configuration, but only as intended bu the user. The latter point emphasized by the fact that only the update endpoint should clobber app_settings.CONFIG_FILE as this is the only place where configuration is intentionally changed by the user.

We should not conflate state and configuration as I did before. Updating the configuration file here from the evolver's dynamic state (which can very much differ from its configuration) is exactly the same as having done so in #185.

it is config, and the abort should update like here - but I see the point about potential internal changes between update and abort. I will change it to update directly from the file

jamienoss · 2024-10-22T16:42:59Z

evolver/hardware/demo.py

    def commit(self):
        self.comitted = copy(self.proposal)
+
+    def abort(self):
+        self.aborted = True


What's this attr for?
Doesn't this need to go in the interface class to be adequately utilized?

this is the demo, the attr is specifically for testing

I think this would be misleading to someone interpreting the "demo" as something to mimic in their implementations, which they shouldn't.

This would be better off in the test code itself, e.g., via a monkeypatch or something similar.

jamienoss · 2024-10-22T16:55:02Z

evolver/hardware/standard/led.py

+        cmd = [b"0"] * self.slots
+        with self.serial as comm:
+            comm.communicate(SerialData(addr=self.addr, data=cmd))


This looks like more than just abort code, as mentioned before, couldn't this go in another abstractmethod named, e.g., "reset", "idle", "off", or "init", etc, that then abort just calls? abort wouldn't have to be abstract.

Looks to me like these should be called upon initialization as a way for controlling a baseline on the hardware's physical state.

I believe the above is what it takes to abort the operation of the device. Do you mean like naming it off since that would look less alarming if called upon initialization (if we do so in the future)?

Not entirely, but kinda yeah, I meant that we both mentioned in #197 it might be desirable to exec these commands upon initial startup, in which case it's not really an "abort" but more of a ground/rest/init/off state.

Technically it's not aborting anything as we have to actually send a commit to command the hardware to some state. Evolver.abort is an "abort" but HardwareDriver.abort isn't.

Either way, I didn't mean anything complicated just something like the following:

class HardwareDriver(...): @abstractmethod def init(self, *args, **kwargs): # could be called "reset" """ Implement to command hardware to its ground/off state. This could be called upon startup/abort.""" ... def abort(self): self.init()

Actually the device abort does send the command directly, no commit required. I'm not sure yet what we will do at startup, but I can definitely rename the device method to off instead. "abort" speaks to the immediacy of it though...

I've renamed driver method to off and added docstring for method expectations. I will defer doing anything with startup in this PR.

Actually the device abort does send the command directly, no commit required.

I only meant that it still has to send a command period and not just block subsequent commands. That these hardware methods don't alter the config, they themselves don't "block" future commanding at all etc, and that they are in fact completely oblivious to this concept (at least they should be). They only alter the hardware to some specific state, a state in this instance that is encoded in the semantics of this function name - which isn't "abort".

"abort" speaks to the immediacy of it though...

Agreed, and evolver.device.Evolver.abort is cool to be named such as it does actually "abort" subsequent control etc, though there's still scope for argument around the fact that it still pings the hardware and reads data so doesn't completely "abort", see #199 #196.

"abort" speaks to the immediacy of it though...

At the same time, however, this isn't entirely true due to the significant (1-2s) latency per call and the fact that these are turned off in sequence rather than proposals being set and committed all at once.

jamienoss · 2024-10-22T16:58:38Z

evolver/app/main.py

+    config = Evolver.Config.load(app_settings.CONFIG_FILE)
+    config.enable_control = False
+    config.enable_commit = False
+    config.save(app_settings.CONFIG_FILE)


Does this mean that whilst the user can hit this endpoint to stop the control, they have to use the update endpoint to start/restart it and have to specifically do so by (hopefully) only manually changing these two flags to True every time?

I don't think this is a sufficient user interface and have opened #198 to add a "start" endpoint.

jamienoss · 2024-10-22T20:54:07Z

evolver/tests/test_device.py

+        def patched_abort(self):
+            self.aborted = True


I think it would be better to still call the original abort here, to future-proof the test if something actually gets added to NoOpEffectorDriver.abort, rather than circumvent the test, no?

jamienoss · 2024-10-25T13:45:38Z

evolver/app/main.py

+    app.state.evolver.abort()
+    # Disable commit also in persistent config in case application needs to restart
+    config = Evolver.Config.load(app_settings.CONFIG_FILE)
+    config.enable_control = False


FYI I've opened #199 for us to also consider aborting the read.

jamienoss

👍

jamienoss reviewed Oct 17, 2024

View reviewed changes

amitschang changed the title ~~abort wip~~ Add abort api Oct 22, 2024

amitschang marked this pull request as ready for review October 22, 2024 16:10

amitschang requested a review from a team as a code owner October 22, 2024 16:10

amitschang requested a review from imaitland October 22, 2024 16:10

jamienoss reviewed Oct 22, 2024

View reviewed changes

This was referenced Oct 22, 2024

Record abort events #196

Closed

Add a start endpoint to start/restart control after having aborted #198

Closed

jamienoss reviewed Oct 22, 2024

View reviewed changes

amitschang force-pushed the gh-121-abort branch from 51eb7ca to 1ee5082 Compare October 23, 2024 14:27

amitschang added 7 commits October 23, 2024 14:52

abort wip

043029f

check ipp abort

23b842a

update only persisted config in abort

ad5e86f

also disable control

aeaa32b

pr update

812e5df

pr updates

bf0ee8b

rename abort command -> off

fdebb69

amitschang force-pushed the gh-121-abort branch from 3ff558a to fdebb69 Compare October 23, 2024 18:54

amitschang requested a review from jamienoss October 23, 2024 18:55

jamienoss assigned amitschang Oct 25, 2024

jamienoss added new feature hardware app labels Oct 25, 2024

jamienoss mentioned this pull request Oct 25, 2024

Consider adding an enable_read switch #199

Open

jamienoss reviewed Oct 25, 2024

View reviewed changes

jamienoss approved these changes Oct 25, 2024

View reviewed changes

amitschang merged commit 864bd43 into main Oct 25, 2024
8 checks passed

amitschang deleted the gh-121-abort branch October 25, 2024 14:48

jamienoss mentioned this pull request Oct 25, 2024

Add start/stop/pause functionality to backend and app #121

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add abort api #194

Add abort api #194

amitschang commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

jamienoss Oct 17, 2024 •

edited

Loading

amitschang Oct 18, 2024

jamienoss Oct 22, 2024 •

edited

Loading

amitschang Oct 22, 2024

jamienoss Oct 22, 2024

jamienoss Oct 22, 2024 •

edited

Loading

amitschang Oct 22, 2024

jamienoss Oct 22, 2024 •

edited

Loading

amitschang Oct 23, 2024

amitschang Oct 23, 2024

jamienoss Oct 25, 2024

jamienoss Oct 22, 2024 •

edited

Loading

amitschang Oct 22, 2024

jamienoss Oct 22, 2024

jamienoss Oct 22, 2024

jamienoss Oct 25, 2024

jamienoss left a comment

Add abort api #194

Add abort api #194

Conversation

amitschang commented Oct 16, 2024 • edited Loading

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

jamienoss Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamienoss Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamienoss Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamienoss Oct 22, 2024 • edited Loading

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

jamienoss Oct 22, 2024 • edited Loading

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

jamienoss left a comment

Choose a reason for hiding this comment

amitschang commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

jamienoss Oct 17, 2024 •

edited

Loading

jamienoss Oct 22, 2024 •

edited

Loading

jamienoss Oct 22, 2024 •

edited

Loading

jamienoss Oct 22, 2024 •

edited

Loading

jamienoss Oct 22, 2024 •

edited

Loading