Add Parallel type class by LukaJCB · Pull Request #1837 · typelevel/cats

LukaJCB · 2017-08-21T17:37:42Z

Should resolve #1830. Only Syntax and type class for now, but more to follow :)

codecov-io · 2017-08-21T18:26:31Z

Codecov Report

Merging #1837 into master will increase coverage by 0.01%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master    #1837      +/-   ##
==========================================
+ Coverage   96.21%   96.22%   +0.01%     
==========================================
  Files         272      279       +7     
  Lines        4628     4748     +120     
  Branches      124      115       -9     
==========================================
+ Hits         4453     4569     +116     
- Misses        175      179       +4

Impacted Files	Coverage Δ
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`92.18% <ø> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/data/OneAnd.scala	`98.36% <ø> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyList.scala	`100% <ø> (ø)`	⬆️
laws/src/main/scala/cats/laws/ParallelLaws.scala	`100% <100%> (ø)`
core/src/main/scala/cats/data/WriterT.scala	`94.62% <100%> (+0.43%)`	⬆️
core/src/main/scala/cats/syntax/parallel.scala	`100% <100%> (ø)`
core/src/main/scala/cats/data/Kleisli.scala	`100% <100%> (ø)`	⬆️
...ain/scala/cats/laws/discipline/ParallelTests.scala	`100% <100%> (ø)`
core/src/main/scala/cats/instances/parallel.scala	`100% <100%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b66b121...0c87a1b. Read the comment docs.

LukaJCB · 2017-08-21T19:58:51Z

+    Parallel.parTraverse(self)(f)
+
+  def parSequence[M[_]: Monad, F[_], B](implicit ev: A <:< M[B], P: Parallel[M, F]): M[T[B]] =
+    Parallel.parSequence(self.asInstanceOf[T[M[B]]])


Anyone know a better solution? Feels a little bit dirty.

LukaJCB · 2017-08-25T10:29:04Z

Summary of what this entails:

Instances for:

Semigroup[E] => Either[E, ?] <-> Validated[E,?]
(Semigroup[E], Monad[M]) => EitherT[M, E, ?] <-> Nested[M, Validated[E,?]]
Parallel[M, F] => OptionT[M, ?] <-> Nested[F, Option, ?]
Parallel[M, F] => WriterT[M, A, ?] <-> WriterT[F, A, ?]
Parallel[M, F] => Kleisli[M, A, ?] <-> Kleisli[F, A, ?]
(Parallel[M, F], Semigroup[E]) => EitherT[M, E, ?] <-> Nested[F, Validated[E, ?]
NonEmptyList[A] <-> ZipNonEmptyList[A]
NonEmptyVector[A] <-> ZipNonEmptyVector[A]
Vector[A] <-> ZipVector[A]
Stream[A] <-> ZipStream[A]
List[A] <-> ZipList[A]
(Parallel[M, F], Alternative[M], Alternative[F]) => OneAnd[M, ?] <-> OneAnd[F, ?] (this is really just for NonEmptyStream)
Future[A] <-> FailFastFuture[A]

Syntax for:

parTraverse
parSequence
Tuple syntax parMapN

Other notes:

We could also add an instance for List with an Applicative instance that zips, there currently is a newtype ZipList in newts, so either it would have to be defined there, or we would have to pull it inside cats.

edmundnoble · 2017-08-26T18:31:04Z

I will also transport cats-mtl type classes across parallel.

In cats, you should also have (Parallel[M,F], MonadError[M]) => ApplicativeError[F].

LukaJCB · 2017-08-26T22:40:25Z

@edmundnoble I've added the instance for it, but now I'm getting failing Serializable Tests, any pointers there? 🤔

It seems, that the instance forApplicativeError[WriterT[Validated[String, ?], ListWrapper[Int], ?], String] is actually the derived one from Parallel[Either[String, ?], Validated[String, ?]] instead of the one given by WriterT.
I don't understand why this doesn't throw an "ambiguous implicits" error.
I'm not sure how implicit resolution for these types of derived instances work, when there's another instance in scope. If anyone had some reading material for this kind of thing, I'd greatly appreciate it.

Otherwise, I'd just delete the ApplicativeError instance, and include it in a follow up PR, this one is already getting very large.

LukaJCB · 2017-08-27T12:23:44Z

Another open question is about an instance for Future. Currently the standard Applicative instance already executes in parallel, which is certainly nice for a lot of people, but also inconsistent with it's Monad instance. I'm not sure how strict we are with inconsistent Monad/Applicative instances, but changing the current Applicative instance would be a massive breaking change, so I'm thinking we should not do it anytime soon and thus a Parallel instance doesn't really make all that much sense right now. Opinions? :)

kailuowang · 2017-08-28T17:14:08Z

+  def applicative: Applicative[F]
+
+  /**
+    * Natural Transformation from the sequential Monad M[_] to the parallel Applicative F[_].


the other way? from the parallel Applicative F[_] to the sequential Monad M[_]

kailuowang · 2017-08-28T17:14:59Z

+  def sequential(implicit M: Monad[M]): F ~> M
+
+  /**
+    * Natural Transformation from the parallel Applicative F[_] to the sequential Monad M[_].


also in reverse order

kailuowang · 2017-08-28T17:21:20Z

+  * Some types that form a Monad, are also capable of forming an Applicative that supports parallel composition.
+  * The Parallel type class allows us to represent this relationship.
+  */
+trait Parallel[M[_], F[_]] {


you want extends Serializable here

LukaJCB · 2017-08-28T19:44:11Z

Thank you @kailuowang! That fixed the failing tests. :)

kailuowang · 2017-08-28T20:34:12Z

My opinion over Future is that we shouldn't try to provide more functionalities over it than we currently have. I think users who are already coding at this level of pure FP probably abandoned Future long ago, especially now that we have so many good options for replacement in the cats ecosystem.

LukaJCB · 2017-08-28T21:35:39Z

Totally agree with you @kailuowang.

CI does not pass the Validated tut tests right now because of ambiguous implicits as I feared before. I 'm not sure why it doesn't fail in the same way in the WriterT and ValidatedTests, but right now importing from cats.implicits._ results in ambiguous implicits for ApplicativeError for `Validated.

We can delete the either derived ApplicativeError instance or the specific one for Validated, or somehow find a way to give the specific instance higher priority. Although I don't really know how we could right now.

edmundnoble · 2017-08-29T05:41:11Z

The Applicative instance for Future is actually not responsible for the evaluation strategy, seeing as all Futures are already evaluated. That is to say, even if you changed Future's Applicative operations to use flatMap, Applicative is not lazy in its first or second parameter, so it's not even possible to make a sequential Applicative. This is fine if you make the frequently-made assumption that if you're using cats with Future you aren't using side effects, because with Future that's exactly what they are, side effects (not suspendable into Future), and cats doesn't morally care about side effects like we don't care about null.

kailuowang · 2017-08-29T13:57:03Z

@LukaJCB to avoid the ambiguous implicits, an option that I can think of is to not implicitly provide the ApplicativeX instances through Parallel (which I am not sure about the use cases are anyway), rather, make them available through explicit methods on Parallel. Users will need to explicitly use them if they need parallel execution.

LukaJCB · 2017-08-29T13:58:51Z

That's an excellent idea, will do! :)

LukaJCB · 2017-08-30T11:06:29Z

CI is happy again, I'd be fine with merging this without further functionality and deal with e.g. a List/ZipList instance afterwards :)

LukaJCB · 2017-09-02T10:49:52Z

Now that #1885 is merged, I went ahead and added instances for NEL and NEV :)

kailuowang · 2017-10-13T12:56:38Z

+      val n = if (arity == 1) { "" } else { arity.toString }
+
+      val parMap =
+        if (arity == 1) s"def parMap[F[_], Z](f: (${`A..N`}) => Z)(implicit p: NonEmptyParallel[M, F]): M[Z] = p.flatMap.map($tupleArgs)(f)"


is the arity=1 version tested?

kailuowang · 2017-10-13T12:57:05Z

+      block"""
+         |package cats
+         |trait ParallelArityFunctions {
+        -  def parMap$arity[M[_], F[_], ${`A..N`}, Z]($fparams)(f: (${`A..N`}) => Z)(implicit p: NonEmptyParallel[M, F]): M[Z] =


are these parMap1~22 tested?

They're not. Related to #1968

kailuowang · 2017-10-13T12:58:42Z

 package tests

-import cats.data.NonEmptyList
+import cats.data.{NonEmptyList}


nit, no need for the braces here.

johnynek

Looks great. One suggestion: I’d like the Parallel to require that the functors are isomorphic, not just the pure.

johnynek · 2017-10-14T03:40:46Z

+  */
+trait NonEmptyParallelLaws[M[_], F[_]] {
+  def P: NonEmptyParallel[M, F]
+


Should we say that the Functors should be the same. If not it seems pretty hard to reason about how they are related.

Yes that's a good idea, I'll add a law!

kailuowang

This is really cool. LGTM

SystemFw · 2017-10-23T19:26:33Z

Just want to reiterate that this is awesome, great work @LukaJCB

johnynek

Sorry for the review latency!

This looks really great.

alexandru · 2017-10-27T07:17:50Z

Just wanted to drop a line to say that I'm really glad to see this PR merged 🤗

johnynek · 2017-10-28T18:51:26Z

This with the opaque types SIP will be great.

LukaJCB force-pushed the add-parallel-class branch from 82b6ae7 to 714b84d Compare August 21, 2017 18:50

LukaJCB commented Aug 21, 2017

View reviewed changes

LukaJCB force-pushed the add-parallel-class branch from ef6c75c to c5fc3ee Compare August 21, 2017 20:37

LukaJCB changed the title ~~Add Parallel type class (WIP)~~ Add Parallel type class Aug 24, 2017

LukaJCB mentioned this pull request Aug 25, 2017

Add a ValidatedT to give ApplicativeError to any Applicative. #1818

Closed

LukaJCB force-pushed the add-parallel-class branch from b6d0a80 to 1836c07 Compare August 26, 2017 22:06

kailuowang reviewed Aug 28, 2017

View reviewed changes

LukaJCB force-pushed the add-parallel-class branch from 7aa9384 to ab229b9 Compare August 29, 2017 14:57

johnynek mentioned this pull request Aug 29, 2017

Add zipWith to NonEmptyList #1870

Closed

kailuowang added the enhancement label Aug 29, 2017

LukaJCB force-pushed the add-parallel-class branch from 52154c9 to b311ab9 Compare September 2, 2017 10:46

LukaJCB force-pushed the add-parallel-class branch from b311ab9 to 9493771 Compare September 2, 2017 11:29

Luka Jacobowitz added 3 commits September 3, 2017 01:55

Initial version of Parallel

eaf34e6

Add Either/Validated Parallel instance

21942c5

Break up syntax for parallel

b37732b

kailuowang self-assigned this Oct 11, 2017

kailuowang reviewed Oct 13, 2017

View reviewed changes

kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017

LukaJCB force-pushed the add-parallel-class branch from 71a1239 to 0276415 Compare October 14, 2017 00:15

Add parFlatTraverse and sequence

4151e7e

LukaJCB force-pushed the add-parallel-class branch from 0276415 to 4151e7e Compare October 14, 2017 00:17

Merge branch 'master' into add-parallel-class

87f7b87

johnynek reviewed Oct 14, 2017

View reviewed changes

Luka Jacobowitz added 2 commits October 14, 2017 10:50

Add isomorphic functor law

4a9f2ad

Fix law test parameters

7a6cd52

kailuowang previously approved these changes Oct 16, 2017

View reviewed changes

Merge branch 'master' into add-parallel-class

cca85ea

kailuowang mentioned this pull request Oct 16, 2017

Getting ready for RC1 #1974

Merged

12 tasks

Merge branch 'master' into add-parallel-class

4bbd46e

LukaJCB dismissed kailuowang’s stale review via 4bbd46e October 19, 2017 15:44

LukaJCB force-pushed the add-parallel-class branch from 8dcfc4a to 4bbd46e Compare October 19, 2017 15:44

Merge branch 'master' into add-parallel-class

0c87a1b

kailuowang approved these changes Oct 23, 2017

View reviewed changes

kailuowang mentioned this pull request Oct 26, 2017

[Meta] PR sign-off policy #1994

Closed

johnynek approved these changes Oct 26, 2017

View reviewed changes

kailuowang merged commit 58a9fd6 into typelevel:master Oct 26, 2017

LukaJCB deleted the add-parallel-class branch October 27, 2017 08:56

johnynek mentioned this pull request Nov 3, 2017

Kleisli#andThen unavailable for functor Validated[X, ?] #2013

Closed

SystemFw mentioned this pull request Nov 6, 2017

Integration with cats Parallel typelevel/fs2#969

Closed

kailuowang mentioned this pull request Jun 11, 2018

Use an abstract type member for the parallel type of the Parallel type class #2233

Closed

Uh oh!

Uh oh!

Conversation

LukaJCB commented Aug 21, 2017

Uh oh!

codecov-io commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukaJCB commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of what this entails:

Instances for:

Syntax for:

Other notes:

Uh oh!

edmundnoble commented Aug 26, 2017

Uh oh!

LukaJCB commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukaJCB commented Aug 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukaJCB commented Aug 28, 2017

Uh oh!

kailuowang commented Aug 28, 2017

Uh oh!

LukaJCB commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edmundnoble commented Aug 29, 2017

Uh oh!

kailuowang commented Aug 29, 2017

Uh oh!

LukaJCB commented Aug 29, 2017

Uh oh!

LukaJCB commented Aug 30, 2017

Uh oh!

LukaJCB commented Sep 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnynek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang left a comment

Choose a reason for hiding this comment

Uh oh!

SystemFw commented Oct 23, 2017

Uh oh!

johnynek left a comment

Choose a reason for hiding this comment

Uh oh!

alexandru commented Oct 27, 2017

Uh oh!

johnynek commented Oct 28, 2017

Uh oh!

Reviewers

Assignees

Labels

codecov-io commented Aug 21, 2017 •

edited

Loading

LukaJCB commented Aug 25, 2017 •

edited

Loading

LukaJCB commented Aug 26, 2017 •

edited

Loading

kailuowang Aug 28, 2017 •

edited

Loading

LukaJCB commented Aug 28, 2017 •

edited

Loading