IQVIA-ML / LightGBM.jl

Julia FFI interface to Microsoft's LightGBM package
Other
93 stars 10 forks source link

feat: use LightGBM_jll instead of precompiled binaries #134

Closed characat0 closed 4 months ago

characat0 commented 1 year ago

https://github.com/IQVIA-ML/LightGBM.jl/issues/112 Using BinaryBuilder will improve reproducibility and prevent platform dependant issues.

characat0 commented 1 year ago

https://github.com/IQVIA-ML/LightGBM.jl/issues/122 might be related too

kainkad commented 1 year ago

Hi @characat0 - thank you for this PR. It seems that LightGBM_jll has compatibility issues though with lower Julia versions and majority of the CI tests are failing.

characat0 commented 1 year ago

Hi @characat0 - thank you for this PR. It seems that LightGBM_jll has compatibility issues though with lower Julia versions and majority of the CI tests are failing.

Yes, I authored the package but didn't consider support for <1.6, since the Artifact system came in 1.3, is it possible to consider making >=1.6 a requirement for this package? Julia has evolved so much since 1.0 and most Industry Data Science workflows involve packages that already have that or greater requirements.

kainkad commented 1 year ago

Hi @characat0 - thank you for this PR. It seems that LightGBM_jll has compatibility issues though with lower Julia versions and majority of the CI tests are failing.

Yes, I authored the package but didn't consider support for <1.6, since the Artifact system came in 1.3, is it possible to consider making >=1.6 a requirement for this package? Julia has evolved so much since 1.0 and most Industry Data Science workflows involve packages that already have that or greater requirements.

Thanks, makes sense for the unsupported versions. I still have some questions:

rikhuijzer commented 1 year ago

Is it possible to make using _jll optional together with the existing 2 ways of using binaries?

It should be quite safe to use only the BinaryBuilder system. For example, see XGBoost.jl where I cannot find evidence that they use any workarounds at the side. Also, many Julia core features are provided via BinaryBuilder only if I'm not mistaken. BinaryBuilder has been setup to allow as many platforms as possible as can be seen at https://github.com/JuliaBinaryWrappers/LightGBM_jll.jl#platforms.

kainkad commented 4 months ago

Thanks @characat0 for looking into this. I think it's reasonable at this stage to go ahead with using jll for the lightgbm v3 until we switch to v4 to address the recurring issues with builds on arm based devices. Would it be possible for you to update the matrix versions to run julia from 1.6 and add macos-14 to supported OSs as it should build now without any errors. I'm not quite sure why Windows on 1.8 is failing https://github.com/IQVIA-ML/LightGBM.jl/actions/runs/8766025114/job/24096116198?pr=134 but given that it works on all other versions, this particular combination could be excluded from the matrix. This should resolve the CI failures so it can be merged in.

characat0 commented 4 months ago

I made a new release for v3.3.5 that should fix the bug in windows and julia 1.8 Should I remove the macos setup step too? Since the jll should work out of the box

kainkad commented 4 months ago

I made a new release for v3.3.5 that should fix the bug in windows and julia 1.8 Should I remove the macos setup step too? Since the jll should work out of the box

That's great you got the fix for windows and julia 1.8 combination 🙂. Yes, the macos setup shouldn't be needed with the jll.

characat0 commented 4 months ago

I'm thinking of updating Project.toml to specify compatibility for julia>=1.6, does that change fit here?

characat0 commented 4 months ago

turns out the issue was resolved only for julia 1.9 Even XGBoost errors the same way in julia 1.8.4 and 1.8.5 and since we want to support those versions, I made a workaround to download the precompiled binaries for windows and those specific versions, showing a warning sign. Maybe we should keep this until julia 1.8 is no longer supported

kainkad commented 4 months ago

turns out the issue was resolved only for julia 1.9 Even XGBoost errors the same way in julia 1.8.4 and 1.8.5 and since we want to support those versions, I made a workaround to download the precompiled binaries for windows and those specific versions, showing a warning sign. Maybe we should keep this until julia 1.8 is no longer supported

Thanks for that windows-julia 1.8.4-1.8.5 hack and using the downloads instead for this specific case. I have also removed the previously required checks on ubuntu with julia 1.0 - 1.5 as this is something that couldn't be updated on the CI matrix itself. Looks good to me and happy to merge but let me know if you were planning to add anything else before we can merge. Once merged I'll update the readme as it contains the old info on the installation/build and then release with these changes so hopefully we'll see no more mac os issues 😊.

characat0 commented 4 months ago

I think we are good to go here, will this be released as 0.7?

kainkad commented 4 months ago

I think we are good to go here, will this be released as 0.7?

Yes it will go as 0.7