Closed jmatsushita closed 2 years ago
This slow behavior exists both on aarch64 and x86. Let me know if you can reproduce :)
I'll have a look :) no idea how I reproduce on aarch64
though.
https://github.com/turion/rhine/runs/6568785125?check_suite_focus=true fails with error: overlay does not take an argument named 'final'
. The nix version used is 2.8.1.
Merge #164 first.
I've given this a spin. No idea what it's supposed to do exactly.
nix develop
. awesome!ctrl+d
or esc
would be great.The issue you had was that your display
component was on the MilliSecond 1000
clock. That means that display is only called every second. Which means it only collects one item do display every second. I changed it slightly so that the prompt beat is on the 1000 ms clock, and the key display is on the terminal clock. Now everything works like a charm it seems.
Ironically, this is exactly the kind of bug rhine
is supposed to resolve :laughing: (but at least it was clear to see and reason about)
Ironically, this is exactly the kind of bug rhine is supposed to resolve π (but at least it was clear to see and reason about)
Ha ha ha, well I suspected the bug was in my mis-use of rhine :) Looking forward to dig into this. Super grateful for being unblocked π
Hi @turion ,
So I got started on the todo list:
- [x] Add
rhine-terminal
tocabal.project
andstack.yaml
.
I've added rhine-terminal
to the flake.nix
. If you want we can change the cabal.project
file to list the packages explicitly. For stack.yaml
it might make more sense to have https://github.com/turion/rhine/pull/166 merged beforehand?
- [x] Factor out the terminal clock to a library.
- [x] Keep the executable as an example application.
I've made good progress I think and followed the rhine-gloss
approach to try and avoid threading term
throughout using a ReaderT, but I got myself into another corner and the app is not running anymore :/ When starting it, nothing happens... Would appreciate your eyes on this.
- [ ] If you're motivated add an integration test that produces some characters on stdout.
Happy to try when the example works!
With regards to your fix and my misunderstanding, I see that you refactored the Rhines such that they are composed with "Time-parallel signal composition", using ||@ terminalConcurrently @||
. I've tried to represent this here:
However this leaves me wondering whether it's possible to fix the problem while keeping the original intent composing sensors and actuators like so:
Should there be a way to compose Rhines :
compRhines :: Rhine m cl a b -> Rhine m cl b c -> Rhine m cl a c
I understand from the paper that Rhines and Signal Networks don't form categories in general, but it seems that when clocks have the same values (or maybe the same tree structure?) it should be possible to compose Rhine end to end like that?
Another way to ask the same question I suppose is: is there currently a way to consume a Rhine m (ParallelClock m clL clR) a (Either b c)
?
I've committed the application code with the refactor and added the compRhines
shenannigans at the bottom, hoping there's a solution down that path too.
- [x] Add
rhine-terminal
tocabal.project
andstack.yaml
.I've added
rhine-terminal
to theflake.nix
.
:+1:
If you want we can change the
cabal.project
file to list the packages explicitly.
No, the fewer places where we need to list things, the better.
For
stack.yaml
it might make more sense to have #166 merged beforehand?
Yes, but that's not blocking.
- [x] Factor out the terminal clock to a library.
- [x] Keep the executable as an example application.
I've made good progress I think and followed the
rhine-gloss
approach to try and avoid threadingterm
throughout using a ReaderT, but I got myself into another corner and the app is not running anymore :/ When starting it, nothing happens... Would appreciate your eyes on this.
I believe it's your MonadInput
etc. instances which are basically infinite loops as far as I understand.
- [ ] If you're motivated add an integration test that produces some characters on stdout.
Happy to try when the example works!
:+1:
Some questions about ways to compose rhines and signal networks
With regards to your fix and my misunderstanding, I see that you refactored the Rhines such that they are composed with "Time-parallel signal composition", using
||@ terminalConcurrently @||
. I've tried to represent this here:![]()
However this leaves me wondering whether it's possible to fix the problem while keeping the original intent composing sensors and actuators like so:
![]()
Awesome graphics :) yes, this is also how I'd look at them I think.
Should there be a way to compose Rhines :
compRhines :: Rhine m cl a b -> Rhine m cl b c -> Rhine m cl a c
No, this doesn't work, for two reasons.
Rhine
does. So you'd force the two clocks in these Rhine
s to be identical, although they can be different values of the same type. This would at most work with Monoid cl
.cl
might be composed of several clocks, which means that b
leaves the first Rhine
at a different rate than it enters the second one. This cannot work without a buffer. At most it would work if cl ~ In cl ~ Out cl
, i.e. if cl
is atomic.If we make both assumptions, we can write something like your compRhines
.
Another way to ask the same question I suppose is: is there currently a way to consume a
Rhine m (ParallelClock m clL clR) a (Either b c)
?
Huh, it seems you've hit a bullseye. It should morally be possible to precompose it with a BehaviourF m (Either b c) ()
. I was so sure that there is a function that does that, but it seems there isn't. Definitely a missing feature. We should open a new ticket for that.
You can definitely postcompose with a pure function: https://hackage.haskell.org/package/rhine-0.7.1/docs/src/FRP.Rhine.SN.Combinators.html#%3E%3E%3E%5E It should be easy to generalise this to a Kleisli arrow a -> m b
, in case you want to add a new simple combinator. But it's not so trivial to generalise to an MSF
, I want to try that myself.
I've committed the application code with the refactor and added the
compRhines
shenannigans at the bottom, hoping there's a solution down that path too.
Not in general I believe.
We cannot do MonadIO here. UnliftIO works in principle, but I don't think it's worth the dependency. I'm working on another solution to concurrency currently which removes the need for schedules, so we shouldn't put too much effort in that direction. -- Diese Nachricht wurde von meinem Android-GerΓ€t mit K-9 Mail gesendet.
Ok I believe I made further progress, and the example does execute properly with the 2 approaches, but I'm sure I'm still not quite there yet:
LocalTerminal
to the TerminalEventClock
, and it makes writing the instance easy, but I have to thread term
through which is not great. (Is the fact that TerminalT
, while a ReaderT under the hood, doesn't export its constructor causing trouble here?)Terminal t
constraint instead of using LocalTerminal
as I think it would help testing (with VirtualTerminal
)instance Clock IO TerminalEventClock
I have an instance Clock (TerminalT LocalTerminal IO) TerminalEventClock
otherwise I struggle with being able to run runTerminalT
once at the top and providing the MonadScreen m
constraints. I get the feeling I'm missing something here. TerminalEventClock
seems wrong. But how can we write one?terminalConcurrently term
in the mainRhine
. Should I try and use concurrently
here instead? It seems that in that case I have to renounce calling runTerminalT
at the top level?The alternate approach with post-composing the ClSF works (ce10eca)! I'm wondering now if there's a way to make this construction more "symmetric". It seems arbitrary somehow that the Rhine
is the sensor
while the display
is the ClSF
, why not the other way around? I understand that the idea is that the Rhine has a clock value, so we make sure we can only compose I suppose "symmetry" could be "restored" if:
sensor'
as a ClSF
, compose it with display
and then only later make a Rhine
? I would need to be able to run beat
and key
in parallel as ClSF
s but that requires a SN
, so we're back to post-composing the display
ClSF
with the SN
but now with >--^
instead of @>-^
which still feels "asymmetric" to me.ClSF
s into a top level SN
and then separately build a clock tree value that we then apply to build a Rhine
? Looking forward to your always helpful and edifying comments!
I tried tests but I'm not sure what to do here:
void $ liftIO $ forkIO $ runTerminalT (flow $ testRhine t) t
If I don't forkIO
then I get the error
rhine-terminal-tests: thread blocked indefinitely in an MVar operation
I have the feeling I shouldn't use flow
here...
I've added a few more features to the example e212594, following the "alternate" approach made possible by the Rhine ClSF post-composition and refactoring the app with that style.
So good things are:
But on the negative side:
J
when hitting enterHope you can take a look and advise π
I think the main problem is that you're using a concurrently
schedule for both SignalClock
and InputClock
. This can't work: It starts two listeners on each clock, and the GHC runtime decides randomly which will be activated with the next event. Meaning that sometimes the SignalClock
will swallow the input and discard it.
Instead you need to use schedSelectClocks
to schedule two SelectClock
s. It will make sure that the main clock will only be started once, and all the events broadcast to each subsystem.
I also get this funny J
popping up each time I press return, no idea where that is coming from.
I took the liberty and added two commits to your branch fixing this.
Either way, I'm still not convinced this is the best layout for the app. The point of Rhine is to split components up into the different clock regions, and by writing display
we're mashing everything together again and need to multiplex with a case
. This is only necessary if the different components need to share some state somehow, but this doesn't seem to be the case here.
I tried tests but I'm not sure what to do here:
void $ liftIO $ forkIO $ runTerminalT (flow $ testRhine t) t
If I don't
forkIO
then I get the errorrhine-terminal-tests: thread blocked indefinitely in an MVar operation
I have the feeling I shouldn't use
flow
here...
No, this is fine. flow
is blocking, so the test wouldn't be productive if you have flow
in the main thread.
Instead you need to use schedSelectClocks to schedule two SelectClocks.
Awesome, I understand now! Maybe schedSelecClocks
should have concurrently
in its name given this pattern:
( ( input ++@ schedSelectClocks @++ signal )
++@ concurrently @++ prompt
I took the liberty and added two commits to your branch fixing this.
Thanks, glad you did!
I'm still not convinced this is the best layout for the app.
That's interesting. I think I'm using this style because indeed as you pointed, I'm envisioning something where state is shared between components and that's not reflected in the current example. Maybe for educational purposes it makes sense to have a simpler example where things are wired as you suggested, and another which has shared state? I've been wondering whether examples could go in the rhine-examples
package, a TerminalSimple.hs
example and a TerminalStateful.hs
example could help to demonstrate different patterns?
This is only necessary if the different components need to share some state somehow
Is there a structure here where
beat >-> displayBeat ||@ concurrently @|| key >-> displayKey
can be "distributed" under some condition as
beat ++@ concurrently @++ key @>-^ (displayBeat ?? displayKey)
Where ??
is the case multiplexing (an ||
for consuming multiplexed sources). I think that's the essence of my diagram from before, and even though I agree in my example there's no reason to lay it out like that, I'm trying to make a mental model of how I can compose sources and sinks in that way (and it's also useful to know when I shouldn't or cannot).
Also in your refactor, you "pushed down" the runTerminalT
from the top level, to inside display
. It makes me see a pattern, and I wonder if it makes sense to see things this way. we have a runTerminalT
inside the TerminalEventClock
, therefore in both SelectClock
that are made from it, and "in" the 2 sources which depend on terminal effects. Then on the other side, in the display
sink which also depends on terminal effects.
I'm wondering:
LiftClock IO AppT
, as now all sources run in IO
because we pushed runTerminalT
down?terminalConcurrently
is actually not needed anymore? Or should we keep it to allow arranging the monad transformer stack differently?beat
are different than the effects of key
, but maybe that doesn't make sense :) No, this is fine. flow is blocking, so the test wouldn't be productive if you have flow in the main thread.
Ok, sorry but I'm still not sure how to make the tests work, any other hints that could help make these tests pass?
Also, I'm happy that we try to converge towards a simple example in order to merge and discuss other topics in another PR or even outside this repo if you'd prefer. Just let me know :)
I'm still not convinced this is the best layout for the app.
That's interesting. I think I'm using this style because indeed as you pointed, I'm envisioning something where state is shared between components and that's not reflected in the current example.
Ah, yes then you're on the right track!
Maybe for educational purposes it makes sense to have a simpler example where things are wired as you suggested, and another which has shared state? I've been wondering whether examples could go in the
rhine-examples
package, aTerminalSimple.hs
example and aTerminalStateful.hs
example could help to demonstrate different patterns?
That's a very good idea. TerminalSimple.hs
could be the example application inside rhine-terminal
, and a bigger example (potentially using also, say, the gloss backend or some other clocks) could reside in rhine-examples
. It's also possible to put two example applications in rhine-terminal
, if they only use the terminal.
Also, I'm happy that we try to converge towards a simple example in order to merge and discuss other topics in another PR or even outside this repo if you'd prefer. Just let me know :)
Yes, that would be great. How about putting the simple example in this PR so we are sure that the library works, and a more elaborate one in a second PR?
Also, before that second PR I might merge #171 which will simplify the schedule story, which might also simplify the bigger second example application.
This is only necessary if the different components need to share some state somehow
Is there a structure here where
beat >-> displayBeat ||@ concurrently @|| key >-> displayKey
can be "distributed" under some condition as
beat ++@ concurrently @++ key @>-^ (displayBeat ?? displayKey)
Where
??
is the case multiplexing (an||
for consuming multiplexed sources). I think that's the essence of my diagram from before, and even though I agree in my example there's no reason to lay it out like that, I'm trying to make a mental model of how I can compose sources and sinks in that way (and it's also useful to know when I shouldn't or cannot).
I think that should be (+++)
or (|||)
from ArrowChoice
. (ClSF
s are Arrow
s.) If you like arrow notation, you can also do something like:
displayBeatOrKey = proc input do
advanceJointState -< input
case input of
Left beat -> displayBeat -< beat
Right key -> displayKey -< key
Also in your refactor, you "pushed down" the
runTerminalT
from the top level, to insidedisplay
. It makes me see a pattern, and I wonder if it makes sense to see things this way. we have arunTerminalT
inside theTerminalEventClock
, therefore in bothSelectClock
that are made from it, and "in" the 2 sources which depend on terminal effects. Then on the other side, in thedisplay
sink which also depends on terminal effects.I'm wondering:
* Is that why BeatClock lost its `LiftClock IO AppT`, as now all sources run in `IO` because we pushed `runTerminalT` down?
Yes. This makes the type signatures simpler for now.
* Does this mean `terminalConcurrently` is actually not needed anymore? Or should we keep it to allow arranging the monad transformer stack differently?
Yes, it should not be needed. (And after #171 it will not be needed at all.)
* Could this be done in an extensible effect kind of way? Where we'd declare the effects of a part of the clock regions, and "consume" them elsewhere, to keep track of them? I suppose I'd like to see more clearly in the types that the effects of `beat` are different than the effects of `key`, but maybe that doesn't make sense :)
Yes, that's a good way to do this. Ideally the clock instances are all of the form
instance MonadFoo m => Clock m FooClock where
initClock = foo ...
Then we never need to specify m
exactly while we compose Rhine
s (thus avoiding LiftClock
etc.). Then at the end after we do eraseClock
or flow
(and thus leave the clock types), we can handle the effects by instantiating with a particular monad or transformer, and running it.
An example for that approach might be https://github.com/turion/rhine/pull/171/files#diff-aff02ce436bc3b78434667d310d71ec7b7da917c52512daa8e055831346673e1, but there I use eraseClock
instead of flow
, which may be confusing.
No, this is fine. flow is blocking, so the test wouldn't be productive if you have flow in the main thread.
Ok, sorry but I'm still not sure how to make the tests work, any other hints that could help make these tests pass?
Ah sorry, I wasn't aware that they're failing. I'll look into it now.
I believe the reason the test is failing is because the virtual terminal doesn't emit events when executing the PutText
command.
I think you'd need to do something else instead. Probably starting flow rhineTerminal
in a thread and then maybe use hPut
or so to simulate input? I haven't tried this, maybe it is necessary to instead start a process with hackage.haskell.org/package/process that produces input.
If testing is too complicated, let's put it in another PR. As you might have noticed, there are no tests for the other packages either, for similar reasons.
As a simple alternative, we can emulate the key event with a TQueue
. Example commit attached. I think this is the best option for now since it works and is simple.
Hi @turion
Yes, that would be great. How about putting the simple example in this PR so we are sure that the library works, and a more elaborate one in a second PR?
I kept the more involved example out of this PR as you suggested, and kept a version that's closer in spirit to the fixes you had initially made to my original code. I've submitted the other example in its current state as a draft PR and will continue building onto it, hopefully with your help.
Also, before that second PR I might merge https://github.com/turion/rhine/pull/171 which will simplify the schedule story, which might also simplify the bigger second example application.
Such a great simplification! I looked at monad-schedule
and feel like it's a really promising abstraction! Looking forward to give it a spin :)
I think that should be (+++) or (|||) from ArrowChoice.
That's a good suggestion, I'll play with Arrow notation in the second example.
Ideally the clock instances are all of the form
instance MonadFoo m => Clock m FooClock where initClock = foo ...
Yeah I'd like to revisit this, as it feels that we should be able to use the MonadTerminal
constraint here.
As a simple alternative, we can emulate the key event with a TQueue. Example commit attached. I think this is the best option for now since it works and is simple.
That's really great π Thanks a ton.
I believe these are now all completed:
[x] If you're motivated add an integration test that produces some characters on stdout.
I've made a few cosmetic changes too, so feel free to re-review in depth π
UPDATE: Will just make sure the CI passes UPDATE2: Funny that stack is giving me trouble... It was not so long ago that cabal was hell, but seems like the mantle was passed π
Oh I just realised there is a loose end still:
the semigroup instance for TerminalEventClock seems wrong. But how can we write one?
instance Terminal t => Semigroup (TerminalEventClock t) where
t <> _ = t
What does it mean and can we do better?
Oh I just realised there is a loose end still:
the semigroup instance for TerminalEventClock seems wrong. But how can we write one?
instance Terminal t => Semigroup (TerminalEventClock t) where t <> _ = t
What does it mean and can we do better?
The reason for that is https://hackage.haskell.org/package/rhine-0.8.0.0/docs/FRP-Rhine-Clock-Select.html#v:schedSelectClocks which needs a Semigroup
instance to schedule the clocks. This requirement will go away with #171.
We want to combine two terminals into one somehow, so if we have two SelectClock (TerminalEventClock t)
, we could make a single terminal that emits all the events for the subclocks to select from. But that doesn't make much sense, especially not if we don't know the type of t
.
For me this seems to be a further hint that we actually want the ReaderT
pattern in https://github.com/turion/rhine/pull/165#pullrequestreview-1026834756. (Sorry that I initially suggested otherwise, but I didn't understand terminal
well enough.)
I'm still not entirely sure whether the Terminal value should reside in the clock, or in a Reader monad.
Yes that makes sense. In fact I tried to go the Reader route earlier but got stuck because TerminalT
although a newtype over ReaderT
doesn't export its constructor nor has a MonadReader
instance... Unless I'm mistaken, we can't work around this, so I'll submit a PR and we can depend on a fork for now.
Unless I'm mistaken, we can't work around this, so I'll submit a PR and we can depend on a fork for now.
I was mistaken. I could write the instance without needing the TerminalT
constructor.
Unless I'm mistaken, we can't work around this, so I'll submit a PR and we can depend on a fork for now.
I was mistaken. I could write the instance without needing the TerminalT constructor.
I'm stuck again.
I can't use:
mainRhine = inputRhine ||@ concurrently @|| promptRhine
β’ Couldn't match type βIOβ with βTerminalT LocalTerminal IOβ
Expected: Schedule App InputClock PromptClock
Actual: Schedule IO InputClock PromptClock
So it seems I need a terminalConcurrently
but I'm stuck as I want to try avoiding importing the TerminalT
constructor to be able to ask
for the Terminal t
value inside the TerminalT (ReaderT t m a)
.
But then I don't know how to write terminalConcurrently
without it.
Also I think this is unrelated, but just wanted to check that instead of using a concrete TerminalT t m
(here still IO
) I should also probably use a MonadTerminal m
constraint in flowTerminal
.
This is a tough one. I guess either opening an upstream PR making the constructor available and using a fork until then, or using unsafeCoerce
to replace the constructor (not sure this works though).
Note: The problem doesn't go away after #171 because there we also need to construct a TerminalT
.
I tried the unsafeCoerce version. It's ugly, but I don't know whether you'd be willing to maintain a fork otherwise. EDIT: But it works in fact!
Ok I believe I've addressed all your comments.
I hope you'll bear with my meticulous review style π I can find a lot of small details if I go looking.
Not at all! Thanks for your patience, I didn't have tons of time in the past couple of weeks :) I think I've addressed all your comments. Feel free to request more changes if you catch anything π
As per https://github.com/turion/rhine/issues/161 this is my attempt at using the
haskell-terminal
library withrhine
and succeeding very moderately, as this works dreadfully slowly.I've stacked the PR on top of my
flake
branch so it's easy to build and run this way:Running this should show a prompt, with a blinking
.
before a>
and a terminal cursor after it. When pressing keys, they appear but after a long delay. This slow behavior exists both onaarch64
andx86
. Let me know if you can reproduce :)I haven't yet split out the Terminal related functionality into a module like
FRP.Rhine.Gloss
yet as we work through the performance problems!