Skip to content
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

rhine-terminal backend with haskell-terminal library and Repl like example #165

Merged
merged 33 commits into from
Jul 27, 2022

Conversation

jmatsushita
Copy link
Contributor

As per #161 this is my attempt at using the haskell-terminal library with rhine 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:

nix build .#rhine-terminal
result/bin/rhine-terminal-repl

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 on aarch64 and x86. 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!

@turion
Copy link
Owner

turion commented May 24, 2022

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.

@turion
Copy link
Owner

turion commented May 24, 2022

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.

@turion
Copy link
Owner

turion commented May 24, 2022

Merge #164 first.

rhine-terminal/Main.hs Outdated Show resolved Hide resolved
@turion
Copy link
Owner

turion commented May 24, 2022

I've given this a spin. No idea what it's supposed to do exactly.

  • It just worked with nix develop. awesome!
  • CPU usage is at 1% or less, which is good.
  • It seems that if I type several characters, they're buffered and only one is printed at a time. Is that what you mean by it being slow? I'll have a look, maybe I can figure it out.
  • There's no way to exit the repl. ctrl+d or esc would be great.

@turion
Copy link
Owner

turion commented May 24, 2022

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.

@turion
Copy link
Owner

turion commented May 24, 2022

Ironically, this is exactly the kind of bug rhine is supposed to resolve 😆 (but at least it was clear to see and reason about)

rhine-terminal/Main.hs Outdated Show resolved Hide resolved
Copy link
Owner

@turion turion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, so this seems in good shape for me to go forward! It seems terminal can be used as a rhine backend. Some todos to proceed:

  • Add rhine-terminal to cabal.project and stack.yaml.
  • Factor out the terminal clock to a library.
  • Keep the executable as an example application.
  • If you're motivated add an integration test that produces some characters on stdout.

@jmatsushita
Copy link
Contributor Author

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 🙏

@jmatsushita
Copy link
Contributor Author

Hi @turion ,

So I got started on the todo list:

  • Add rhine-terminal to cabal.project and stack.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 #166 merged beforehand?

  • Factor out the terminal clock to a library.
  • 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!

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:

image

However this leaves me wondering whether it's possible to fix the problem while keeping the original intent composing sensors and actuators like so:

image

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.

@jmatsushita jmatsushita requested a review from turion June 10, 2022 20:17
.envrc Outdated Show resolved Hide resolved
rhine-terminal/Main.hs Outdated Show resolved Hide resolved
rhine-terminal/Main.hs Outdated Show resolved Hide resolved
rhine-terminal/Main.hs Outdated Show resolved Hide resolved
@turion
Copy link
Owner

turion commented Jun 13, 2022

  • Add rhine-terminal to cabal.project and stack.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.

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.

  • Factor out the terminal clock to a library.
  • 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.

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!

👍

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:

image

However this leaves me wondering whether it's possible to fix the problem while keeping the original intent composing sensors and actuators like so:

image

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.

  1. Clocks in rhine have identity. These are called "clock values". A signal network doesn't contain a clock (although it requires a specific clock type), but a Rhine does. So you'd force the two clocks in these Rhines to be identical, although they can be different values of the same type. This would at most work with Monoid cl.
  2. Even then 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.

rhine-terminal/Main.hs Outdated Show resolved Hide resolved
rhine-terminal/Main.hs Outdated Show resolved Hide resolved
@turion
Copy link
Owner

turion commented Jun 13, 2022 via email

@jmatsushita jmatsushita force-pushed the terminal branch 2 times, most recently from 570615f to d3be9ff Compare June 18, 2022 13:10
@jmatsushita jmatsushita requested a review from turion July 6, 2022 09:10
flake.nix Outdated Show resolved Hide resolved
Copy link
Owner

@turion turion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope you'll bear with my meticulous review style 😅 I can find a lot of small details if I go looking.

flake.nix Outdated Show resolved Hide resolved
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE UndecidableInstances #-}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I use Text OverloadedString is nice. The rest is not used indeed,

rhine-terminal/TerminalSimple.hs Outdated Show resolved Hide resolved
Comment on lines 47 to 50
, select }
where
select :: Tag TerminalEventClock -> Maybe Input
select = \case
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact if you use \case, you can define it directly without a where clause! I like that a lot. I'll use \case more in my own code I think.

rhine-terminal/TerminalSimple.hs Outdated Show resolved Hide resolved
rhine-terminal/src/FRP/Rhine/Terminal.hs Show resolved Hide resolved
rhine-terminal/src/FRP/Rhine/Terminal.hs Show resolved Hide resolved
rhine-terminal/src/FRP/Rhine/Terminal.hs Show resolved Hide resolved
rhine-terminal/src/FRP/Rhine/Terminal.hs Outdated Show resolved Hide resolved
rhine-terminal/tests/Main.hs Outdated Show resolved Hide resolved
@jmatsushita
Copy link
Contributor Author

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 👍

Copy link
Owner

@turion turion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! Thanks a lot for contributing!

@turion turion merged commit 8b935a0 into turion:master Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants