[GAWB-2056] The status of the last run is showing green when there are errors in them #709

rtitle · 2017-06-09T21:20:00Z

https://broadinstitute.atlassian.net/browse/GAWB-2056 (see latest comments)

Problem:

In "get workspace" calls, Rawls was returning the timestamps of the most recent successful and failed workflows. This is used in the UI to display either green or red in the workspace summary and list pages (red if latest failed workflow timestamp > latest successful workflow timestamp; green otherwise).

However, these workflows might be in the same submission. In the UI, a submission is red if any of its workflows failed. This led to confusing behavior: a workspace might be green in the Summary tab, but have only failed submissions in the Monitor tab.

Solution:

Change semantics of WorkspaceSubmissionStats to return the timestamps of the most recent successful and failed submission, taking into account the logic that a submission is a failure if any of its workflows are a failure.

Tested via unit tests and manually using a UI. Also confirmed the generated slick query matches the query in the comments.

Also, query explain plan:

Review cycle:
- LR reviews
- Rest of team may comment on PR at will
- LR assigns to submitter for feedback fixes
- Submitter rebases to develop again if necessary
- Submitter makes further commits. DO NOT SQUASH
- Submitter updates documentation as needed
- Submitter reassigns to LR for further feedback

…most recent submission, not the most recent workflow

rtitle · 2017-06-09T21:24:33Z

core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala

@@ -731,14 +759,6 @@ trait WorkspaceComponent {
      WorkspaceGroups(toGroupMap(realmAclRecs), toGroupMap(accessGroupRecs))
    }
  }
-
-  private def groupByWorkspaceId(runningSubmissions: Seq[(UUID, Int)]): Map[UUID, Int] = {


These methods seemed useful so I cats-ified and generalized them and moved to DriverComponent.

helgridly · 2017-06-12T14:53:58Z

core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/DriverComponent.scala

+    * }}}
+    */
+  def groupPairsK[F[_], A, B](pairs: Seq[(A, F[B])])(implicit M: MonoidK[F]): Map[A, F[B]] =
+    groupPairs(pairs)(M.algebra[B])


something tells me it'll be a while before this PR gets merged! (i have to wrap my head around this)

Yeah sorry if this got a little fancy.. it's not really too bad. Check out:
https://github.com/typelevel/cats/blob/master/docs/src/main/tut/typeclasses/semigroupk.md

A bunch of feedback here.

Check out CollectionUtils - seems like a good place for these to go.

I don't understand this.

Link to the HTML version of the documentation: it contains hugely important comment lines that aren't rendered inside GitHub!

sbt console gives me this when I try to define groupPairsK:

<pastie>:18: warning: higher-kinded type should be enabled by making the implicit value scala.language.higherKinds visible. This can be achieved by adding the import clause 'import scala.language.higherKinds' or by setting the compiler option -language:higherKinds. See the Scaladoc for value scala.language.higherKinds for a discussion why the feature should be explicitly enabled.

groupPairs returns a map. You're then dereferencing the key defined by M.algebra[B]? I don't know what that does and I'd have thought that this would give you a result type of F[B].

Your groupPairsK example is equivalent to toMap, which doesn't help me understand. Perhaps

scala> groupPairsK(Seq(("a", Foo(1).some), ("b", Foo(2).some), ("b", Foo(3).some))) res0: Map[String,Option[Foo]] = Map(b -> Some(Foo(2)), a -> Some(Foo(1)))

would be more helpful, assuming I've read the Cats documentation (and therefore know that in the absence of a quantified F (= Option) means it'll just do orElse).

TLDR: I am 👍 on groupPairs and groupTriples, though they should go into CollectionUtils. I am 👎 on the K-versions; they rely on non-obvious behaviour (e.g. orElse for Option) that make them hard to understand.

Thanks for the feedback here. After reflecting a bit, maybe it would have been better to leave groupByWorkspaceIdThenStatus and groupByWorkspaceId as is -- I didn't really need to change them for this bug. I'm still learning how to scala in a large team, and readability is important.

Digging in to your points:

+1

see below

ok

Huh, I haven't seen that warning before. I had to enable the -feature compiler flag to see it. I guess it's complaining about the F[_] type parameter. The scaladoc is kind of funny:

Why control it? Higher kinded types in Scala lead to a Turing-complete type system, where compiler termination is no longer guaranteed. They tend to be useful mostly for type-level computation and for highly generic design patterns. The level of abstraction implied by these design patterns is often a barrier to understanding for newcomers to a Scala codebase. Some syntactic aspects of higher-kinded types are hard to understand for the uninitiated and type inference is less effective for them than for normal types. Because we are not completely happy with them yet, it is possible that some aspects of higher-kinded types will change in future versions of Scala. So an explicit enabling also serves as a warning that code involving higher-kinded types might have to be slightly revised in the future.

4,5. So here's why I added the K version. In this case I wanted to group triples where the map value is an Option[java.sql.Timestamp]. Therefore I need a Monoid instance for java.sql.Timestamp, which doesn't exist. It wouldn't make much sense to define a Monoid for timestamps -- what would you do, add them together? However, in this case I'm guaranteed to have no key conflicts because of the SQL structure (group by, etc). So I can just use a Monoid for Option which just takes one or the other, regardless of the value inside the "box". That's exactly what MonoidK[Option] does.

Furthermore, I'm not sure how I would implement this in terms of groupTriples without using MonoidK. Perhaps this would be more clear?

private def groupByWorkspaceIdThenStatus(workflowDates: Seq[(UUID, String, Option[Timestamp])]): Map[UUID, Map[String, Option[Timestamp]]] = { // bla bla comment about bringing a monoid into scope with Option.orElse behavior implicit val optionUniversalMonoid: Monoid[Option[Timestamp]] = MonoidK[Option].algebra[Timestamp] CollectionUtils.groupTriples(workflowDates) }

Then at least the MonoidK stuff is localized in a private method with some explanation specific to the use case.

I went ahead and pushed a commit with ^ those changes, please take a look.

helgridly · 2017-06-12T22:07:47Z

core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala

-        val workflowDatesByWorkspaceByStatus: Map[UUID, Map[String, Option[Timestamp]]] = groupByWorkspaceIdThenStatus(workflowDates)
-        val runningSubmissionCountByWorkspace: Map[UUID, Int] = groupByWorkspaceId(runningSubmissions)
+        val submissionDatesByWorkspaceByStatus = groupTriplesK(submissionDates)
+        val runningSubmissionCountByWorkspace = groupPairs(runningSubmissions)


I think the rename from groupByWorkspaceIdThenStatus -> groupTriplesK and groupByWorkspaceId -> groupPairs loses a lot of readability here. Can you keep the function definitions for readability, even if they just point to other ones?

helgridly · 2017-06-13T17:32:08Z

core/src/main/scala/org/broadinstitute/dsde/rawls/util/CollectionUtils.scala

+    * }}}
+    * */
+  def groupPairs[A, B: Monoid](pairs: List[(A, B)]): Map[A, B] =
+    pairs.foldMap { case (a, b) => Map(a -> b) }


groupByTuplesFlatten above could call this, right?

also these should probably prefer Seq to List

Re Seq: sure

I guess this would work:

//A saner group by than Scala's. def groupByTuples[A, B]( tupleSeq: Seq[(A,B)] ): Map[A, Seq[B]] = { tupleSeq.toList.foldMap { case (a, b) => Map(a -> Seq(b)) } } def groupByTuplesFlatten[A, B]( tupleSeq: Seq[(A, Seq[B])] ): Map[A, Seq[B]] = { groupPairs(tupleSeq) }

Why would it prefer Seq? Do you expect it to be taking arbitrary Seqs?

Seq is less specific and we use it all over the place. We'd have to start jamming .toList everywhere were we to use List.

Maybe people shouldn't have overused Seq and then that wouldn't be an issue

We have the same problem too and it's annoying. We occasionally run into issues where the thing really only works properly on List but things compile due to Seq and someone jammed something bad in there.

I meant just groupByTuplesFlatten anyway, but oh well

@rtitle Cuz the Cats folk understand the value in saying what you mean ;)

Thanks for your input, Jeff! When we wholesale switch from Seq to List I'll let you know so you can say I told you so.

I'll change these to take Seq (and just call toList on them).

I won't touch groupByTuples/groupByTuplesFlatten because that would require introducing monoids and I don't want to break any code.

helgridly · 2017-06-13T17:40:06Z

core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala

+    // structure (group by, etc).
+    //
+    // TL/DR: The following line brings into scope a Monoid[Option[Timestamp]] which combines values
+    // using Option.orElse.


I think part of my confusion here is that you're introducing Monoid, which is used to combine things, using a non-combining operation (orElse), in a situation where you never need to combine two of them anyway because the UUID/String combo is guaranteed to be unique. This is a pretty head-bendy way to achieve the desired result, even if it does work.

At least the orElse is explicit now. Before, in this code:

...mapValues { case Seq((_, _, timestamp)) => timestamp })

it's still expecting a unique UUID/String combo, and if that were not the case, there would be a runtime MatchError.

I still think this is less readable than doing it explicitly but I'm going on vacation tomorrow and thus can't afford to spend more time arguing about it :) Instead, if you replace your comment with the following one, I'll shut up and thumb this. In the long-term, we need a centralised place for these explanations, because throwing them in comments on their first use won't help new hires understand them if they happen upon the uncommented second or third use first. But we can worry about that (a little) later.

The function groupTriples, called below, transforms a Seq((T1, T2, T3)) to a Map(T1 -> Map(T2 -> T3)). It does this by calling foldMap, which in turn requires a monoid for T3. In our case, T3 is an Option[Timestamp], so we need to provide an implicit monoid for Option[Timestamp].

There isn't really a sane monoid implementation for Timestamp (what would you do, add them?). Thankfully it turns out that the UUID/String pairs in workflowDates are always unique, so it doesn't matter what the monoid does because it'll never be used to combine two Option[Timestamp]s. It just needs to be provided in order to make the compiler happy.

To do this, we use the universal monoid for Option, MonoidK[Option]. Note that the inner Option takes no type parameter: MonoidK doesn't care about the type inside Option, it just calls orElse on the Option for its "combine" operator. Finally, the call to algebra[Timestamp] turns a MonoidK[Option] into a Monoid[Option[Timestamp]] by leaving the monoid implementation alone (so it still calls orElse) and poking the Timestamp type into the Option.

rtitle · 2017-06-13T19:21:27Z

core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala

+      //   join workflow on workflow.submissionId = submission.id
+      //   where submission.workspaceId in (:workspaceIds)) v
+      // group by 1, 2
+      // having (status = 'Failure' or (status = 'Succeeded' and count(v.*) = 1))


@helgridly based on our conversation how does this revised query look to you?

select workspaceId, status, max(subEndDate) from ( select submission.id, submission.workspaceId, workflow.status, max(workflow.statusLastChangedDate) as subEndDate from submission join workflow on workflow.submissionId = submission.id where submission.workspaceId in (:workspaceIds) group by 1, 2, 3) v group by 1, 2 having (status = 'Failure' or (status = 'Succeeded' and count(v.id) = 1))

Explanation:

inner query returns the most recent workflow status change date, per workflow status and submission

outer query returns the most recent workflow status change date where:

the status is Failure; or

the status is Succeeded and that is the only status in the submission

I can code this up and try to break it with tests too, just thought I'd post the SQL first.

I think that's right, though SQL isn't my native language!

Actually after some testing I think it's not right. SQL is not my native language either (or maybe it's my slick mapping). :( Still working through it, expect another iteration...

rtitle · 2017-06-15T13:20:31Z

@helgridly I think this is ready for another look, thanks

helgridly · 2017-06-15T14:47:56Z

core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala

+      }.groupBy { case (submissionId, workspaceId, _, _) =>
+        (submissionId, workspaceId)
+      }.map { case ((submissionId, workspaceId), recs) =>
+        (submissionId, workspaceId, recs.map(_._3).max, recs.map(_._4).max)


The third element is supposed to be a count, right? recs.length?

Actually I meant max. sum would work as well. This is here so the value will be >0 if the submission contains any failed workflows, and it will be 0 if it contains all successful workflows.

count would not work since it would just count all the rows, regardless of the workflow status (in SQL count(0) == count(1) == count(*) since 0 and 1 are both non-null values).

Yeah, I meant sum -- sorry. This jumped out at me because the outerSubmissionDateQuery line calls this value numFailures. If it's just a 1/0 then maybe just call exists here so the later line can be hasFailures.

I'm not sure .exists would work in this context -- I just changed it to .count to better reflect the variable name/type.

MatthewBemis · 2017-06-16T13:48:19Z

Could you explain the explain plan diagram? :p

Looking at it I see the words "full table scan" twice which makes me think maybe we're missing an index somewhere.

rtitle · 2017-06-16T14:22:06Z

Sure:

First, the diagram in the description is out of date since I changed the query - below is the current one.
The bottom "Full table scan" was just due to how I was testing it:

// causes full table scan since no index on workspace.name
... where s.workspace_id in (select id from workspace where name = 'my_test_workspace') ...

The below diagram is how it is executed in code:

// no full table scan
... where s.workspace_id in (:workspaceIds) ...

helgridly · 2017-06-16T17:35:42Z

core/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/WorkspaceComponent.scala

+    // using Option.orElse.
+
+    implicit val optionTimestampMonoid: Monoid[Option[Timestamp]] = MonoidK[Option].algebra[Timestamp]
+    CollectionUtils.groupTriples(workflowDates.toList)


you don't need the toList here any more, now you've swapped them to use Seq

helgridly · 2017-06-16T17:37:28Z

👍 pending two final nitpicks

rtitle added 5 commits June 9, 2017 14:43

GAWB-2056: WorkspaceSubmissionStats should reflect the status of the …

cbe29ff

…most recent submission, not the most recent workflow

Fix SQL query logic

21b62b0

Cleaned up comments a bit

fe4fcfa

Fix broken workspace api tests due to behavior change

3ee7afb

Update swagger doc

5144a52

rtitle commented Jun 9, 2017

View reviewed changes

Bah, fix query logic - should use having instead of where

7136d42

rtitle requested a review from helgridly June 12, 2017 14:44

helgridly reviewed Jun 12, 2017

View reviewed changes

rtitle added 2 commits June 13, 2017 11:11

Code review feedback

22d8f0b

Use more specific cats imports to help compilation time

ee31eb5

helgridly reviewed Jun 13, 2017

View reviewed changes

Prefer Seq to List

a5120c7

rtitle commented Jun 13, 2017

View reviewed changes

rtitle added 5 commits June 13, 2017 17:35

Fix SQL

746feaa

Add interleaved submission test case: it passes

f819423

Another attempt at fixing the query. Seems good, will do more testing.

2a7610e

minor: clearer variable names

7927a22

Fix unit test failure due to adding test workflows

103a4da

helgridly reviewed Jun 15, 2017

View reviewed changes

rtitle added 2 commits June 16, 2017 10:03

Change .max to .count

bc1aebf

Oops, meant .count

3553256

helgridly reviewed Jun 16, 2017

View reviewed changes

Final code review comments

bb205b3

Remove another unnecessary .toList

00df98b

rtitle merged commit c1d9779 into develop Jun 16, 2017

rtitle deleted the rt-gawb-2056 branch June 16, 2017 18:12

[GAWB-2056] The status of the last run is showing green when there are errors in them #709

[GAWB-2056] The status of the last run is showing green when there are errors in them #709

Conversation

rtitle commented Jun 9, 2017 • edited Loading

Problem:

Solution:

rtitle Jun 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Jun 13, 2017 • edited Loading

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

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

rtitle Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtitle Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

rtitle commented Jun 15, 2017

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

MatthewBemis commented Jun 16, 2017

rtitle commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helgridly commented Jun 16, 2017

rtitle commented Jun 9, 2017 •

edited

Loading

rtitle Jun 9, 2017 •

edited

Loading

rtitle Jun 12, 2017 •

edited

Loading

rtitle Jun 13, 2017 •

edited

Loading

rtitle Jun 13, 2017 •

edited

Loading

rtitle Jun 13, 2017 •

edited

Loading

rtitle Jun 13, 2017 •

edited

Loading