Closed devmotion closed 8 months ago
Attention: 76 lines
in your changes are missing coverage. Please review.
Comparison is base (
cf6e3c0
) 50.80% compared to head (6d33c71
) 51.08%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for the PR!
deps
. It went fine on Julia 1.8, but I encountered an issue on 1.10 which I hadn't time to investigatertstar
test? The need to pass 1_000
rather than the default 100 appears suspicious. I wouldn't expect a significant discrepancy compared to XGBoost (with its default to max_round=10 and learning_rate=0.3, I'd actually expects its fit to be inferior). If you'd have a data to share, I'd be curious to see if there might be an issue in the classifier routine.
- I'm not clear whether CUDA should be kept in deps. It went fine on Julia 1.8, but I encountered an issue on 1.10 which I hadn't time to investigate
Yes, CUDA should be kept in the deps
section for backwards compatibility with Julia < 1.9. https://pkgdocs.julialang.org/v1/creating-packages/#Transition-from-normal-dependency-to-extension state explicitly
Make sure that the package is both in the [deps] and [weakdeps] section. Newer Julia versions will ignore dependencies in [deps] that are also in [weakdeps].
Problems with 1.10 should be caused by something else.
2. I'm not clear why the CI test did fail on nightly. My understanding is that CUDA should load fine regardless if the machine isn't NVIDIA compatible. Has there been a change to that in Julia 1.10?
No, it should work fine (the main problem with CUDA is that it pulls in many dependencies and in particular CUDA jlls). The problem with the current nightlies (https://github.com/TuringLang/MCMCDiagnosticTools.jl/actions/runs/6428712477/job/17456380934#step:6:335) is that CUDA defines GPU overrides for Random.make_seed
(https://github.com/JuliaGPU/CUDA.jl/blob/da140359fc68f453e511608d9af552c90d699d9a/src/device/random.jl#L53) but Random.make_seed
was removed last week (https://github.com/JuliaLang/julia/pull/51436).
Thanks for the clarifications. Regarding the Project.toml keys order, I'll likely move towards the Julia v1.6 order in subsequent release. Let me know if that's an issue for you. Or feel free to make an adjustment in this PR. Otherwise, good to go!
I updated the order of the sections in the Project.toml file to the convention in Julia > 1.6.
Registraion of new release v0.16.3 is underway. Thanks for your patience!
Thank you!
I moved the CUDA-specific code to an extension. On Julia < 1.9, CUDA will continue to be a dependency and CUDA is supported automatically; on Julia >= 1.9, the package will not install CUDA by default but CUDA support can be enabled by loading CUDA (explicitly with e.g.
using CUDA
or when loaded by another dependency).It was reasonably straightforward:
device_ones
->ones
on the CPU,CUDA.ones
on the GPU,device_array_type
->Array
on the CPU,CuArray
on the GPU, andpost_fit_gc
-> no-op on the CPU and garbage collection on the GPU) were sufficient.The main problem is that I don't have access to an NVIDIA GPU, and hence can't verify that I did not break CUDA support 😅 I would recommend adding CUDA-specific tests with buildkite to the repo (see https://github.com/JuliaGPU/buildkite#adding-a-repository, the setup is quite straightforward, I did add it to a few repos successfully).
Fixes #226.