support OpenCL backend

[simeonschaub]

Copying memory to itself causes problems when running with the OpenCL backend where aliased memcopies throw an error. According to the docstring of copy!:

Behavior can be unexpected when any mutated argument shares memory with any other argument.

so this should probably be avoided.

(This doesn't fully fix CoolPDLP.jl on OpenCL.jl, since
adapt(OpenCLBackend(), arr) is not type stable, so the @stable
annotation would need to be removed for perform_conversion) Implemented

[gdalle]

Thanks!

• Can we add tests on the OpenCL backend?
• How come we even have a self-copy in the first place?

[simeonschaub]

Can we add tests on the OpenCL backend?

Sure, I can add some tests using PoCL. Are you fine with removing the @stable requirement for perform_conversion?

How come we even have a self-copy in the first place?

Just above, the logic is:

if restart_stats.restart_from_avg
    sol_cand = sol_avg
else
    sol_cand = sol
end

so iff !restart_stats.restart_from_avg, we get the self-copy

[simeonschaub]

I had to disable @stable altogether for now. Initially I was just going to mark the relevant methods as @unstable but since that doesn't recursively apply to other functions calling those methods this didn't work. Happy for other suggestions though, I have personally had good experiences with using JET to test the type stability of the main method instances you care about, but it's more manual

[gdalle]

Perhaps we could just temporarily slap an @unstable on every function that ends up calling the problematic conversion you spotted? There shouldn't be many

[simeonschaub]

OK, I now added the @unstable annotation only where necessary

[gdalle]

The buildkite failure is a red herring (any idea why it happens?).
I'll review today :)

[simeonschaub]

Hangs should hopefully be fixed by JuliaGPU/GPUCompiler.jl#830

[gdalle]

You can merge from main now

[gdalle]

Thanks! I gave it some further thought and I don't think it's worth making so many functions @unstable just to accommodate one (rather seldom used) backend. A better option would be to deactivate DispatchDoctor for the OpenCL tests specifically, using e.g. https://github.com/MilesCranmer/DispatchDoctor.jl#-options. What do you think?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
src/algorithms/pdlp.jl	100.00% <100.00%> (+8.23%)	⬆️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[simeonschaub]

A better option would be to deactivate DispatchDoctor for the OpenCL tests specifically, using e.g. https://github.com/MilesCranmer/DispatchDoctor.jl#-options. What do you think?

I must admit that I am not a big fan of the DispatchDoctor approach in general, since you are trying to make type stability guarantees for generic methods that then end up calling arbitrary code you have no control over and which might have good reasons to not be type stable in all instances. As I said above, I tend to prefer testing method on the exact type signatures that you want to be type stable with JET, which is more manual, but it doesn't break the flexibility multiple dispatch and a dynamic type system gives you in the same way DispatchDoctor does.

That said, this is not my package and this is just my personal opinion, so I am fine going with your proposal for now of disabling DispatchDoctor for the OpenCL tests specifically

[gdalle]

DispatchDoctor is much easier for developers than JET, which frequently breaks or is overly sensitive and requires manual testing for each function. Let's keep it that way for the time being. Note that, like our OpenCL test suite, downstream users can also disable DispatchDoctor specifically for CoolPDLP if they have purposefully type-unstable routines.

[simeonschaub]

OK, I have implemented those changes, does this look good to you?