Fix compat with GenericSchur and GenericSVD

[davidanthoff]

The general type piracy in this package here, GenericSVD and GenericSchur recently broke precompile across an incredibly large number of packages, for example pretty much all of Queryverse. This PR is an (incomplete) attempt to improve on the current situation.

The backstory is that this package here, GenericSVD and GenericSchur all commit type piracy with LinearAlgebra methods, and in some cases the three packages add the same methods. The net effect is that if one tries to load these packages simultaneously, precompile is broken. This issue was brought up before (#60) and the suggested resolution was that one shouldn't use these packages at the same time. I think this is no longer a realistic position: there are many packages that pull these packages here in, and so as an end-user it is way too easy to end up with multiple packages via indirect dependencies, an incredibly frustrating experience. Where this hit recently is that DoubleFloats has a dependency on GenericSVD and GenericSchur and Polynomials, and Polynomials has a dependency on GenericLinearAlgebra (thanks @pearlzli and @GregPlowman for figuring this out!), so any package that depends on DoubleFloats currently has precompile broken, which via TextParse is for example a huge part of Queryverse.

This PR here tries to tackle the problem in the following way: wherever GenericLinearAlgebra added a method that is also present in GenericSVD or GenericSchur, I removed that method in GenericLinearAlgebra, so that the version in GenericSVD or GenericSchur is used instead. These methods were literally identical, so I think this should be mostly a very harmless change.

As part of that I also made use of the Hessenberg struct in newer versions of Julia that is already used in GenericSchur. That part required me to use some internals from GenericSchur, @RalphAS would you be willing to treat these internal details as part of the public interface of GenericSchur, insofar that you would bump the major version of the package if you changed anything about that? This might also constitute a breaking change, I guess.

This PR gets rid of a lot of the conflicts, but not all of them. There is still a problem with schur! and eigvals! that I was not able to solve, primarily because at first sight the implementation in GenericLinearAlgebra and GenericSchur looked quite different, so I was not sure what to do there. @RalphAS and @andreasnoack do you have an idea? To see what exactly is still broken, just run the tests on this branch and you should get the relevant warning messages. My gut sense is that GenericLinearAlgebra should probably hook into GenericSchur.gschur! somehow?

Also pinging @JeffreySarnoff (from DoubleFloats), @michakraus and @jverzani (from the Polynomials change) and @simonbyrne (from GenericSVD).

I would greatly appreciate it if we could try to find some solution for this as a priority because of the very large number of packages and users affected (and on a selfish note this is causing huge problems for an internal project at the moment as well).

[codecov-io]

Codecov Report

Merging #71 (9ca91cf) into master (74e8183) will decrease coverage by 1.56%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
- Coverage   72.53%   70.97%   -1.57%     
==========================================
  Files          11       11              
  Lines        1453     1502      +49     
==========================================
+ Hits         1054     1066      +12     
- Misses        399      436      +37     

Impacted Files	Coverage Δ
src/GenericLinearAlgebra.jl	100.00% <ø> (ø)
src/qr.jl	97.67% <ø> (-0.41%)	⬇️
src/svd.jl	80.70% <ø> (+0.84%)	⬆️
src/eigenGeneral.jl	64.81% <50.00%> (-1.46%)	⬇️
src/householder.jl	48.27% <0.00%> (-31.49%)	⬇️
src/juliaBLAS.jl	18.62% <0.00%> (-4.08%)	⬇️
src/cholesky.jl	100.00% <0.00%> (ø)
src/eigenSelfAdjoint.jl	92.44% <0.00%> (+0.45%)	⬆️
src/rectfullpacked.jl	89.47% <0.00%> (+0.58%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74e8183...9ca91cf. Read the comment docs.

[pearlzli]

Thanks for taking the lead on this, @davidanthoff!

[jverzani]

Would it be useful if I removed the GenericLinearAlgebra dependency in Polynomials for now. Its usage is not central to that package.

[davidanthoff]

@jverzani that would probably be a quick fix that would work! If that would be ok with you, it would certainly be great to fix this quickly.

I think we should probably still pursue something like this PR in parallel, though, you shouldn't have to make those kind of decisions in your package :)

[jverzani]

I'll merge this as soon as it passes and bump the version.

[andreasnoack]

While I fully understand how frustrating it can be that precompilation fails because of conflicts between indirect dependencies and I appreciate the effort to resolve the conflicts, I don't think this PR is the right approach. In theory, this package serves two purposes: 1. it provides some generic algorithm 2. it defined pirate methods that make the algorithms in 1. easy to use. In my opinion 2. makes this package as well as the two conflicting packages unfit as direct dependencies. Instead, I think they should be loaded by end-users that are interested in broader linear algebra routines, and, therefore, I think it's right that Polynomials removes the dependency on this package.

I suspect that the main usage of this package is for 2., but there might be downstream packages that internally make use of the generic implementations here. If that is the case, then I should consider splitting this package in two. The number of direct dependents is fairly small, see https://juliahub.com/ui/Packages/GenericLinearAlgebra/Tm5A3/0.2.4?t=2, so hopefully, we can resolve this soon. I'm pinging the main contributors here to get some feedback

• DFTK (@mfherbst, @antoine-levitt)
• FCA (@lingluanwh)
• Hypatia (@chriscoey)
• MonteCarloMeasurements (@baggepinnen)
• Polynomials
• PressureFieldContact (@ryanelandt)

ArbNumerics and DoubleFloats no longer depend on this package but let me cc @JeffreySarnoff since the general issue applies to GenericSVD and GenericSchur which his packages depend on.

Regarding the relationship between the three conflicting packages here then I think this package covers all of the functionality in GenericSVD so if only that package is involved in the conflict which this one then I think the solution should be to use this package in the case where the maintainer wants to continue to depend on one of the packages. GenericSchur is more complicated since it has more features than the implementation here.

[baggepinnen]

I mitigated the issue by not using GenericLinearAlgebra in MonteCarloMeasurements.jl, I only have it in the project file to declare compatibility and make use of it in the tests.
I have thus effectively taken the second approach. It would however be nice to be able to make the GLA functionality available by default when loading MonteCarloMeasurements, as some users might encounter a missing method error and give up, not having found the possibility of manually making such method available by reading through the documentation.

[JeffreySarnoff]

@andreasnoack Any reason not to incorporate the GenericSchur functionality here?

[andreasnoack]

I only have it in the project file

Doesn't that trigger precompilation of the dependency in Julia 1.6?

Any reason not to incorporate the GenericSchur functionality here?

It's been my intention to cover (at least some [balancing] of) the missing functionality. Unfortunately, I haven't had the time. I might consider to add GenericSchur as a dependency, remove the current overload, at leave the current implementation unexported. However, I don't think changes the fundamental issue that pirating packages such as this one is problematic to have as a dependency.

[ryanelandt]

Packages are created to fill gaps in the software ecosystem. For example, GenericLinearAlgebra was created because parts of Julia algebra library weren't generic. GenericSVD was created because GenericLinearAlgebra didn't have an SVD capability...

When issues like this arise, I'd like to see this solved in such a way that enhances the capabilities of more fundamental packages. I think that @andreasnoack and @JeffreySarnoff are on the right track here. I think that the solution in this PR is a step in the wrong direction.

[RalphAS]

I agree with @andreasnoack position above: I don't think the pirate packages should be dependencies when they are just "friends", and end users should be directed to add them by documentation.

Having the pirate methods is a great convenience for direct users of either package, and essential for transparently adding capabilities to third-party packages. I haven't found a pleasant way to make the piracy optional, but am willing to consider suggestions. I'm also willing to make GenericSchur more cooperative with GLA, if it can be done without loss of utility.

[JeffreySarnoff]

Well, that sounds reasonable, Ralph.

[mfherbst]

Just to confirm that in DFTK our usage of GenericLinearAlgebra is also the second one mentioned by @andreasnoack. I'll also mitigate by moving the dependency to the test dependencies. We only use it to support generic floating point types (intervals, DoubleFloat, etc), which are certainly expert usage, so no need to support it out of the box.
From this perspective I similarly think it is completely reasonable to direct people to our documentation and mention the explicit requirement of adding using GenericLinearAlgebra there.

For me adding GenericLinearAlgebra to DFTK was just the "quick fix to get things working" when I experimented with generic types in DFTK. Back than I wasn't really thinking about the implications, but on hindsight it's not super surprising that hard-depending on a package that type-pirates base might lead to downstream conflicts. Perhaps it would be a good idea to highlight the issue discussed in this PR prominently in the docs of the generic linear algebra packages in the Julia ecosystem, just to avoid other people doing down the same route?

[JeffreySarnoff]

We want GenericLinearAlgebra to "just work" with generic number types that are designed to "just work" with other packages that allow for numbers other than the built-in types. It seems to me that giving this up, relegating such conformance to expert use, detracts mightily from one of the pillars of Julia software construction and use.

[mfherbst]

I agree with you @JeffreySarnoff that this is one of the absolute strong points of Julia and in principle one should not give up on this easily.

In the case of DFTK, however, there are more aspects to consider, just originating from the fact that using floating-point types other than Float64 is pretty much unexplored territory in DFT (the physical problem we implement). So if you are willing to go down that path you will eventually hit other issues (failing numerics, missing support in third-party libraries etc.). I certainly would love to see DFTK work with any floating-point types out of the box in the future, but it's still a bit of research to get there 😄 ... and so I'm ok to ask users for the extra step.

[baggepinnen]

The fundamental problem here is the piracy, so short of moving the implementations to the stdlib LinearAlgebra, maybe a mechanism to legalese piracy in a structured way would be required to solve the problem in general. A more pragmatic approach would be to structure the pirating packages such that they do not duplicate pirating methods, but rather use each other, e.g., GenericLinearAlgebra not defining methods for svd but rather using GenericSVD.

[simonbyrne]

I think one way to resolve this is to deprecate GenericSVD (JuliaLinearAlgebra/GenericSVD.jl#30).