IQVIA-ML / LightGBM.jl

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

Update to latest bundled binaries #119

Closed yaxxie closed 2 years ago

yaxxie commented 2 years ago
yaxxie commented 2 years ago

@IQVIA-ML/lightgbm-maintainers As you can see, the CI for 1.0, 1.1 and 1.2 is failing. It is just dependency resolution for lower versions (specifically related to MLJ) Either I can drop CI coverage for these lower versions, or break tests up so that MLJ tests are run for upper versions and regular tests for lower versions.

If we see here we can see that 1.6.x is the new LTS (supplanting 1.0.x series).

So the question becomes if you're ok with dropping official support for pre-1.6 releases.

yaxxie commented 2 years ago

P.S. I'm working on 1.7 CI fixes. P.P.S I'm also going to upgrade the bundled binary

yaxxie commented 2 years ago

@ablaom I'm doing a bit of investigation as to whats happening for our CI and I found that for v1.0, 1.1 and 1.2 julia versions

If I have MLJModelInterface@1.4.2 and ask for MLJBase the solver gives MLJBase@0.10.1, which is completely wild. In the meantime If I just ask for MLJModelInterface and MLJBase the solver gives much more reasonable versions of 1.3.3 and 0.18.19 respectively.

Because I'd like a) the CI to not fail when it can pass (and I can't actually find someone to turn off early 1.x as required checks) and b) I personally find it quite unsatisfying that we can't continue to support the early 1.x series, I'm going to pin this package to the 1.3.x series of MLJModelInterface -- I'd be grateful if you could summarise what issues this might cause for us down the line.

IFF you also have the inclination, it'd be nice to see this working again. I appreciate that the early 1.x series will never any longer get cutting edge MLJ packages but it doesn't make sense for the solver to give versions which are prior to the initial release of this LGBM package. I totally appreciate you may not care for this given that 1.6.x is now LTS, but as I mentioned, I'd personally appreciate it.

yaxxie commented 2 years ago

@ablaom as a follow up, I can see that pre-1.6 won't be supported. I still think the wonky package resolution is weird but since MLJ no longer cares about pre-1.6 users I've made it so our tests only operate on 1.6+

ablaom commented 2 years ago

I agree it would be nice to maintain support for < 1.6. However, there is very little appetite for this among developers, probably because we are all spread so thin. I recall that the SciML organisation was actively discouraging devs to support < 1.6.

MLJModelInterface 1.4 adds a couple of traits and some doc-string convenience functions. Adding traits should not be breaking - we took special precautions. So I don't see why MLJModelInterface 1.3 is a problem if you want to use an old Julia version. But, as you can see, we've given up and no longer support Julia 1.5 in MLJBase.

ablaom commented 2 years ago

FYI: We are rolling out standardized docstrings for all MLJ models. I have a technical writer that may be able to make a PR, but if you're inclined to do this yourself, all the better.

kainkad commented 2 years ago

Thanks @yaxxie for this PR and yes splitting the CI tests in this case to get all versions run for lightgbm and 1.6+ with MLJ makes sense given the comments from @ablaom I just added minor comments but LGTM

kainkad commented 2 years ago

@ablaom thanks for the heads up on the standardised docstrings. I couldn't open the URL but have looked at the docstrings for MLJ models. A PR would be helpful

yaxxie commented 2 years ago

I have a few things to fix which @FatemehTahavori pointed out to me so please hold off merging

ablaom commented 2 years ago

@yaxxie Sorry, here's the doc-roll out link: link : https://github.com/alan-turing-institute/MLJ.jl/issues/913 The new standard is detailed here: https://alan-turing-institute.github.io/MLJ.jl/dev/adding_models_for_general_use/#Document-strings