Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to latest bundled binaries #119

Merged
merged 39 commits into from
May 25, 2022
Merged

Update to latest bundled binaries #119

merged 39 commits into from
May 25, 2022

Conversation

yaxxie
Copy link
Contributor

@yaxxie yaxxie commented May 16, 2022

  • Fix the CI
  • Add 1.7 to CI matrix
  • Fix 1.7 tests
  • Upgrade bundled binary to latest LGBM release (3.3.2 as of writing)
  • Update README

@yaxxie
Copy link
Contributor Author

yaxxie commented May 17, 2022

@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
Copy link
Contributor Author

yaxxie commented May 17, 2022

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

@yaxxie
Copy link
Contributor Author

yaxxie commented May 23, 2022

@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 [email protected] and ask for MLJBase the solver gives [email protected], 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 yaxxie changed the title WIP Update to latest bundled binaries May 23, 2022
@yaxxie
Copy link
Contributor Author

yaxxie commented May 23, 2022

@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+

@yaxxie yaxxie marked this pull request as ready for review May 23, 2022 20:47
@ablaom
Copy link
Contributor

ablaom commented May 23, 2022

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
Copy link
Contributor

ablaom commented May 23, 2022

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.

cleaner shrimp
@kainkad
Copy link
Contributor

kainkad commented May 24, 2022

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
Copy link
Contributor

kainkad commented May 24, 2022

@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
Copy link
Contributor Author

yaxxie commented May 24, 2022

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

@yaxxie yaxxie merged commit f59fc0b into master May 25, 2022
@yaxxie yaxxie deleted the update_bundled_binary branch May 25, 2022 12:49
@ablaom
Copy link
Contributor

ablaom commented May 25, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants