Open purrgrammer opened 5 years ago
I think with Concurrent
we should be able to implement batch
in parallel as well. The code won't end up looking as nice as with Par
, but that is probably a price worth paying if we can loose the Par
constraint in a lot of places.
Would it be possible to make the restriction to Effect
and define a couple of implementations if there is a ConcurrentEffect
instance?
2 cents from the peanut gallery for what it's worth...
From a users perspective moving from earlier versions of Fetch, it was very surprising to me to have to sprinkle implicit Timer
and ContextShift
magic into my otherwise Monix.Task
-based DAOs - e.g.:
def fetchString[F[_] : ConcurrentEffect](n: Int): Fetch[F, String] =
Fetch(n, ToStringSource)
//Why do I need to do all of this in order to interpret a Fetch into an IO?
//I understand *why* but it's counter-intuitive given that when we
//run the Fetch to produce the IO we still haven't generated any side-effects.
val ec: ExecutionContext = implicitly[ExecutionContext]
implicit val timer: Timer[IO] = IO.timer(ec)
implicit val cs: ContextShift[IO] = IO.contextShift(ec)
val io: IO[String] = Fetch.run[IO](fetchString(123))
val task: Task[String] = Task.fromEffect(io)
So if I've got code like this:
trait KVStore{
def get(key: String): Task[String]
}
within which I'm trying to use Fetch
interpreted into a Task
, I now need to either utilize some sort of constructor injection, trait mixing, or implicit currying to get the desired ExecutionContext
(which I don't think was previously required, right?) down to where it needs to go - e.g.:
trait KVStore{
def get(key: String)(implicit ec: ExecutionContext): Task[String]
}
I'm not sure of the details of Par
vs. ConcurrentEffect
, but if removing the dependency on Par
would eliminate the need for the additional implicit Timer
and ContextShift
evidence in scope in cases like this one I'm all for it.
Thinking about this more... would it be possible to interpret a Fetch
to a Reader[(Timer[IO], ContextShift[IO]), IO]
so that the requisite Timer
and ContextShift
could be injected at the "end of the world" when the program is run?
I'm not sure of the details of Par vs. ConcurrentEffect, but if removing the dependency on Par would eliminate the need for the additional implicit Timer and ContextShift evidence in scope in cases like this one I'm all for it.
We use Par
for leveraging parallel traverse operations and don't have to implement it ourselves, it has been mentioned that we could implement those parallel operations without requiring Par
so I'm all for removing it. Also, it introduces another third-party dependency that is not officially supported by typelevel/cats.
Thinking about this more... would it be possible to interpret a Fetch to a Reader[(Timer[IO], ContextShift[IO]), IO] so that the requisite Timer and ContextShift could be injected at the "end of the world" when the program is run?
About Timer
and ContextShift
: not sure if we can remove them when running a Fetch
into an IO
since the latter needs evidence of them existing. I could be wrong but I don't think we can not remove those, I'll spend some time trying though. Not sure the solution to inject those running a Fetch
into a Reader
could work since we need implicit evidence.
Thanks for your feedback, I'll get back to this issue when I make some progress. If you have any ideas/proofs-of-concept for making implicits less restrictive I'm happy to review and merge PRs too.
I think with
Concurrent
we should be able to implementbatch
in parallel as well. The code won't end up looking as nice as withPar
, but that is probably a price worth paying if we can loose thePar
constraint in a lot of places.
I think Par
is worth having to avoid having to duplicate its implementation for types that have Concurrent. And I'm afraid a parMap2
(or similar) written with IO's Concurrent, for example, might need substantial work to be as bug-free as the parMap2
that comes with IO's Parallel. I doubt users would mind F[_]: Par: Concurrent
.
it introduces another third-party dependency that is not officially supported by typelevel/cats.
It's a very small library that's actually endorsed by cats maintainers until cats breaks binary compatibility (see gitter channel). I wouldn't consider this a major obstacle :)
I'm all +1 for getting rid of any Effect constraints. They should be used as rarely as possible.
I think Par is worth having to avoid having to duplicate its implementation for types that have Concurrent.
I think I disagree know with what I said almost 6 months ago as well, and I agree that we should look if we can't get by with just Concurrent
(instead of ConcurrentEffect
).
Would you feel any different about Par if cats-par was officially endorsed by typelevel?
Here are all the reasons I found not to require Concurrent:
parTraverse
for which Parallel/Par is enoughdelay
, async
, cancelable
and friends; which is the main reason why it's usually a good idea to use it sparingly and hide the details like calls to race
behind algebras that'll be implemented with Concurrentrun
part (in which MonadError[F, Throwable]
would probably suffice)Id
or Either
and not a full blown IO
that may or may not consist of asynchronous blocksFYI ContextShift is completely unused in the code for fetch, too, so the only thing that remains is Clock, usage of which could be superseded by a custom algebra for measuring time (allowing custom implementations that'd simply return a dumb value for tests).
Would you feel any different about Par if cats-par was officially endorsed by typelevel?
We removed it because we only wanted to depend on cats-effect, introducing Par
from cats-par felt odd, but is something we can reconsider. Taking a look at #184, thanks for the PR kubukoz!
Currently the methods that fetch data in
DataSource
require an effect typeF
withPar[F]
andConcurrentEffect[F]
.Par
is required due to theDataSource#batch
implementation defaulting to running individual fetches in parallel. If we move away from it, Data sources that don't implement batching will run their fetches sequentially.Par[F]
will still be required when creating or running a fetch toF
, since is used when running multiple batches in parallel.ConcurrentEffect
is perhaps too restrictive, since we may not need the cancellation semantics thatConcurrent
provides. We could useEffect
here, thoughts?