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

Remove parallel errors from ZPure #1432

Merged
merged 5 commits into from
Dec 27, 2024

Conversation

ghostdogpr
Copy link
Member

@ghostdogpr ghostdogpr commented Dec 21, 2024

I propose to remove parallel errors from ZPure for several reasons:

  • They were initially introduced when the plan was to make Validation use ZPure, but this idea was abandoned when ZValidation was introduced
  • They are broken since the implementation of foldM is lossy via the use of first: foldCauseM((cause: Cause[E]) => failure(cause.first), success). This means a wide range of operators (mapError, catchAll, etc) are only keeping the first error anyway.
  • The usage of Cause being different from zio.Cause is confusing, as well as the term Par which evokes concurrency, removing it simplifies the signature of runAll.
  • There was a bug for years with zipParLeft and zipParRight not accumulating errors correctly and nobody found it until now, which makes me think nobody uses these.

@ghostdogpr ghostdogpr requested a review from a team as a code owner December 21, 2024 07:57
guizmaii
guizmaii previously approved these changes Dec 22, 2024
Copy link
Member

@guizmaii guizmaii left a comment

Choose a reason for hiding this comment

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

LGTM but @sideeffffect might have a different opinion

kyri-petrou
kyri-petrou previously approved these changes Dec 22, 2024
Copy link
Contributor

@kyri-petrou kyri-petrou left a comment

Choose a reason for hiding this comment

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

I think this change makes sense, not only for the reasons mentioned but also because as a user I found the *Par methods confusing given that they don't execute concurrently (and shouldn't given that ZPure is synchronous)

@guizmaii
Copy link
Member

guizmaii commented Dec 22, 2024

@kyri-petrou I agree this is bad naming. We need to find a solution as this Par naming is used a bit everywhere in zio-prelude

In my opinion, the example of These is representative of our issue.
These has methods named &> and zipPar, while the code of these methods is not parallelising anything.
The issue is that, unlike ZValidation (See #1433), we can't just replace the &> method with *> as *> already exists. Same for These#zipPar as These#zip already exists too.

Any idea, guys, on how we could fix that? 🤔

Copy link
Member

@sideeffffect sideeffffect left a comment

Choose a reason for hiding this comment

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

What I find the crux of the issue is whether we can make foldM behave properly.

If yes, let's modify it to be lossless. Because these Par methods can be useful (and intuitively named, since they hint at semantic parallelism).

If not, let's remove it all.

So what is the case? :)

@ghostdogpr ghostdogpr dismissed stale reviews from kyri-petrou and guizmaii via c567bfc December 27, 2024 00:03
@ghostdogpr
Copy link
Member Author

ghostdogpr commented Dec 27, 2024

What I find the crux of the issue is whether we can make foldM behave properly.

If yes, let's modify it to be lossless. Because these Par methods can be useful (and intuitively named, since they hint at semantic parallelism).

If not, let's remove it all.

So what is the case? :)

I don't think it makes sense to fix foldM and others because their signature takes a single E. Changing it to a list is effectively turning then into the existing Cause variants. And we don't want to keep only those because they make the UX very bad for people who don't use parallel errors (which I assume is nearly all ZPure users, since no one noticed parallel errors were broken).

An alternative is to do cause.forEach(failure) but I think it breaks semantics because it would make operators like catchAll or catchSome run the passed ZPure program multiple times.

I'm personally not too bothered by the Par suffix (once you get used to it, it's fine and cats uses the same terminology), however I find the Cause usage very confusing as the operators mimic the ones with zio Cause but with a completely different behavior. Imagine if catchAll in ZIO was swallowing the Die errors. This is what happens here.

So, I think we should get rid of it 😄 And if someone really want to run multiple ZPure, they can use .either and do what they want with the result pretty easily.

sideeffffect
sideeffffect previously approved these changes Dec 27, 2024
Copy link
Member

@sideeffffect sideeffffect left a comment

Choose a reason for hiding this comment

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

OK, if we can't make it 100 % correct and consistent, then we must remove it.

Are we still using Cause? Or can we remove it altogether now?

@ghostdogpr
Copy link
Member Author

Are we still using Cause? Or can we remove it altogether now?

Removed it (replaced a few usages by ParSeq[Unit, A])

sideeffffect
sideeffffect previously approved these changes Dec 27, 2024
Copy link
Member

@sideeffffect sideeffffect left a comment

Choose a reason for hiding this comment

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

Feel free to merge, thank you 🙏

@sideeffffect sideeffffect merged commit b7b1f09 into zio:series/2.x Dec 27, 2024
20 checks passed
@sideeffffect
Copy link
Member

https://github.com/zio/zio-prelude/releases/tag/v1.0.0-RC36

@guizmaii
Copy link
Member

My PoV on the "semantic parallelism" thing: #1433 (comment)

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

Successfully merging this pull request may close these issues.

4 participants