Seqexec execution with a Zipper #85

jdnavarro · 2016-09-21T15:53:39Z

With the Zipper there is now a deeper data hierarchy with a top level Queue, _Sequence_s, _Step_s, _Execution_s and _Action_s. Previously there were only the equivalent of _Execution_s and _Action_s. Also with this structure it comes support for marking Executions as completed instead of remove them.

The executions for each test can be seen printed to the console. The tests themselves only check that the executions finish.

Don't bother too much reviewing this PR since the changes are quite messy. It can be considered as a starting point for the Zipper based execution.

These 2 functions help with initialization and finalization of a Seqexec queue. Now `next` is only used when the execution is ongoing.

cquiroz · 2016-09-21T18:05:26Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

+  def actions: List[Action] = {
+
+    // not available in scalaz?
+    def lefts[L, R](xs: List[L \/ R]): List[L] = for { -\/(r) <- xs } yield r


leftMap perhaps?

I was thinking about the Haskell lefts. leftMap maps over every left but I just want to collect every left in a list of \/. I'm pretty sure there'll be something in scalaz to do that but can't find how it's named.

ok, you can do collect on the list. something like

xs.collect { case -\/(e) => e }

or perhaps

xs.collect { case e @ -\/(_) => e }

.collect is probably the most efficient solution but you could also say .separate._1. Scalaz would take a PR if you wanted to add this to MonadPlus.

I try to avoid collect if there is another alternative with total functions but this looks like an ideal case.

I didn't know about .separate._1. I'll also look into the laws for MonadPlus in Scalaz. In Haskell there is no consensus about which laws should a MonadPlus obey and I'm curious to see which ones Scalaz sticks to.

cquiroz · 2016-09-21T18:12:35Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

  /**
    * Transform an *unconsed* pending `Execution` into `Current` in addition to
    * returning the unwrapped `Execution`.
    */
  // TODO: Use same structure for `Current` and `Queue.Execution3`?
-  private def currentify(exe3: Queue.Execution3[Action]): (Execution[Action], Current) = {
+  private def currentify(exe3: Queue.Execution3[Action]): (Current) = {


What do you mean by cons and uncons?

To be honest I didn't really know what cons stands for until I looked it up but it's often used when adding an element to the front of something that looks like a single linked list. I guess the term is more widespread in Haskell/Lisp than anywhere else.

I could rename it if there is another term for that in Scala. How is :: pronounced in Scala?

In any case these functions should be internal to just that module.

Right :: is called cons in a list. I just wondered what was the use here. I haven't seen uncons though

Why's the return type in parentheses?

It's not clear to me how the opposite of cons should be named, I've seen it as uncons and snoc. Monocle uses snoc so if we end up using Monocle, snoc would be a better name.

I forgot to remove the parentheses, good catch.

snoc typically is right-append. I think uncons is more typical.

Apparently SEQNG-4 is no longer an issue after fixing SEQNG-1 and SEQNG-3. This test only prints the output to console, it doesn't test the execution is correct on its automatically.

jdnavarro · 2016-09-21T19:55:38Z

After writing more tests I realized the completed Executions are not added properly. I'll include the fix in this PR.

cquiroz · 2016-09-21T21:41:28Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

-      case -\/(-\/((actions, seqid, stepid))) => (actions, Current(vec(actions), Some((seqid, stepid).left)))
-      case -\/(\/-((actions, stepid))) => (actions, Current(vec(actions), Some(stepid.right)))
-      case \/-(actions) => (actions, Current(vec(actions), None))
+      case -\/(-\/((actions, seqid, stepid))) => Current(vec(actions), Some((seqid, stepid).left))


it is a bit strange to see two \/ nested. what do they represent?

Each sum type of the \/ represents if either a Sequence or a Step or a Execution in the current Step was completed. It's explained in the documentation for that function but I should put a small comment for each case pattern to make the code easier to follow.

\/ with more than 2 types was strange for me too because in Haskell you can't do that. One of the nice features of \/ is that it supports sum types of arbitrary number of elements, however in practice it turns out it's not so great because as you can see, pattern matching is not so straightforward.

The solution for this is to create a custom sum data type. This will be necessary, in any case, when adding more complex Steps so I'm writing this down as an improvement to do later.

In any case this weird sum types are internal to this object, they are not meant to be used anywhere else.

In think it's fine this way, unless you think a custom type will add value

Shapeless provides an open coproduct type that lets you express A \/ B \/ C as A :+: B :+: C :+: CNil, and then you can fold by applying a polytypic function. It's much nicer in general but probably not introducing for isolated cases like this.

The way \/ handles more than 2 types with those constructors is a bit hacky. For now it's fine, but I'll keep and eye on Shapeless if I ever need more robust sum types.

cquiroz · 2016-09-21T21:42:00Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

@@ -187,7 +201,8 @@ object Queue {
          mseq match {
            // No more Steps in current Sequence, remove Sequence.
            // TODO: listTailPLens?
-            case None => ((exe, seq0.id, stepid).left.left, Queue(q.sequences.tailOption.getOrElse(List())))
+            case None => ((exe, seq0.id, stepid).left.left,
+                          Queue(q.sequences.tailOption.getOrElse(List())))


Often List() is represented as Nil

Good to know. I think I have more instances of List() spread over the code, so I'll change them all.

cquiroz · 2016-09-21T21:44:40Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/package.scala

+        gets(_.current.actions) >>= (
+          actions => Nondeterminism[Task].gatherUnordered(
+            actions.zipWithIndex.map(act(_))
+          ).liftM[EngineStateT]


Can you remind me what liftM does here?

This is just for lifting a Task into Engine: Task[A] ~> Engine[A]. Engine is a monad transformer of just 1 layer, the StateT with the Seqexec State over Task as the base monad.

What I got from the discussion we had a while back, with @swalker2m and @tpolecat too, is that 1 layer is manageable but more than 1 layer become too cumbersome in Scala. It seems that when the need for more transformer layers arise all the solutions lean towards the use of Free Monads for cleaner and more composable effects.

I wanted to experiment first with a simple toy example combining 2 effects using Free monads and coproducts before trying to do it in the Seqexec code, but so far I could get away with just 1 transformer layer without too much trouble.

However, I don't think the code will be able to scale in complexity properly with just 1 layer. For example, another layer I wish we had is a Reader monad (a.k.a Kleisli) with the scalaz-stream queue so that we don't have to be passing the queue explicitly as a parameter all over the code. Also, a Reader will be inevitable when/if we need to access a global configuration object from the engine.

So at some point all those liftM should go away.

right, the config system uses Kliesli for configuration. About the scalaz-stream queue the current implementation has a single queue in an object becoming global. Would that work instead of passing it as a param?

A global mutable variable (in order to create a queue we would have to do IO at some point) brings me painful memories from my Python times :). In the concrete case of queue as a parameter it looks like it would be OK to use an unsafe global: it's only created once and it's only read everywhere and not meant to be modified.

But since it seems will have to include the Kleisli for configuration anyway, we could piggyback the queue in that Kleisli.

Actually in ocs3 the queue is passed as a variable, but initialized on the object construction

https://github.com/gemini-hlsw/ocs3/blob/8c859ddb61d920a8faf8d6d5e5f00737dc3812e2/modules/edu.gemini.seqexec.server/src/main/scala/edu/gemini/seqexec/server/ExecutorImpl.scala#L127

So passing it on the Kliesli sounds equivalent

Oh, now I see what you mean, I didn't consider that pattern. I'm reading now about TaskRef to understand how ExecutorImpl works. I also realize that the DhsClient will have to follow a similar pattern to the scalaz-stream queue creation, so I think it's worth exploring the possibilities.

In the end, I think it'll be worth having something similar to an Environment object embedded in a Kleisli with everything that needs to be read-only throughout the Seqexec code. This object can be created only once, when initialized.

tpolecat

A few comments.

tpolecat · 2016-09-22T17:01:22Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

-                  status: Status)
+                  status: Status) {
+
+  def isEmpty: Boolean = this.pending.sequences.isEmpty && this.current.isEmpty


Minor stylistic thing; it's not idiomatic to use this. for members unless there is a need to disambiguate.

I was reluctant to use this but wanted to make isEmpty a method of Queue instead of a standalone function over Queue because it seems more natural for a publicly exposed API.

Is there a more functional way to implement this kind of methods? The Syntax Ops packages seem a bit overkill but perhaps they are worth writing for these cases anyway.

Never mind my previous comment. I just realized the parameters of a case class are in scope in its case class block.

tpolecat · 2016-09-22T17:02:33Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

-    (exe3pending, qp) <- Queue.uncons(st.pending)
-    (actions, c) = currentify(exe3pending)
-  } yield (actions, QState(qp, c, qd, st.status))
+  def next(qs: QState): Option[QState] = cleanup(qs) >>= (prime(_))


You should be able to write this as cleanup(qs) >>= prime.

tpolecat · 2016-09-22T17:05:18Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

+        t => {
+         val (exe3, q) = t
+         QState(q, currentify(exe3), qs.done, qs.status)
+        }


A function literal can be expressed in cases.

Queue.uncons(qs.pending).map { case (exe3, q) => QState(q, currentify(exe3), qs.done, qs.status) }

tpolecat · 2016-09-22T17:06:16Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

  /**
    * Transform an *unconsed* pending `Execution` into `Current` in addition to
    * returning the unwrapped `Execution`.
    */
  // TODO: Use same structure for `Current` and `Queue.Execution3`?
-  private def currentify(exe3: Queue.Execution3[Action]): (Execution[Action], Current) = {
+  private def currentify(exe3: Queue.Execution3[Action]): (Current) = {


snoc typically is right-append. I think uncons is more typical.

tpolecat · 2016-09-22T17:09:51Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

-      case -\/(-\/((actions, seqid, stepid))) => (actions, Current(vec(actions), Some((seqid, stepid).left)))
-      case -\/(\/-((actions, stepid))) => (actions, Current(vec(actions), Some(stepid.right)))
-      case \/-(actions) => (actions, Current(vec(actions), None))
+      case -\/(-\/((actions, seqid, stepid))) => Current(vec(actions), Some((seqid, stepid).left))


Shapeless provides an open coproduct type that lets you express A \/ B \/ C as A :+: B :+: C :+: CNil, and then you can fold by applying a polytypic function. It's much nicer in general but probably not introducing for isolated cases like this.

tpolecat · 2016-09-22T17:14:33Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/model.scala

+  def actions: List[Action] = {
+
+    // not available in scalaz?
+    def lefts[L, R](xs: List[L \/ R]): List[L] = for { -\/(r) <- xs } yield r


.collect is probably the most efficient solution but you could also say .separate._1. Scalaz would take a PR if you wanted to add this to MonadPlus.

tpolecat · 2016-09-22T17:17:17Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/package.scala

+        case Result.OK => q.enqueueOne(completed(i))
+        case Result.Error => q.enqueueOne(failed(i))
+      }
+    }


Prefer match over destructuring assignment because you'll get exhaustiveness checking. Also in general use flatMap unless >>= is clearly better because it's more efficient.

... = t match { case (action, i) => action.flatMap { case ... } }

Oh, I thought destructuring assignments were syntactic sugar for _ match {.... Also, TIL flatMap and >>= are not the same! I thought they were aliases of each other. Good to know.

No, destructing assignments are not checked carefully so you can get a match error at runtime. >>= is equationally identical to flatMap but has higher overhead because there is an intervening implicit conversion. In practice it rarely matters but it's good to remember.

tpolecat · 2016-09-22T17:18:02Z

modules/edu.gemini.seqexec.engine/src/main/scala/edu/gemini/seqexec/engine/package.scala

+      case Status.Running => (
+        gets(_.current.actions) >>= (
+          actions => Nondeterminism[Task].gatherUnordered(
+            actions.zipWithIndex.map(act(_))


You can probably say .map(act) here.

tpolecat · 2016-09-22T17:22:01Z

I don't really understand the new code review feature, sorry. Some of my comments are out of context because I added them to running conversations. So maybe look at the code tab.

Among them: - case deconstruction instead of assignment deconstruction. - `>>=` -> `flatMap`. - Remove `this`. - Use `collect` instead of list monad with for-comprehension. - More commenting. This takes care of comments for gemini-hlsw#85.

jdnavarro · 2016-09-26T18:49:14Z

The completion of Executions is still broken but since I'm realizing it will take some time to fix it, will it be OK to merge this PR as it is?

Among them: - case deconstruction instead of assignment deconstruction. - `>>=` -> `flatMap`. - Remove `this`. - Use `collect` instead of list monad with for-comprehension. - More commenting. This takes care of comments for gemini-hlsw#85.

Switch to Wavelength

SEQNG-1 SEQNG-3 Introduce prime and cleanup #resolve

c8b76f3

These 2 functions help with initialization and finalization of a Seqexec queue. Now `next` is only used when the execution is ongoing.

cquiroz reviewed Sep 21, 2016

View reviewed changes

SEQNG-4 Add test with pause #resolve

72c4ac5

Apparently SEQNG-4 is no longer an issue after fixing SEQNG-1 and SEQNG-3. This test only prints the output to console, it doesn't test the execution is correct on its automatically.

jdnavarro changed the title ~~SEQNG-1 SEQNG-3 Introduce prime and cleanup #resolve~~ Seqexec execution with a Zipper Sep 21, 2016

cquiroz reviewed Sep 21, 2016

View reviewed changes

tpolecat reviewed Sep 22, 2016

View reviewed changes

jdnavarro added 2 commits September 22, 2016 16:40

Stylistic/Optimization changes

96bff42

Among them: - case deconstruction instead of assignment deconstruction. - `>>=` -> `flatMap`. - Remove `this`. - Use `collect` instead of list monad with for-comprehension. - More commenting. This takes care of comments for gemini-hlsw#85.

Add more Engine tests

2040320

jdnavarro merged commit 9a8f153 into gemini-hlsw:feature/engine Sep 27, 2016

cquiroz pushed a commit to cquiroz/ocs3 that referenced this pull request Jun 11, 2018

Merge pull request gemini-hlsw#85 from swalker2m/gmos_central_wavelength

788b1d5

Switch to Wavelength

Seqexec execution with a Zipper #85

Seqexec execution with a Zipper #85

Conversation

jdnavarro commented Sep 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdnavarro commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdnavarro Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdnavarro Sep 22, 2016 • edited

Choose a reason for hiding this comment

tpolecat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpolecat Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdnavarro Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpolecat commented Sep 22, 2016

jdnavarro commented Sep 26, 2016

jdnavarro commented Sep 21, 2016 •

edited

jdnavarro Sep 22, 2016 •

edited

jdnavarro Sep 22, 2016 •

edited

tpolecat Sep 22, 2016 •

edited

jdnavarro Sep 22, 2016 •

edited