-
Notifications
You must be signed in to change notification settings - Fork 8
Bot State Persistence #55
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
Conversation
7f13b36
to
2452f86
Compare
You should probably change the target of the PR to the branch for #54 to make this easier to review. |
@solomon-b Thanks. Looks like some of the persistence stuff went into #54 though. |
6ef179e
to
f58db2b
Compare
2452f86
to
857871e
Compare
f58db2b
to
f189b7a
Compare
857871e
to
0f4aec2
Compare
@masaeedu yeah sorry about that. Its fixed now. I just made some interface changes in the other branch and the rebasing got mixed up. I'll paste a script for how to use it in like an hour. You can also look at the parser to figure it out. |
f189b7a
to
9b11eee
Compare
4233541
to
c911b2f
Compare
9b11eee
to
9bc7e02
Compare
2ed52b1
to
bb69fda
Compare
bb69fda
to
b5622ee
Compare
app/Main.hs
Outdated
simplifyMatrixBot $ | ||
bot | ||
process | ||
state <- readState "state" |
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.
what type does this get?
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.
Read s, Show s => s
src/CofreeBot/Bot.hs
Outdated
|
||
fixBotPersistent :: forall m s i o. (MonadIO m, Serializable s) => Bot m s i o -> s -> IO (Behavior m i o) | ||
fixBotPersistent (Bot bot) initialState = do | ||
saveState "state" initialState |
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.
Probably callers of this function should give a key, otherwise they will all clobber one another, right?
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.
Are you imagining having multiple bots running on the same linux user?
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.
I'm imaging each sub bot wants to manage its own state separately from the global composition
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.
Do you mean that you would want bots to be able to state explicitly how their individual state is persisted?
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.
I'm imaging each sub bot wants to manage its own state separately from the global composition
I think @isovector is talking about a scenario in which a user fixBotPersistent
s several independent bots, then laterally composes the resulting Behavior
s (or whatever it is we're calling them now), instead of doing it the other way around as we're doing now.
This is a totally reasonable use case (although not one we need right now), and it'd be pretty easy to support by just adding a parameter for the file path here. We can just pass in xdgCache </> "state"
or whatever it is as that file path closer to Main
.
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.
Oh ok that is a totally reasonable user expectation. I'll thread in the path from main rather then hard code it here.
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.
done!
@@ -30,7 +31,7 @@ main = do | |||
TokenCmd TokenCredentials {..} -> do | |||
session <- createSession (getMatrixServer matrixServer) matrixToken | |||
matrixMain session xdgCache | |||
CLI -> cliMain | |||
CLI -> cliMain xdgCache |
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.
We probably don't have enough config yet to warrant this change, but we might want to eventually have an app-specific monad in which "global" config like this is accessible via MonadReader
, so that we don't have to pass it around.
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.
Yeah I agree once the executable gets a bit more sophisticated.
liftIO $ saveState cachePath newState | ||
pure (output, go) | ||
|
||
readState :: Read s => String -> IO (Maybe s) |
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.
It would be nice if we could avoid opening and closing this file over and over, but I don't have any good ideas at this moment about how to do it.
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.
I guess we could read the file once and then persist it in an IORef/MVar/TVAR for the lifecycle of the bot? If we go that route we should put it in a followup PR
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.
LGTM, nice
Adds a context function which moves a bot's in-memory state to the file system.