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

Added parFlatMapN #4243

Merged
merged 9 commits into from
Oct 7, 2022
Merged

Added parFlatMapN #4243

merged 9 commits into from
Oct 7, 2022

Conversation

TonioGela
Copy link
Member

Hello 👋!

Taking inspiration from #4009 I added flatParMapN generation in Boilerplate. Let me know if it works for you or if something needs to be changed.

@armanbilge armanbilge changed the title Added flatParMapN Added flatParMapN Jun 17, 2022
@satorg
Copy link
Contributor

satorg commented Jun 17, 2022

To me it seems that the name should be parFlatMapX rather than flatParMapX...

@TonioGela
Copy link
Member Author

To me it seems that the name should be parFlatMapX rather than flatParMapX...

After a discussion with @armanbilge, I decided that parFlatMapN could have been confusing since it's not flatMap related but it's a mere mapN(...).flatten. It's equivalent to a flatMap just in the 1-arity case.

@armanbilge
Copy link
Member

armanbilge commented Jun 17, 2022

Right, I initially proposed parFlatMapN but I realized it doesn't make sense.

The whole concept of Parallel is about a relationship between a Monad and its "parallel" Applicative (FlatMap and its parallel Apply). That is to say, there is no "parallel" flatMap and thus a parFlatMap is non-sensical IMO.

However, if you imagine flatMap is named for "flattening a map", then in this case we are "flattening a parMap".

tests/shared/src/test/scala/cats/tests/SyntaxSuite.scala Outdated Show resolved Hide resolved
Comment on lines 351 to 354
- p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) }
-
- def flatParMap$arity[M[_], ${`A..N`}, Z]($fparams)(f: (${`A..N`}) => M[Z])(implicit p: NonEmptyParallel[M]): M[Z] =
- p.flatMap.flatten(p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) })
Copy link
Member

@armanbilge armanbilge Jun 17, 2022

Choose a reason for hiding this comment

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

Not directly about this PR, but I'm confused by the code here (also for the existing method above it). Why is it calling p.flatMap.map instead of p.apply.map? Since the Apply is the parallel instance.

Am I confused or is this a bug? Maybe we need some tests using e.g. EitherNec[String, A]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes agree, good catch... So now I wonder too.

Copy link
Member

@armanbilge armanbilge Jun 18, 2022

Choose a reason for hiding this comment

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

Ahh, no this is completely fine. It's because it's calling vanilla map on a single argument (a tuple), which is then unpacked by the case in the lambda. I was mistakenly reading it as calling a mapN-like method, in which case this would be wrong.

The map must be consistent between the flatMap and the apply by law, and using the apply instance would require a conversion to the parallel datatype and back. So it's an optimization to use the one from the flatMap instead.

Copy link
Member

Choose a reason for hiding this comment

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

Actually ... this is just flatMap ... right? 😂

Suggested change
- p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) }
-
- def flatParMap$arity[M[_], ${`A..N`}, Z]($fparams)(f: (${`A..N`}) => M[Z])(implicit p: NonEmptyParallel[M]): M[Z] =
- p.flatMap.flatten(p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) })
- p.flatMap.map(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) }
-
- def flatParMap$arity[M[_], ${`A..N`}, Z]($fparams)(f: (${`A..N`}) => M[Z])(implicit p: NonEmptyParallel[M]): M[Z] =
- p.flatMap.flatMap(${nestedExpansion.products}) { case ${nestedExpansion.`(a..n)`} => f(${`a..n`}) }

Copy link
Member Author

Choose a reason for hiding this comment

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

It is! I run the tests locally just to be sure that I was not missing something syntax related :)

@satorg
Copy link
Contributor

satorg commented Jun 18, 2022

On the second thought... If flatParMapN is just parMapN followed by flatten, what is the advantage we can get by adding it? I mean if we could optimize it somehow even a little bit by avoiding creating interim object... But seems that we just cannot do anything like that and the proposed implementation does exactly the same thing as parMap + flatten.

In other words, having

def foo: F[Foo]
def bar: F[Bar]
def woo: F[Woo]
def baz: F[Baz]

def combineThemAll: (Foo, Bar, Woo, Baz) => F[Result]

then instead of

(foo, bar, woo, baz).parMap(combineThemAll).flatten

we can get

(foo, bar, woo, baz).flatParMap(combineThemAll)

But it comes at price of adding 22+22 new methods.

@armanbilge
Copy link
Member

On the second thought... If flatParMapN is just parMapN followed by flatten, what is the advantage we can get by adding it?

I see your point, but how is this any different than #4009?

armanbilge
armanbilge previously approved these changes Jun 18, 2022
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

💯 Although it would be good to address #4243 (comment) :)

Thanks for picking this up!

@satorg
Copy link
Contributor

satorg commented Jun 18, 2022

The difference is not that huge though, but:

  • Added flatMapN #4009 introduces FlatMapArityFunctions which is mixed into FlatMap which is a typeclass and thus can be potentially optimized for some implementations.
  • in contrast to that this PR adds new methods to:
    • ParallelArityFunctions which is mixed into object Parallel as well as
    • final class TupleNParallelOps which are, well, final classes.

Therefore there no way the flatParMap stuff can be optimized in any case.

That said, I am not strictly against this addition... Just to double check that we understand all the pros and cons we're going to get.

@armanbilge
Copy link
Member

  • ParallelArityFunctions which is mixed into object Parallel as well as

Ah yes, that's a good point. I only just realized this myself but the implication didn't register.

I was curious why they are defined on the object instead of the trait itself.

@TonioGela
Copy link
Member Author

I was curious why they are defined on the object instead of the trait itself.

I am curious too, but I can imagine that one of the reasons might have been to have a consistent behaviour between the parMap2..22 functions. Having ParallelArityFunctions mixed with the trait would have let parMap2 and parMap3 have completely different implementations (within the limits of the signature of course).

That said, this might be a reason to not want a FlatParMapArityFunctions trait to be mixed with Parallel, but I'm not sure I'm able to foresee all the implications.

What do you think @satorg?

@johnynek
Copy link
Contributor

To bikeshed this a bit, I think parFlatMap might be a better name: the par part of it happens, then we flatMap.

That or maybe parMapFlatten, which at least reads like what is happening: first we parMap then we flatten.

@TonioGela
Copy link
Member Author

That or maybe parMapFlatten, which at least reads like what is happening: first we parMap then we flatten.

To keep up with the bikeshedding: I love parMapFlatten in the 1-arity case but both parMapFlattenN and parMapNFlatten (or parMapFlatten2, parMap2Flatten and siblings) are quite unreadable IMO.

@armanbilge armanbilge added this to the 2.9.0 milestone Jun 19, 2022
@TonioGela TonioGela requested a review from satorg June 20, 2022 11:35
armanbilge
armanbilge previously approved these changes Jun 21, 2022
@satorg
Copy link
Contributor

satorg commented Jun 21, 2022

I would also like to add one more point on par*-like name. To me it looks like an established convention – if a method name starts with parFooBar then we could assume that it tries to do the same thing as just fooBar but via the Parallel (or NonEmptyParallel) instance. So it was pretty clear and straightforward assumption: map <-> parMap, tupled <-> parTupled, traverse <-> parTraverse and so it.

Now the proposed name flatParMapN breaks this convention and thus it makes its discoverability quite impeded. So I would still vote for making it parFlatMapN because semantically it is the same as flatMapN but just via Parallel.

I bet that most users who want to turn their parMapN(...).tupled expression into a single call will be looking for parFlatMapN name first because they could be aware that the same transition from mapN(...).tupled will be flatMapN.

@armanbilge
Copy link
Member

armanbilge commented Jun 21, 2022

So I would still vote for making it parFlatMapN because semantically it is the same as flatMapN but just via Parallel.

This is the part that I really struggle with. As we know Parallel is a relationship between a Monad and an Applicative.

My intuition for a "par" method is that it delegates to the "parallel" Applicative implementation of that method. For example:

  • parMapN <-> parallel.mapN
  • parProduct <-> parallel.product
  • parAp <-> parallel.ap
  • parReplicateA <-> parallel.replicateA
  • ...
  • parFlatMap <-> parallel.flatMap 🤯 no such thing!

@TonioGela
Copy link
Member Author

Both arguments are valid ones

I bet that most users who want to turn their parMapN(...).tupled expression into a single call will be looking for parFlatMapN name first because they could be aware that the same transition from mapN(...).tupled will be flatMapN.

I would love thea coherence you're describing here, but in the first place I have to admit that once I read in the changelog about the addition of flatMapN I needed a minute or 2 to realize that it was a mere shortcut for mapN(...).flatten.

At first, I began searching for the implementation to check whether it was implemented like (M[A], M[B]) => M[A].flatMap(a => M[B].flatMap(...)) (so sequentially) and it was there that I had the idea to add a parMapN(...).flatten alias.

From a certain perspective, this episode might demonstrate that having flatMapN and parFlatMapN side by side would be less confusing, in particular given the fact that a method requires NonEmptyParallel[M] while the other a FlatMap[F].

parFlatMap <-> parallel.flatMap 🤯 no such thing!

The problem IMHO arises here, in the 1-arity case. I suppose that the first impression of a lot of people in front of a parFlatMap would be a "WTH? I just understood that monads are a great tool to encode sequential computation, how can a parallel flatMap exist?"

That said, I would like to gather further opinions, we are just 4 people discussing it here and maybe someone else might have an enlightening idea on the right name to pick. What do you think?

Copy link
Member

@danicheg danicheg left a comment

Choose a reason for hiding this comment

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

Hey there! In Mouse, we got a request to add parFlatMapN, but already there is this PR in cats. Can I get TLDR about this work? Personally, I like parMapN and countless times did the _.parMapN(...).flatten things.

UPD: 👍 from me to parFlatMapN name. I love it a bit more.

@TobiasRoland
Copy link

TobiasRoland commented Sep 13, 2022

👋 hi, person-who-raised-the-issue-against-mouse here, I never even considered googling for flatParMap - it's down to taste I guess, but to me parFlatMapN makes most sense (also, if IDE support becomes good, I can go .par... and my IDE can suggest all the parallel methods which would be nice)

@armanbilge
Copy link
Member

Thanks both for chiming in. It seems everyone likes parFlatMapN ... but I still disagree. Particularly about it being a matter of taste, rather it is simply wrong and not following Cats conventions. There is no Parallel version of .flatMap.

Can anyone at least respond specifically to my point in #4243 (comment)? It's okay if the answer is "I don't care because I want IDE completion" 😂

@danicheg
Copy link
Member

Can anyone at least respond specifically to my point in #4243 (comment)?

Alright, you asked for it! 😆
I think you described rather implementation details than the actual naming convention/contract of API:

  • parMapN <-> parallel.mapN
  • parProduct <-> parallel.product
  • parAp <-> parallel.ap
  • parReplicateA <-> parallel.replicateA
  • ...
  • parFlatMap <-> parallel.flatMap 🤯 no such thing!

What's more important for user – one will get the parallel (concurrent actually) behavior of execution multiple effects using parFlatMapN <-> parMapN(...).flatten. Prefix par here is for highlighting that behavior, as for me.
It's also pretty obvious – executing the underlying effect will go after executing the batch of effects. It comes from the contract straightforwardly.
And I agree with @TobiasRoland that better support of IDE is a killer feature.

@armanbilge
Copy link
Member

armanbilge commented Sep 13, 2022

I think you described rather implementation details than the actual naming convention/contract of API:

Ha, okay, fair point. I guess it's just a coincidence that all the existing implementation follow this pattern so far 😉 I do think it would be a good convention to have.

Personally I also think that:

map(...).flatten <-> flatMap(...)

screams that:

parMapN(...).flatten <-> flatParMapN(...)

Because this seemed like a convention too. But maybe it's just an implementation detail all along :)


It's also pretty obvious – executing the underlying effect will go after executing the batch of effects. It comes from the contract straightforwardly.

I thought about this for a while and I agree. I think it's pretty easy to see what parFlatMapN does based on an understanding of par and its method signature. And it is certainly better for IDE completion, but I'm not convinced this is a good argument to use for Cats.

I would prefer we establish a convention here based on the existing implementations and merge this PR as-is, but I won't stand in the way.

Thanks for the response :)

johnynek
johnynek previously approved these changes Sep 27, 2022
Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

minor comment about simplifying by avoiding Kleisli in a test.

test("ParMapN over f should be consistent with parFlatMapN over f lifted in Kleisli") {
forAll { (as: List[Int], bs: List[Int]) =>
val f: (Int, Int) => Int = _ + _
val mf: (Int, Int) => List[Int] = Function.untupled(Kleisli.fromFunction[List, (Int, Int)](f.tupled).run)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what Kleisli is doing here. Couldn't we just do: val mf: (Int, Int) => List[Int] = { (a, b) => List(a + b) }?

Copy link
Member Author

Choose a reason for hiding this comment

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

not very much, my intention was to show that _.parMapN(f) is equivalent to _.parFlatMapN(mf) where mf is f: A => B lifted in a Kleisli[M, A, B] where Parallel[M] exists. It turned out bad since I had to tuple/untuple unluckily (Kleisli.fromFunction(f).run is way more readable). For the sake of readability I'll use a cleaner impl of that function, gimme sec :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Just pushed this

val f: (Int, Int) => Int = (a, b) => a + b
val mf: (Int, Int) => List[Int] = (a, b) => List(a + b)

@TonioGela TonioGela requested review from johnynek and satorg and removed request for johnynek September 30, 2022 06:57
@TonioGela TonioGela requested a review from satorg October 6, 2022 06:59
Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

made a comment, take or leave it.

@TonioGela
Copy link
Member Author

TonioGela commented Oct 7, 2022

made a comment, take or leave it.

Answered in the single discussions, thanks for the review

Copy link
Member

@danicheg danicheg left a comment

Choose a reason for hiding this comment

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

Thank you for your efforts in putting this to the finish. Great work!

@TonioGela
Copy link
Member Author

TonioGela commented Oct 7, 2022

Thank you for your efforts in putting this to the finish. Great work!

image

@danicheg danicheg merged commit aadce06 into typelevel:main Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants