-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
boostrap framework #7
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
from abc import ABC, abstractmethod | ||
from pydantic import BaseModel | ||
|
||
|
||
class Adapter(ABC): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What exactly is an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could call it |
||
class Config(BaseModel): | ||
pass | ||
|
||
def __init__(self, evolver, config: Config = None): | ||
self.config = config or self.Config() | ||
|
||
@abstractmethod | ||
def react(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this stage of the review I'm not sure if I get this, but I wasn't expecting |
||
pass | ||
|
||
|
||
class NoOpAdapter(Adapter): | ||
ncalls = 0 | ||
|
||
def react(self): | ||
self.ncalls += 1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,61 @@ | ||
import asyncio | ||
from fastapi import FastAPI | ||
from fastapi.responses import RedirectResponse | ||
|
||
from evolver.device import Evolver, EvolverConfig | ||
from .. import __project__, __version__ | ||
|
||
|
||
app = FastAPI() | ||
evolver = Evolver() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really the best place for this comment, but before I forget - I think we should remove all usage of "evolver" from the code itself. All we have here is simple control loop type stuff and I see no reason to tie it to "evolver". Evolver can come in at the config or UI level. That way the users can see it etc, just doesn't need to be a code literal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure. What else would we call it? This is for the evolver after all, no shame in that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant more of the attr, not the class, e.g., |
||
|
||
|
||
@app.get("/") | ||
async def root(): | ||
return RedirectResponse(url='/docs') | ||
async def describe_evolver(): | ||
return { | ||
'config': evolver.config, | ||
'state': evolver.state, | ||
'last_read': evolver.last_read, | ||
} | ||
|
||
|
||
@app.get('/state') | ||
async def get_state(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'll need a set_state as well, so that stuff can be manually controlled via the web api. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. totally! just not here yet, and I think we need to ensure that the manual control and loop control don't happen together, as that could be confusing to user. |
||
return { | ||
'state': evolver.state, | ||
'last_read': evolver.last_read, | ||
} | ||
|
||
|
||
@app.post("/") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if it's a good idea to post to root. Might be better/safer to have an explicit path to update the config. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're going to need to think about locks of some sort since the control and update can happen from the web API and multiple users may be concurrently accessing - maybe they just can't by def of the lock. This may just need to be in the web API (hopefully), or lower (hopefully not much lower). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're probably gonna have to bake into web api/ui some notion of the above even for a single user since the API is async and inpatient (or not) sequential posts have no guarantee of order. E.g., not being able to execute some commands (path funcs) if the system is not in a ready state to do so. |
||
async def update_evolver(config: EvolverConfig): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might need to think about when this allowed, i.e., always, only once at the beginning, or...? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would think any time. There could be a problem mid loop I suppose in such a case (e.g. during the |
||
evolver.update_config(config) | ||
|
||
|
||
@app.get('/schema') | ||
async def get_schema(): | ||
return evolver.schema | ||
|
||
|
||
@app.get('/history/{name}') | ||
async def get_history(name: str): | ||
return evolver.history.get(name) | ||
|
||
|
||
@app.get("/healthz") | ||
async def healthz(): | ||
return {"message": f"Running '{__project__}' ver: '{__version__}'"} | ||
|
||
|
||
async def evolver_async_loop(): | ||
while True: | ||
evolver.loop_once() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might add versatility to add
Perhaps even with chaining return values into the next call. |
||
await asyncio.sleep(evolver.config.interval) | ||
|
||
|
||
@app.on_event('startup') | ||
async def start_evolver_loop(): | ||
asyncio.create_task(evolver_async_loop()) | ||
|
||
|
||
if __name__ == '__main__': | ||
import uvicorn | ||
uvicorn.run(app, host="127.0.0.1", port=8000, log_level="info") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import pytest | ||
from fastapi.testclient import TestClient | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI PEP8 likes a line between each of the import kinds and that existing line separated 3rd party from local.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we make ruff catch this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like we should be able to, see: |
||
from ..main import app # Leave as relative for use in template: ssec-jhu/base-template. | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,3 +11,22 @@ def test_healthz(self, app_client): | |||||
assert response.status_code == 200 | ||||||
if __version__: | ||||||
assert __version__ in response.json()["message"], response.json() | ||||||
|
||||||
def test_evolver_app_default_config_dump_endpoint(self, app_client): | ||||||
response = app_client.get('/') | ||||||
assert response.status_code == 200 | ||||||
assert sorted(response.json().keys()) == ['config', 'last_read', 'state'] | ||||||
|
||||||
def test_evolver_update_config_endpoint(self, app_client): | ||||||
data = {'hardware': {'test': {'driver': 'evolver.hardware.NoOpSensorDriver'}}} | ||||||
response = app_client.post('/', json=data) | ||||||
assert response.status_code == 200 | ||||||
newconfig = app_client.get('/').json()['config'] | ||||||
assert newconfig['hardware']['test']['driver'] == 'evolver.hardware.NoOpSensorDriver' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
def test_evolver_app_react_loop_setup(self, app_client): | ||||||
# The context manager ensures that startup event loop is called | ||||||
# TODO: check results generated in react (may require hardware at startup, or forced execution of loop) | ||||||
with app_client as client: | ||||||
response = client.get('/') | ||||||
assert response.status_code == 200 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
import time | ||
from pydantic import BaseModel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the genericity of
|
||
from evolver.util import load_class_fqcn, driver_from_descriptor | ||
from evolver.hardware import SensorDriver, EffectorDriver | ||
from evolver.serial import EvolverSerialUART | ||
from evolver.history import HistoryServer | ||
|
||
|
||
DEFAULT_SERIAL = EvolverSerialUART | ||
DEFAULT_HISTORY = HistoryServer | ||
|
||
|
||
class AdapterDescriptor(BaseModel): | ||
driver: str | ||
config: dict = {} | ||
|
||
|
||
class HardwareDriverDescriptor(AdapterDescriptor): | ||
calibrator: AdapterDescriptor = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, this might be the right layer for calibrators. |
||
|
||
|
||
class EvolverConfig(BaseModel): | ||
vials: list = list(range(16)) | ||
hardware: dict[str, HardwareDriverDescriptor] = {} | ||
adapters: list[AdapterDescriptor] = [] | ||
Comment on lines
+29
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussion point for tomorrow, in case I forget. |
||
serial: AdapterDescriptor = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. serial -> comms (another discussion point) |
||
history: AdapterDescriptor = None | ||
enable_react: bool = True | ||
enable_commit: bool = True | ||
interval: int = 20 | ||
|
||
|
||
class Evolver: | ||
hardware = {} | ||
adapters = [] | ||
last_read = {} | ||
Comment on lines
+39
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these meant to be class attrs - is this ok? Might be better to use a singleton as these look a little dangerous as is. |
||
|
||
def __init__(self, config: EvolverConfig = EvolverConfig()): | ||
self.update_config(config) | ||
|
||
def update_config(self, config: EvolverConfig): | ||
self.config = config | ||
for name, driver in config.hardware.items(): | ||
self.setup_driver(name, driver) | ||
for name in list(self.hardware.keys()): | ||
if name not in config.hardware.keys(): | ||
del(self.hardware[name]) | ||
del(self.last_read[name]) | ||
Comment on lines
+50
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just nuke "update" might also not be entirely accurate for this kind of set-del, perhaps "configure" and/or "reconfigure"? Not saying that there isn't a place for an "update", just that an "update" might be expected to operate similarly to a |
||
self.adapters = [] | ||
for adapter in config.adapters: | ||
self.setup_adapter(adapter) | ||
Comment on lines
+55
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, I'm still confused about what's hardware vs adapter? Is the hardware the box and adapters those devices attached to the box? |
||
if config.serial is not None: | ||
self.serial = driver_from_descriptor(self, config.serial) | ||
else: | ||
self.serial = DEFAULT_SERIAL() | ||
if config.history is not None: | ||
self.history = driver_from_descriptor(self, config.history) | ||
else: | ||
self.history = DEFAULT_HISTORY() | ||
|
||
def setup_driver(self, name, driver_config: HardwareDriverDescriptor): | ||
driver_class = load_class_fqcn(driver_config.driver) | ||
config = driver_class.Config.model_validate(driver_config.config) | ||
calibrator = None | ||
if driver_config.calibrator is not None: | ||
calibrator = driver_from_descriptor(self, driver_config.calibrator) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussion point - looks like single inter-thingy calibration (if I've understood "device" correctly). |
||
self.hardware[name] = driver_class(self, config, calibrator) | ||
self.last_read[name] = -1 | ||
|
||
def setup_adapter(self, adapter): | ||
self.adapters.append(driver_from_descriptor(self, adapter)) | ||
|
||
def get_hardware(self, name): | ||
return self.hardware[name] | ||
Comment on lines
+78
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed? |
||
|
||
@property | ||
def sensors(self): | ||
return {k: v for k,v in self.hardware.items() if isinstance(v, SensorDriver)} | ||
|
||
@property | ||
def effectors(self): | ||
return {k: v for k,v in self.hardware.items() if isinstance(v, EffectorDriver)} | ||
Comment on lines
+81
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if it's beneficial to separate sensors and effectors this explicitly. Perhaps at some higher-level layer (which, this might actually be). Perhaps it would be better to just have devices derived from a base interface that has get/set or read/write methods. |
||
|
||
@property | ||
def calibration_status(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussion point.
|
||
return {name: device.calibrator.status for name,device in self.hardware.items()} | ||
|
||
@property | ||
def state(self): | ||
return {name: device.get() for name,device in self.sensors.items()} | ||
|
||
@property | ||
def schema(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if a schema is requested whilst the config is being updated? Looks like we'll need a mutex at some class level, i.e., the "testbed" or main controller/scheduler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Context managers are useful for this. |
||
hardware_schemas = [] | ||
for n, hw in self.hardware.items(): | ||
s = {'name': n, 'kind': str(type(hw)),'config': hw.Config.model_json_schema()} | ||
if isinstance(hw, SensorDriver): | ||
s['output'] = hw.Output.model_json_schema() | ||
if isinstance(hw, EffectorDriver): | ||
s['input'] = hw.Input.model_json_schema() | ||
hardware_schemas.append(s) | ||
return { | ||
'hardware': hardware_schemas, | ||
'adapters': [{'kind': str(type(a)), 'config': a.Config.model_json_schema()} for a in self.adapters], | ||
} | ||
|
||
def read_state(self): | ||
for name, device in self.sensors.items(): | ||
device.read() | ||
self.last_read[name] = time.time() | ||
self.history.put(name, device.get()) | ||
|
||
|
||
def evaluate_adapters(self): | ||
for adapter in self.adapters: | ||
adapter.react() | ||
|
||
def commit_proposals(self): | ||
for device in self.effectors.values(): | ||
device.commit() | ||
|
||
def loop_once(self): | ||
self.read_state() | ||
# for any hardware awaiting calibration, call calibration update method here | ||
if self.config.enable_react: | ||
self.evaluate_adapters() | ||
if self.config.enable_commit: | ||
self.commit_proposals() | ||
Comment on lines
+128
to
+133
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since both the flags/switches and methods belong to the instance, this logic should happen within the methods themselves. Well, maybe at least |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
from abc import ABC, abstractmethod | ||
from copy import copy | ||
from pydantic import BaseModel | ||
|
||
|
||
class VialConfigBaseModel(BaseModel): | ||
vials: list[int] | None = None | ||
|
||
|
||
class VialBaseModel(BaseModel): | ||
vial: int | ||
|
||
|
||
class BaseCalibrator(ABC): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be better if we removed the notion of "calibration". Instead, I think it would be more generic if we added At a higher-level, i.e., "evolver" layer this is good, the above stmnt was meant for lower-level interface code, which I don't think this is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how this would work. Lets go over the existing calibration routines and see |
||
class Config(BaseModel): | ||
calibfile: str = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the calibration is to be used for a given hardware/device config, does it make sense that it has its own config unless:
|
||
|
||
def __init__(self, evovler = None, config: Config = Config()): | ||
self.config = config | ||
|
||
@property | ||
@abstractmethod | ||
def status(self): | ||
pass | ||
|
||
|
||
class NoOpCalibrator(BaseCalibrator): | ||
@property | ||
def status(self): | ||
return True | ||
|
||
|
||
class HardwareDriver(ABC): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is intended to have more of an interface since it's abstract, e.g., communicate/send methods that use the config-specified communications layer, i.e., serial? |
||
class Config(BaseModel): | ||
pass | ||
calibrator = NoOpCalibrator() | ||
|
||
def __init__(self, evolver, config = None, calibrator = None): | ||
self.evolver = evolver | ||
self.reconfigure(config or self.Config()) | ||
if calibrator: | ||
self.calibrator = calibrator | ||
|
||
def reconfigure(self, config): | ||
self.config = config | ||
|
||
|
||
class VialHardwareDriver(HardwareDriver): | ||
def reconfigure(self, config): | ||
super().reconfigure(config) | ||
if config.vials is None: | ||
config.vials = self.evolver.config.vials | ||
else: | ||
if not set(config.vials).issubset(self.evolver.all_vials): | ||
raise ValueError('invalid vials found in config') | ||
|
||
|
||
class SensorDriver(VialHardwareDriver): | ||
class Config(VialConfigBaseModel): | ||
pass | ||
class Output(VialBaseModel): | ||
raw: int | ||
value: float | ||
outputs: dict[int, Output] = {} | ||
|
||
def get(self) -> list[Output]: | ||
return self.outputs | ||
|
||
@abstractmethod | ||
def read(self): | ||
pass | ||
|
||
|
||
class EffectorDriver(VialHardwareDriver): | ||
class Config(VialConfigBaseModel): | ||
pass | ||
class Input(VialBaseModel): | ||
value: float | ||
proposal: dict[int, Input] = {} | ||
committed: dict[int, Input] = {} | ||
|
||
def set(self, input: Input): | ||
self.proposal[input.vial] = input | ||
|
||
@abstractmethod | ||
def commit(self): | ||
pass | ||
|
||
|
||
class NoOpSensorDriver(SensorDriver): | ||
class Config(VialConfigBaseModel): | ||
echo_raw: int = 1 | ||
echo_val: int = 2 | ||
|
||
def read(self): | ||
self.outputs = { | ||
i: self.Output(vial=i, raw=self.config.echo_raw, value=self.config.echo_val) | ||
for i in self.config.vials | ||
} | ||
|
||
def get(self): | ||
return self.outputs | ||
|
||
|
||
class NoOpEffectorDriver(EffectorDriver): | ||
def commit(self): | ||
self.comitted = copy(self.proposal) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import time | ||
from abc import ABC, abstractmethod | ||
from collections import defaultdict | ||
from pydantic import BaseModel | ||
|
||
|
||
class History(ABC): | ||
class Config(BaseModel): | ||
pass | ||
|
||
@abstractmethod | ||
def put(self, name, data): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does a single history instance manage all "names" or is |
||
pass | ||
|
||
@abstractmethod | ||
def get(self, query): | ||
pass | ||
|
||
|
||
class HistoryServer(History): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I see, but now there's no interface for a given history, just them all aka the server. I think utilizing python logging or something else, would be good here. |
||
def __init__(self, config = None): | ||
self.history = defaultdict(list) | ||
|
||
def put(self, name, data): | ||
self.history[name].append((time.time(), data)) | ||
|
||
def get(self, name): | ||
return self.history[name] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[revision] Is it cool if we move this out of the
__init__.py
, please? I think it's hard to find code of this significance when it's hidden in a__init__.py
. It also doesn't help by creating indirect imports.I think it would be cleaner and more findable if base interface classes went in their own module file, i.e.,
adapter.interface.py
or something similar.Same goes for
hardware.__init__.py
.