DiamondLightSource / tickit

Event-based hardware simulation framework
Apache License 2.0
7 stars 0 forks source link

Replace run_forever with start/stop #152

Open callumforrester opened 1 year ago

callumforrester commented 1 year ago

Speculative Issue for Discussion

The run_forever() API is prevalent throughout tickit. Components, adapters, schedulers etc. all have it. The idea is to collect this bucket of things into a giant asyncio task that is essentially :

asyncio.wait([thing.run_forever() for thing in all things])

This is convenient for running arbitrary simulations but painful to test. Tests need to know a component/adapter/scheduler has completed startup tasks and is ready to accept API calls. See https://github.com/dls-controls/tickit/issues/76. run_forever() often performs startup tasks opaquely, so the tests can't know when it's done. There have been various workarounds throughout the codebase such as exposing asyncio.Events or just having a sleep to allow servers time to start. None of these are ideal.

We also cannot easily shut things down between tests, there have been instances of tests breaking other tests. In some cases we have restored forcing task cancels.

I propose we go through and change all of these APIs from

async def run_forever(self) -> None:
    ...

to

async def start(self) -> None:
    ...

async def stop(self) -> None:
    ...

The contract then is that start() performs all startup tasks and can be awaited on and stop() can be awaited on until all tasks have shut down. It is then easy to offer a fixture for a particular component et al.

@pytest_asyncio.fixture
async def thing() -> Thing:
    thing = Thing(...)
    await thing.start()
    yield thing
    await thing.stop()

Tagging for discussion: @abbiemery @coretl @tpoliaw @DiamondJoseph

rosesyrett commented 1 year ago

@abbiemery has anyone been asigned to do this?