dmlc / XGBoost.jl

XGBoost Julia Package
Other
289 stars 110 forks source link

Make CUDA and Term weak dependencies on Julia >= 1.9 #176

Closed devmotion closed 1 year ago

devmotion commented 1 year ago

This PR makes CUDA and Term weak dependencies on Julia >= 1.9 but does not change dependencies on Julia <= 1.8.

To be able to move Term support to a separate extension, it is necessary to remove the definitions of show(::IO, ::MIME"text/plain", ::Node/Booster/etc.) that are based on Term currently. Term-based output can be obtained by explicitly calling Panel(booster) etc. instead. Probably (in a follow-up PR?) one could redefine these show methods using only AbstractTrees and possibly PrettyTables.

On purpose, I did not replace CUDA with GPUArrays(Core): It seems xgboost does only support CUDA currently, so it seemed safer to restrict supported GPU arrays to CuArray instead of a generic GPUArraysCore.AbstractGPUArray.

ExpandingMan commented 1 year ago

Thanks! I must confess I have no idea how this works only for 1.9. I will clone it and take a careful look at it later when I get a chance.

devmotion commented 1 year ago

It's explained here: https://pkgdocs.julialang.org/dev/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions)

I should add that the PR is not ready yet (hence a draft) and not well tested. It was too late when I put it together yesterday,

chriselrod commented 1 year ago

Any progress on this (curious about draft status)?

ExpandingMan commented 1 year ago

This will be a priority for me after the official release of 1.9 (which is imminent), so if the author of this PR doesn't pick this up I will do it within the next couple of weeks.

devmotion commented 1 year ago

I'll have another look but the PR should be ready. IIRC it mainly needs to be tested more thoroughly (even though coverage should not be worse than on the master branch, so maybe it does not matter).

ExpandingMan commented 1 year ago

Alright, I'll check it out when I get the chance and see if we can get it into a state where it can be merged. I must confess I still feel a bit confused by the details of how the new extensions feature works.

devmotion commented 1 year ago

It's a "better Requires.jl". I suggest reading the Pkg docs linked above: https://pkgdocs.julialang.org/dev/creating-packages/#Conditional-loading-of-code-in-packages-(Extensions)

devmotion commented 1 year ago

I just rechecked and all tests pass locally on Julia 1.6 and 1.9 (see also https://github.com/dmlc/XGBoost.jl/pull/180#issuecomment-1545443690).

devmotion commented 1 year ago

BTW I don't think any of these PRs is sufficient to fix #156, and in particular to avoid pulling in the CUDA_runtime_jll dependencies. AFAICT this would require to build separate versions of XGBoost_jll with and without CUDA support, e.g., XGBoost_jll and XGBoost_CUDA_jll, similar to SuiteSparse_jll and SuiteSparse_GPU_jll. It's just not clear yet to me how one could optionally use the jll with CUDA support.

ExpandingMan commented 1 year ago

Hmm... I did not realize xgboost_jll was the thing pulling in CUDA_runtime_jll... Come to think of it I have no idea how to solve that issue, I don't know if it's possible to have separate versions based on whether or not cuda is present.

ExpandingMan commented 1 year ago

This looks good to me, but what happened to the CI/CD? Did we somehow lose it again? "Checks" is empty.

devmotion commented 1 year ago

I re-opened the PR to re-trigger CI. Apparently the CI run has to be approved first.

ExpandingMan commented 1 year ago

We were already failing on windows so I will merge this this weekend unless there are any objections.

ExpandingMan commented 1 year ago

Thanks @devmotion !