added option for macOS to use LightGBM_jll #141
Conversation
kainkad
commented
Oct 2, 2023
kainkad
commented
- allows macOS users on both Intel and arm-64 architectures to use LightGBM_jll
- leaves other operating system users with the option to use the current pre-compiled binary (Intel), or custom binaries
.github/workflows/main.yml
Outdated
| @@ -12,7 +12,7 @@ jobs: | |||
| strategy: | |||
| fail-fast: false | |||
| matrix: | |||
| version: ['1.0', '1.1', '1.2', '1.3', '1.4', '1.5', '1.6', '1.7', '1.8', '1.9'] | |||
| version: ['1.6', '1.7', '1.8', '1.9'] | |||
There was a problem hiding this comment.
It's possible to safe some compute time by setting only the following
| version: ['1.6', '1.7', '1.8', '1.9'] | |
| version: | |
| - '1.6' | |
| - '1' # Latest stable release. |
This is fairly common in the Julia ecosystem, see for example https://github.com/JuliaData/DataFrames.jl/blob/main/.github/workflows/ci.yml.
It assumes that if 1.6 works and the latest stable works (1.9.3 currently), then probably everything in between does as well.
src/LightGBM.jl
Outdated
| # for macOS users alternative LightGBM_jll - binaries compiled with Binary Builder | ||
| # particularly to address the issues raised for M1/aarch64-apple-darwin | ||
| @eval using LightGBM_jll | ||
| LGBM_library[] = Libdl.dlopen(find_library("lib_lightgbm", [@__DIR__])) |
There was a problem hiding this comment.
@eval using LightGBM_jll
The using can safely be moved to the package level. The source code inside LightGBM_jll is fairly minimal and shouldn't really affect startup time. Also, if the dependency is specified in Project, then it will be downloaded already anyway.
src/LightGBM.jl
Outdated
| # for macOS users alternative LightGBM_jll - binaries compiled with Binary Builder | ||
| # particularly to address the issues raised for M1/aarch64-apple-darwin | ||
| @eval using LightGBM_jll | ||
| LGBM_library[] = Libdl.dlopen(find_library("lib_lightgbm", [@__DIR__])) |
There was a problem hiding this comment.
LGBM_library[] = Libdl.dlopen(find_library("lib_lightgbm", [@__DIR__]))
This appears to work when testing it on aarch64. I have a LIGHTGBM_EXAMPLES_PATH failure, but apart from that the MLJ interface tests test set passes.
There was a problem hiding this comment.
This is because some tests are directly using Microsoft lightgbm examples so to run these tests you need to export the LIGHTGBM_EXAMPLES_PATH to the location where these examples are downloaded like it's mentioned here.
There was a problem hiding this comment.
Thank you @kainkad. To verify, I just ran this binary-builder branch on an aarch64 laptop with MLJBase = "0.21" in test/Project.toml and all tests pass.
src/LightGBM.jl
Outdated
| @@ -39,7 +40,16 @@ end | |||
|
|
|||
|
|
|||
| function __init__() | |||
| LGBM_library[] = Libdl.dlopen(find_library("lib_lightgbm", [@__DIR__])) | |||
| if Sys.isapple() | |||
There was a problem hiding this comment.
This conditional does not check the right thing. It will incorrectly enter lines 44-47 for x86 Apple systems and will incorrectly enter lines 49-50 for ARM chips such as the ones by Ampere Computing (https://amperecomputing.com/) or the well known Raspberry Pi. It will also not be ready for RISC-V chips that are coming in.
It's probably the safest bet to default always to BinaryBuilder and let that system handle all the different architectures that exist and will exist in the future. Ever since the slowing of Moore's law, "things will only get weirder from here", according to Chris Lattner. Luckily, BinaryBuilder is very future proof with their system.
There was a problem hiding this comment.
This condition works for both x86_64 and aarch64 on macOS (given that most of the installation issues were for M1 users) but it is correct it will not work for non-Apple ARM architectures as it will trigger the other condition which is the current status quo. Of course it can get weirder ;) I'll have a look into a suggestion of overriding the jll artifacts to allow for custom binaries as I wouldn't want to compromise on this as we do have some use cases for custom binaries.
|
@kainkad as a side-note, the tests in Julia 1.9 fail due to a recent breaking MLJBase update (JuliaAI/MLJBase.jl#937) causing errors like To fix it either load the measures from StatisticalMeasures or set in |
src/LightGBM.jl
Outdated
| # for macOS users alternative LightGBM_jll - binaries compiled with Binary Builder | ||
| # particularly to address the issues raised for M1/aarch64-apple-darwin | ||
| @eval using LightGBM_jll | ||
| LGBM_library[] = Libdl.dlopen(find_library("lib_lightgbm", [@__DIR__])) |
There was a problem hiding this comment.
@giordano is it correct that
LGBM_library[] = Libdl.dlopen(find_library("lib_lightgbm", [@__DIR__]))all that is necessary to get the LightGBM_jll binaries?
There was a problem hiding this comment.
The library is automatically dlopened when using the package, I don't understand the point of this complication. If you need only the path, it's LightGBM_jll.lib_lightgbm_path (see https://docs.binarybuilder.org/stable/jll/#LibraryProduct), otherwise I don't know what's happening here.
There was a problem hiding this comment.
The library is automatically dlopened when
usingthe package, I don't understand the point of this complication. If you need only the path, it'sLightGBM_jll.lib_lightgbm_path(see https://docs.binarybuilder.org/stable/jll/#LibraryProduct), otherwise I don't know what's happening here.
Thank you @rikhuijzer and @giordano for your comments. Correct me if I'm wrong but by only using LightGBM_jll it is no longer possible to provide custom built binaries because LightGBM_jll will provide the prebuilt "lib_lightgbm" binaries for the given architecture without giving the user a way to alter it, if needed. Currently, it is possible to still built a lightGBM binary from source and for e.g. get the CUDA version of lightgbm given that the original gpu build is OpenCL (and I think this is from lightgbm v4 not v3 anyway so for v3 either way needs to be built from scratch).
There was a problem hiding this comment.
|
I'm closing this one as there hasn't been any good way to have the binary builder optional. We might just go ahead with this one: #134 |
