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

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

Closed
LukaJCB opened this issue Apr 17, 2018 · 14 comments · Fixed by #3012
Closed

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

LukaJCB opened this issue Apr 17, 2018 · 14 comments · Fixed by #3012

Comments

@LukaJCB
Copy link
Member

LukaJCB commented Apr 17, 2018

Right now we're using two type parameters which essentially means bidirectional functional dependencies. However all of our functions making use of parallel are uniquely determined by F, meaning we can make G an abstract type parameter which would get rid of the some of boilerplate when writing abstract code over Parallel. This is a breaking change, so would need to go to cats 2.0. :)

@LukaJCB LukaJCB added this to the 2.0 milestone Apr 18, 2018
@ChristopherDavenport
Copy link
Member

I've created a temporary lib to deliver this feature until we can get this for 2.0.

Let me know if anything would like to be tweaked or to initiate a PR upstream to cats. Everything is working for me so I'm inclined to release a 0.1.0

https://github.com/ChristopherDavenport/cats-par

@kailuowang
Copy link
Contributor

Not trying to be annoying but I'd like to point out that I did mention the risk of releasing something in as stable before it's battle tested. #1837 (comment)
If we chose to roll this one out in a more gradual way ( e.g. released in an experiment module first) we might avoided this problem. Of course, there would be other trade-offs.

@johnynek
Copy link
Contributor

johnynek commented Oct 2, 2018

Another idea here is a Sequential[A[_]] type class which would be the dual of a Parallel[M[_]]

So, if you have an applicative, but you want to go sequential for a bit (I am using Validated, but here I need andThen) you take the Sequential[ValidatedNec[String, ?]] and then it can allow you to work with Either for bit and convert back to ValidatedNec at the end.

@oscar-stripe
Copy link
Contributor

one work around I did internally was just make Parallel1 very similar to @ChristopherDavenport 's example.

We could do that in cats core. 1 meaning 1 type parameter...

It's a bit of an ugly name but it makes it much more convenient to use and we don't need to wait until cats 3.0.

I think we have to accept warts if we want binary compatibility (which I do).

@LukaJCB
Copy link
Member Author

LukaJCB commented Jan 17, 2019

So just duplicate it all into Parallel1 for now, and then at some point deprecate Parallel? Might be a good solution

@johnynek
Copy link
Contributor

Well my solution was to resolve Parallel using Parallel1 so Parallel1 is a wrapper for Parallel only used to ease implicit resolution.

Otherwise existing instances of Parallel wouldn’t be usable.

@johnynek
Copy link
Contributor

Here is what I added:

import cats.Parallel

sealed trait Parallel1[F[_]] extends Serializable {
  type A[_] // applicative type
  implicit def parallel: Parallel[F, A]
}

object Parallel1 {
  type Aux[F[_], A0[_]] = Parallel1[F] { type A[x] = A0[x] }

  implicit def parallel1Instance[F[_], A0[_]](implicit p: Parallel[F, A0]): Parallel1.Aux[F, A0] =
    new Parallel1[F] {
      type A[x] = A0[x]
      val parallel = p
    }

  implicit def parallelFromParallel1[F[_]](implicit p: Parallel1[F]): Parallel[F, p.A] =
    p.parallel
}

Then you can use it with:

import Parallel1.parallelFromParallel1

def sequence[F: Parallel1, A](as: List[F[A]]): F[List[A]] =
  Parallel.parSequence(as)

@johnynek
Copy link
Contributor

We could probably also make parallelFromParallel1 a low priority on the Parallel object and not even have the import.

@ChristopherDavenport
Copy link
Member

Why not use Par and then existing users on cats-par can just migrate via changing imports?

@ChristopherDavenport
Copy link
Member

It sounds a lot like you have spent time to recreate the work that has been built out incrementally in cats-par.

@johnynek
Copy link
Contributor

No offense Chris, but this is 27 lines of code in total. It was less work that updating our builds to depend on a third party we would have to keep up to date with changes to cats/etc...

And as you note in your readme, I was the one that suggested this approach originally.

@ChristopherDavenport
Copy link
Member

ChristopherDavenport commented Jan 17, 2019

I totally was suggesting bringing it in. Not depending on the library.

Edit: I see that was about your company approach rather than cats. What I was suggesting is that if we are going to bring in a slightly altered name into cats, that doing so in a way that allows those on the third-party lib to transition would be a smooth way to upgrade.

@kailuowang
Copy link
Contributor

kailuowang commented Jan 18, 2019

@ChristopherDavenport a mere rename is how I read your intention as well.
I think I like Parallel1 siightly better, because it looks supplementary to Parallel, while it's harder to tell the difference between Par and Parallel. For cats-par users, they can do a replace
import cats.temp.par._ to import cats.{Parallel1 => Par}, not much extra cost to them.
a global replacement of Par to Parallel1 shouldn't be terrible either.

Update: My replace example might be off, but you get the idea, a replacement to add a name alias shouldn't be too much more costly than removing the import right?

@kailuowang
Copy link
Contributor

@LukaJCB so how about we create a new issue with @johnynek 's solution and label it with "help wanted"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants