silverback icon indicating copy to clipboard operation
silverback copied to clipboard

feat!: add `app.state`

Open fubuloubu opened this issue 1 year ago • 4 comments

What I did

This PR adds worker shared state (accessible via app.state) and adds 2 new system tasks to support loading and storing a snapshot of that managed state. Currently this is just last_block_processed and last_block_seen.

This paves the way for the Parameter feature, but does not include that just yet.

BREAKING CHANGE: state snapshot features migrates from runner to worker and must be triggered by runner

How I did it

How to verify it

Checklist

  • [x] Passes all linting checks (pre-commit and CI jobs) ~~- [ ] New test cases have been added and are passing~~
  • [ ] Documentation has been updated
  • [x] PR title follows Conventional Commit standard (will be automatically included in the changelog)

fubuloubu avatar May 05 '24 02:05 fubuloubu

Given the multiprocessing bug likely rugs my initial approach, I will refactor to a delegated storage system where each read of something from app.state.* will trigger a call through a BaseDatastore subclass to fetch the most up-to-date value, and every write will push an update to that central datastore resource (likely involving a lockfile for the json backup option, but can get away with an async lock for the default in-memory implementation)

For the platform, this will involve an http call to the CCS to fetch latest parameters and update them (no lock required, just FIFO based on API call)

Unfortunately, this requires more strict typing of what values can be stored in state (similar to parameters)

fubuloubu avatar May 07 '24 04:05 fubuloubu

So given that distributed TaskIQ sequencing currently operates as multi-threading (and not multi-processing), this approach of worker-managed application state is okay as long as there isn't multiple workers trying to write the same state at the same time. We may need to integrate a lock to the AppState datatype in order to ensure this, but in terms of the mechanism it will actually function as long as there is only one container with one process operating the multi-threaded workers.

fubuloubu avatar May 30 '24 23:05 fubuloubu

TODO:

  • [x] write documentation about app.state and how it gets used

fubuloubu avatar May 31 '24 14:05 fubuloubu

I still don't really understand why the datastore is on the worker side.

Originally I don't think I actually understood what you were saying, but yes it is very clear to me now that needs to be inverted. The application state being worker side is what I wanted here, but the state snapshot loading and backup should be entirely on the runner client to perform. My mistake.

I hope you agree the solution should be to update this system function and add startup_state as a parameter it receives from the runner to update the local state on startup, then update this system function to just return the state snapshot back to the runner to write to disk; lastly, can move the AppDatastore back to the SDK's BaseRunner (and maybe make that a configurable ABC if you need to use it downstream, up to you).

fubuloubu avatar Aug 17 '24 01:08 fubuloubu

I still don't really understand why the datastore is on the worker side. I'm going to have to basically re-invent that whole thing for the platform side, more or less reverting or duplicating some of this design. But I think a bunch of this was already touched in previous discussions so probably pointless to mention.

Will try and figure out how to make this work on the cluster side when I get a chance.

@mikeshultz resolved in https://github.com/ApeWorX/silverback/pull/80/commits/07dd9f9643c9f47efa7a6630c584d1839541e1c9

fubuloubu avatar Aug 26 '24 17:08 fubuloubu

Note for reviewers: SILVERBACK_APP_NAME changes to SILVERBACK_BOT_NAME from this PR, which also made some other larger changes for the whole app -> bot rewording

Also need to publicize this, since SilverbackApp also changes to SilverbackBot

fubuloubu avatar Oct 09 '24 20:10 fubuloubu