ssec-jhu / evolver-ng

Next gen eVolver controller for bioreactor project - wip
BSD 3-Clause "New" or "Revised" License
3 stars 0 forks source link

Global evolver instance at the app layer #67

Closed jamienoss closed 2 months ago

jamienoss commented 3 months ago

Currently, we have:

# evolver.app.main

app = FastAPI()
evolver = Evolver()

This won't work when running the app from uvicorn using multiple workers, they'll each get their own copy that won't be the same instance. It's possible that this may not matter, however, I expect that multiple serial connections will not work. Oh, and the main loop def won't work as desired.

For the app on the box, we probably don't want to run with multiple workers anyhow.

Running the app elsewhere though, should still be ok as this would have to go indirectly using the web app connection rather than the serial connection.

From my understanding either way, the more correct FastAPI way to do this, is to stash it in app, though I'm not yet sure of what happens there.

# evolver.app.main

app = FastAPI()
app.evolver = Evolver()
amitschang commented 3 months ago

I agree that there is no case for running multiple workers, there should only be a single evolver (manager) process due to both the serial and control functionality to not conflict.

I don't quite understand the latter part. What would be the reason the evolver instance should be stored in a property of the app? I doubt that property would ever end up being something used in fastapi internals (but could, I suppose), also wondering what problems it would solve?

jamienoss commented 2 months ago

Global scope just isn't sufficient here, for example, if evolver.app.main.evolver was ever nullified (for whatever reason), from evolver.app.main import evolver wouldn't work as this refs the state of evolver at import time, not when it was updated (nullified) as it's not a pointer.

A more obvious scenario is that addressed in #76 where it's more correct to instantiate the instance of evolver in the startup event, in which case it's not even present at import time.

It also just makes it easier to assign to rather than having to do something like globals()["evolver"] = ... from path funcs in evolver.app.main.

jamienoss commented 2 months ago

See https://github.com/ssec-jhu/evolver-ng/pull/73#discussion_r1619464865

I see now what is going on here. I think the correct location is under app.state (e.g. app.state.evolver), which is a starlette State datastructure (https://github.com/encode/starlette/blob/master/starlette/datastructures.py#L680) and is meant for arbitrary app/request state. This way it avoids potential to conflict with app internals