search

rapidsai / dask-build-environment

Build environments for various dask related projects on gpuCI
4 stars 11 forks source link

Add dask-cudf and dask-cuda to Distributed environment #73

Closed charlesbluca closed 1 year ago

charlesbluca commented 1 year ago

This should allow us to run a few of the tests getting skipped in Distributed's GPU CI, namely one added in https://github.com/dask/distributed/pull/8148 that requires dask-cuda

pentschev commented 1 year ago

Sorry, I was too late. I'm just wondering if we should actually be testing Dask-CUDA stuff here. Would it make more sense to add basic tests to Distributed's gpuCI and move the full tests to the Dask-CUDA repo?

charlesbluca commented 1 year ago

Maybe, meant to leave some comments in #8148 around the fact that I would've expected cuDF spill logging to work with a standard LocalCluster, but I seemingly needed to use CUDAWorkers for things to get initialized properly - okay with migrating tests over to dask-cuda if you think things are stable enough in Distributed where we don't need to check on every PR, but will note that test_rmm_diagnostics.py also depends on dask-cuda

pentschev commented 1 year ago

I understand your point, I'm myself over the wall in this situation. I think it would make more sense if we keep dependencies to a minimum here, testing against Dask-CUDA feels to me like we're trespassing a bit. I'm ok if people have different opinions and we should keep those tests here, I'd prefer preventing breaking gpuCI too much here, as it may start getting ignored by contributors when they see it's consistently failing.

charlesbluca commented 1 year ago

That makes sense to me, I've triggered a rebuild already so can follow up here if I encounter any issues there or on subsequent GPU CI runs

pentschev commented 1 year ago

Thanks Charles, just to be very clear, I don't want to dictate what we should do here. If there are diverging opinions and strong reasons to have it in here as well, I'm more than happy for us to continue discussing and shift my thinking. 🙂

pentschev commented 1 year ago

Although, thinking a little bit further we want to have your cuDF PR tested. So let's add dask-cuda/dask-cudf for now and get your PR merged and discuss this matter again once that's in? Sorry for the back and forth.