Move typelevel/algebra into cats repo #3877
Conversation
|
Pending final answers to questions 1 and 2 above, I think this is in good shape for a basic code migration. Is there any follow-up work to do? e.g. typelevel/algebra#199 |
|
I'd vote for Note that I'd like to include one change from Spire if this gets baked into cats: right now, So the algebra hierarchy goes |
|
Thanks for that work! It'll take me a bit to go through everything. To answer your questions:
Like @denisrosset I'd say
No, I think that's too hard.
It would be best to ask first before sending the PR 🙂 But on a case by case basis we can discuss this, please email me. |
|
Note: I've moved the Spire stuff that should be in algebra in these two PRs: typelevel/algebra#246 typelevel/algebra#247 . The second one can wait forever, but we should consider merging the first one -- as we're breaking binary compatibility, we can make that change now. |
|
@denisrosset Brilliant! That helps a lot, thanks. |
|
I think I understand the ring stuff in algebra pretty well, including the tradeoffs in naming stuff, duplication between standard/additive/multiplicative group-like structure. We also have pretty common data types (polynomials, integers, rational numbers) take exercise this stuff at different levels in the hierarchy. Well... except the noncommutative rings, or worse, structure with noncommutative addition; but I see that algebird depends on those, and they fit into a regular naming scheme already in cats-kernel. However, I don't understand well the structure of the lattice subpackage. Why is there duplication between Should we merge all of that as it this into cats? |
algebra-core/src/main/scala/cats/algebra/ring/EuclideanRing.scala
Outdated
Show resolved
Hide resolved
|
@denisrosset How do you think should we move forward on this? Personally I am a big fan of not letting best be the enemy of good 😄 So if we have some actionable changes then we should do them but otherwise we can let it be. |
|
typelevel/algebra#108 looked easily actionable to me and also seemed to achieve a consensus in the original discussion (I assume it was never actioned due to binary compatibility constraints). At least for me, the biggest confusion working on this is not determining what is/isn't actionable but what there is/isn't consensus to take action on :) I think it's worth trawling algebra issues/pulls for other binary-incompatible changes since this is our chance to get them in and then ... put them to a vote?? |
| @@ -0,0 +1,10 @@ | |||
| package cats.algebra.laws.platform | |||
|
|
|||
| private[laws] object Platform { | |||
There was a problem hiding this comment.
Can we use the same definition from kernel-laws? There, it is defined as private[cats] so it should be usable here.
There was a problem hiding this comment.
Yup, I think so. Adding the kernel-laws dependency also lets us drop a bunch of other dupes, like serialization laws.
There was a problem hiding this comment.
Done. Now also using Serialization, Eq, and (Partial)Order tests directly from kernel-laws.
AFAICT most issues on Algebra are 4+ years old, so not sure if there's renewed interest in voting. Since I have no horse in the (algebra) race I'd suggest that @denisrosset and you just pick all changes that you feel are appropriate? |
| * This type can be useful for problems where multiple algorithms can | ||
| * be used, depending on the type classes available. | ||
| */ | ||
| sealed trait Priority[+P, +F] { |
There was a problem hiding this comment.
It looks like this trait is never actually used, so I think we should scrap it.
There was a problem hiding this comment.
Funny, I make good use of it in my own project and maybe it even got submitted to Scala? typelevel/algebra#67
Anyway, scrapped.
|
@denisrosset @johnynek I just noticed your parallel discussions in typelevel/algebra#246 and typelevel/algebra#247, which makes my work here confusing (if algebra is not frozen, then we should put this migration effort on halt). May I kindly request that either we discuss here, or perhaps @denisrosset can make his PRs against armanbilge:migrate-algebra? |
|
@armanbilge I suggest we finish discussing the details on the separate PRs, and then I'll be happy to rewrite the changes against your PR. Works for you? I'll dig a bit into the logic/lattice stuff, but I never used it in my own code. Right now, I don't get some of the design decisions, but it may click at some point (anyway, do you need the lattice/logic stuff for your code? at least the lattice stuff is textbook clear, we could merge that part first if needed). |
|
@denisrosset No problem, perhaps I was too gung-ho with this PR. I'm personally not in any rush for it so (assuming no other urgency) it can lay low until things stabilize over at algebra, at which point I can continue migration. |
|
I'm very strongly of the opinion that users do not benefit from us changing from There is a nice motivation, from which users benefit, from us having the code in this repo: we can more easily publish and we are more likely to leverage algebra in cats proper. But, breaking every previous user of algebra just to make the package match cats, to me, is a loss. Algebra was an effort started many years ago to allow Spire and Algebird to have interoperable types. It took a long time to even achieve that goal. It was easy to make the algebra code, but to actually include it in algebird and spire took a few years. Every breaking change puts a cost downstream that we don't see here. It's a negative that generally slows, not speeds, adoption. This PR proposes we will say to the two projects that came together to make algebra: you need to push a binary incompatible change onto your users so we can have a package name we prefer more. In fact, algebra has changed VERY little over the years. This is not surprising: it was already a distillation of the common parts of algebird and spire, each of which already had some learnings in the design space. I would MUCH prefer if we just keep the same artifact names I don't want to see this as an opportunity to break things just because we feel it is a fine time to make some cosmetic changes. |
|
Algebra has been released for Scala 2.12, 2.13 and 3, so users do not have any pressing need to migrate to In fact, the two proposed changes (artifact name + package name) go hand in hand and make migration a ton easier, since both artifacts can coexist. OTOH, keeping both the same and shoe-horning Algebra into the Cats version scheme would be difficult from a maintenance PoV. Since Cats is where most of the action happens nowadays, the ability to re-start development greatly outweighs the (negligible) downstream costs. |
|
Okay so spire and algebird, the folks who made this project, should just ignore cats-algebra and then we have two typelevel algebra projects? Or are you really saying there are negligible downstream users of those two projects (who are impacted by changing the names of these types of folks adopt them). It's also weird how this decision is made. Being a founder of this project doesn't carry much weight? Is the idea we just find one typelevel committer to agree? |
Oscar, I'm not sure where you're getting the idea that your voice is not being heard. We are quite literally discussing right now about the decision, which is because you're commenting on an open not a closed PR. Obviously this contribution has significant impact so we're keeping this open for a bit to discuss. If you scroll up in this thread you see that a lot of discussion has already happened.
That's not what I said at all. My point is that they can migrate at their own pace. Of course I can't speak for Algebird but I'm pretty sure we can commit to porting Spire.
With "negligible downstream cost" I mean hypothetically merging this PR as it is incurs 0 cost to downstream users. It would be different if we'd – for the sake of argument – kept the artifact name but still incur breaking changes. |
It's also worth pointing to the original thread in #2041. As far as I can tell this particular issue was never raised (and certainly not so strongly) and at the time @johnynek seemed open to artifact/package changes, although I recognize that's an older thread, and opinions/requirements certainly evolve.
Difficult maybe but since Cats versioning is ahead of Algebra's, technically this should be possible right? Algebra would skip from 2.2 to 2.6 I think. However, personally I am inclined to agree with @larsrh's point that
Admittedly, I am also not a user of Algebird. But doesn't Spire potentially have some breaking changes ahead, as we figure out how to port it to Scala 3? I can certainly commit to migrating Spire to cats-algebra. |
|
@armanbilge I think the comment I made here is very in line with what I'm saying now: Lars, yes I realize that the issue is still open, but to the point: how will we come to a conclusion. This comment: #3877 (comment) speaks with a sound of certainty: e.g. what package? It does not discuss how we will decide. Obviously, it seems you and I are diametrically opposed on visions for moving forward. Like @armanbilge I don't see a problem with advancing the version number of algebra up to match cats. That seems fine to me. I'd like to see a list of the concrete things we really want to change. If we don't really have those now, why not defer the package/artifact rename until we do. Some of what @denisrosset has suggested amounts to basically a rewrite. If we want to go down that avenue, we shouldn't merge first. Note, I would disagree with his argument (renaming things to CommutativeAdditiveMoniod to Plus0 would be like renaming cats.Functor to Mappable, but we can discuss that separately). If a rewrite is desired, just start as cats.algebra. But importing all the code, renaming the package, but then not actually making the changes incurs the cost without the benefit. Lastly, I think there should be a strong prior on NOT breaking compatibility, rather than assuming a break is negligible costs. Look at the pain of the binary breaks in cats-effect 3. In that example, there is a strong reason for it, yet still the ecosystem has to resort to suffix versioning of downstream dependencies. I think we should put the burden of proof on those who argue for a break, not on those who argue against it. |
|
Also in fairness to you all, if you want to reply to my subtweet of this event: |
You are overinterpreting. I made my comment about two weeks after the initial PR has been created with nobody else chiming in. Forgive me for not prefixing literally everything I write with "IMHO".
That much is clear. Then again it does not give you the right to complain that you're not being heard.
I don't get this, there are changes in this PR beyond renaming the package.
CE3 is not comparable to this because they didn't change the artifact coordinates. |
In case you missed it, I did make the changes that you and @denisrosset discussed in typelevel/algebra#246 and typelevel/algebra#247. As well as some other changes (open to review/discussion of course) to provide a better cats experience: removing conflicting implicits (although it may be possible to do this in a binary-compat way) and using the laws/test distinction. |
|
@larsrh you say I am not complaining about not being heard. I asked a question about how this decision is made: I think I do have the right to ask how a decision is made on this. If I don't who does? I am a committer in cats and a founder of algebra. Not enough contribution to get a seat at the table? It sounds like you are saying "we have left the PR open, you can comment here" but then? How do we decide to merge? I mean, I believe in free and open source software, so you can do whatever you want, but taking someone else's project copying the code and then saying "yeah, we heard your objections" isn't a friendly way to work, IMO. About the changes: the changes you two reference could have been included in algebra (and indeed were PRs before @denisrosset ran out of time). So, the motivation to make a new artifact and package name, to me, should be to correct things that cannot be done in a compatible way. |
Literally nobody is denying you the right to discuss this here. You made a point. I made a counter-point. You complained that your opinion does not carry weight, which is a non-sequitur.
I don't know, in the absence of any formal voting process, wait for a few other people to chime in? We're not on a deadline here. What makes this PR so different from others before it? Do you claim a veto right? And please don't tell me this isn't a friendly way to work when you're at the same time insinuating that I'm not listening to you. |
My understanding was @denisrosset made those PRs in response to this one (see #3877 (comment)) and they would be binary-breaking to algebra.
I was definitely confused about this when I started as well #3877 (comment). I was (and still am) open to go any direction. So far, @denisrosset @larsrh and @johnynek have chimed in, 2 for the artifact/package change, 1 against. Plenty in the original thread #2041 also seemed to support a change. |
|
@Lars I guess we can't discuss this. You keep putting words in my mouth: I said:
and you replied:
Are you really saying that it should be totally fine to take some project, import it into cats, make a package name change, and ignore the folks who made the project? That's what makes this different? In 2013 @non @tixxit @sritchie and I had a call to try to unify the typeclasses in Spire and Algebird. I think the fact that was the mission of the project and we worked on it for the past 8 years is what is different here. You have the right to copy any OSS code you want. But I don't think you should surprised if you copy someone else's project into cats, but then don't come to an agreement that they feel frustrated. This is not a new story. |
|
If we're going to break binary compatibility, then changing the package name is a good approach, which will avoid linkage errors and allow co-existence until downstream projects can publish for the new artifacts. It seems like the critical question here is whether or not we need to break binary compatibility. I suggest we enumerate the issues that cannot be done without breaking compatibility then figure out a path forward from there. Is that something @denisrosset and @armanbilge can provide? |
|
This PR was binary compatible up to c51b726. I'm no expert on binary compatibility, so I can't be 100% sure without running MiMa, but to the best of my knowledge these are the binary-breaking changes made: a6ec3b8 Merge DeMorganLaws into LogicLaws per TODO Some of these could possibly be done more carefully to maintain binary compatibility. |
We shouldn't think of it as "once we break one thing we might as well change anything we want". With that in mind, binary compatibility for the
I don't see anything in that list that looks like a huge win to users, and I think we can find a way to do what we want and remain compatible. As is probably clear: I am very -1 on us breaking compatibility as perfectionist library authors when that has no significant benefit to users. The users aren't here advocating for themselves. The library authors, who want the code to be as clean as possible and often don't have to deal with building apps composed of hundreds or even thousands of dependencies, are represented. I feel it is important to weight heavily the downstream effects of changes.
|
Personally, I feel the binary-breaking changes could fall into roughly two categories:
I just took a read through #762 which I believe is the origins of
Coupled with @johnynek's #3877 (comment) above:
Now, I understand much better where Algebra's goals (to be agnostic and unopinionated) diverge with Cats' (to define an ecosystem). This leads to pain points such as typelevel/algebra#199 and explains why I am strongly of the opinion that, if one of the goals of moving Algebra into the Cats repo is to re-vitalize development (something that @larsrh and @johnynek seem to agree on), then we should use this opportunity for Algebra to more closely follow the patterns of a Cats-style project. I can certainly understand why 4-5 years ago when Cats was younger the preference for agnosticism, but now Cats seems to be a much more established part of the ecosystem. Any On the other hand, if Algebra's mission first and foremost continues to be to be the bridge between Spire and Algebird, I'm not sure at all it belongs in the Cats repo. I started writing this before @johnynek's comment above about Finally, In case it needs to be said: all just my humble opinion, as a relatively new participant in the Typelevel community (although longtime user of Spire). I opened the PR open to keeping binary compatibility and would be happy to return to that path. |
|
Totally agree on In addition to removing
Me too (see my feedback every time we discuss cats changes for Scala 3, or the great lengths I've gone to in scodec-bits) and I appreciate your reminders about the importance of binary compatibility. I don't think it's fair to say this is a case of perfectionist library authors though -- folks are trying to implement changes that seemed generally agreeable. Let's continue to discuss those changes and figure out if we can do the ones we want without breaking compatibility. We should also figure out how we handle suggestions or ideas which break compatibility and document that process. I want to avoid situations where folks put a bunch of work in to a PR and then we have to abandon it b/c the bincompat implications weren't made clear.
Yep! Hence, if we break compat, a new package name is warranted -- especially given the wildly different release cadences of those projects. But let's defer that discussion until we figure out whether we really need to break compatibility or not. How about this path forward: let's create a new PR that starts at the last bincompat commit, add mima checking for |
Very nearly all of them. See the full story over at #3918. |
|
just wanted to add a data point: https://mvnrepository.com/artifact/org.typelevel/algebra/usages?p=1 there are 35 direct dependencies on algebra. The top three of which have more than 200 dependencies total. We are talking about hundreds of downstream projects impacted here. It's hard to guess the number of developers but I would guess it is likely more than 10k (perhaps much more). To me this is what merits being very biased to not breaking. |
|
Superseded by #3877. |
|
by #3918, you mean :-) |
|
Lol oops! Better fix the algebra README too. Thanks! |
This PR will migrate algebra into cats. As far as I can tell @denisrosset got the final word in at #2041 (comment).
So far I've followed his proposal and moved algebra core and laws into the cats repo as their own modules, which I believe should already be compiling/passing tests for 2.12.
Questions
Should algebra be its own module or merged into kernel? What should we name the artifact?
What should be the package,
algebraorcats.algebra?cats.kernel??Do we care about binary compatibility wrt the previous algebra artifacts?
How to handle collections-related deprecations? Possible solutions:Add@nowarnannotations for 2.13; disable fatal warnings for 3.0Migrate source to 2.13 collections w/ scala-collection-compatMaintain separate version-dependent sourcesI was confused about the build; easy to fix w/
@nowarn.Where should the algebra docs go?The algebra docs were mostly boilerplate except for the overview which I added to the cats docs.
Do we want to keep this tiny benchmark?I'm 99% sure this isn't benchmarking anything, so not migrating it.
This effort is specifically mentioned as a paid maintainer's responsibility; might I kindly expect some compensation? :)