Allow turning off checking device factorizations to avoid blocking calls

[kshyatt]

The idea of this PR is to make the checks involving @allowscalar optional. These copies are extremely expensive and block the device, so by (optionally) disabling them, we should be able to make better use of the GPU.

So far I've just done the CUSOLVER ones, but I can also do ROCSOLVER if we like the idea. I think it would make sense to have a settable flag that turns the checks on and off so that users can control it, or a CheckedCUSOLVER driver. Anyone have thoughts on that angle?

[lkdvos]

Definitely looks reasonable to me, although it would be nice to get a sense of how bad the performance is to try and decide if we want to turn this on or off by default.

Is this specifically for the case where we apply the factorizations to a bunch of blocks and don't want to sync inbetween? There might also be a case for writing a batched version that tries to only sync at the end or something similar

[kshyatt]

Is this specifically for the case where we apply the factorizations to a bunch of blocks and don't want to sync inbetween?

Indeed it helps with this but not only here, another example is calc_convergence in PEPSKit where we're computing svd_vals 8 times (4 corners, 4 edges).

[lkdvos]

Yeah fair enough, I definitely think the keyword argument is a good solution, just slightly on the fence about disabling checks by default. 🤷 it's a bit annoying to think that PEPSKit would run without these and then if at some point something doesn't converge it just silently goes haywire, and then you have to do a full rerun with checks on to figure out what happened, but I guess that's what it has to be

[kshyatt]

I agree, that's why I think in fact this should be settable at __init__ or with an env var. But what to call it?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
ext/MatrixAlgebraKitAMDGPUExt/yarocsolver.jl	88.63% <100.00%> (+0.37%)	⬆️
ext/MatrixAlgebraKitCUDAExt/yacusolver.jl	96.22% <100.00%> (+0.10%)	⬆️
src/MatrixAlgebraKit.jl	100.00% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Jutho]

It seems like all four of cuSOLVER.chkargsok, cuSOLVER.chklapackerror, chkargsok and chklapackerror are used. We should probably replace this to something more consistent. Are chklapackerror and chkargsok equivalent? Is one the LinearAlgebra.LAPACK version and the other the cuSOLVER version?

[Jutho]

Also, I don't fully understand why this needs to be so expensive. Is there something with @allowscalar that would be different then e.g. only(collect(info)) or only(copy!(Vector{Int}(undef,1), info)). It does seem like this is the standard way that gesvd is called:
https://github.com/NVIDIA/CUDALibrarySamples/blob/6e7b3fa0debcfc857495b57df196a278d1931e5a/cuSOLVER/gesvd/cusolver_gesvd_example.cu#L110

[lkdvos]

The expensive part is less about the cost of the operation, and more about the fact that sending back a scalar requires synchronization, so it breaks the asynchronous programming model, which im assuming is dominating the runtime for the tail of small blocks. Probably this can be improved in a batched implementation, but I do see the value in having a switch for this, although I do want the default to be having the checks on

[kshyatt]

Indeed I think @allowscalar blocks and synchronizes the entire device, so it's quite painful from an "overlapping work" point of view...

[lkdvos]

Overall I'm definitely okay with this, although it would really be nice to have other ways of circumventing this. For example, can we do a non-blocking NaN-poison of the results? My main reservations are that we do actually have a LAPACK error every now and again, and I have a hard time gauging how much of a pain it is to not have this error and simply produce garbage.

Ultimately, I think the real win is still the batched solution, but since this PR is such a small change and really doesn't hurt I don't see why not :)

[kshyatt]

OK, let me add the ROCm checks too tomorrow

[lkdvos]

Test failures look unrelated, probably ok to force merge