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

lightgbm 3.2.0 #73665

Closed
wants to merge 3 commits into from
Closed

Conversation

chenrui333
Copy link
Member

action-homebrew-bump-formula


Created with brew bump-formula-pr.

@alebcay alebcay added the build failure CI fails while building the software label Mar 22, 2021
Copy link
Contributor

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenrui333 Thank you very much for updating the formula!

Reason of failing CI is in GitHub doesn't include submodules into auto-created artifacts. And it seems that they don't have plans to do this. I've just uploaded complete sources that should be built OK.

Refer to the discussion microsoft/LightGBM#3872 (comment) for more details.

Formula/lightgbm.rb Outdated Show resolved Hide resolved
@SMillerDev
Copy link
Member

Maybe we're better of with a checkout with submodules then?

@StrikerRUS
Copy link
Contributor

StrikerRUS commented Mar 23, 2021

@SMillerDev

Maybe we're better of with a checkout with submodules then?

If this is supported by Homebrew and not hard to implement, I think it will be more reliable than handcrafted archives.
But we're going to automate the process of publishing complete source code at CI level, so there will be no time delta between new release and attaching correct *.tar.gz artifact.

@SMillerDev
Copy link
Member

You just switch from an archive to specifying a tag and a revision.

Copy link
Contributor

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SMillerDev

You just switch from an archive to specifying a tag and a revision.

Like this one?

Formula/lightgbm.rb Outdated Show resolved Hide resolved
@carlocab
Copy link
Member

carlocab commented Mar 23, 2021

Some of these external libs are provided by Homebrew (e.g. eigen, fmt). Can we use Homebrew dependencies for those?

For the remaining two: compute should probably be provided by boost, and fast_double_parser could probably be a new formula.

@StrikerRUS
Copy link
Contributor

StrikerRUS commented Mar 24, 2021

@carlocab

Some of these external libs are provided by Homebrew (e.g. eigen, fmt). Can we use Homebrew dependencies for those?

To be honest, I don't think it is possible and good idea.

We run tests in LightGBM only against fixed commits of those external libs. New versions might be incompatible and break the code. If Homebrew is able to specify exact commit hash for a dependency, it is not a problem, though.

More serious problem is that LightGBM expects all these dependencies to be located in a specific place (we don't respect system-wide installations and need sources of those libs, not compiled files), e.g.
https://github.com/microsoft/LightGBM/blob/4e47c9350828aca5c86ee4221cd72a9aac5e3796/include/LightGBM/utils/common.h#L33-L37

@StrikerRUS
Copy link
Contributor

compute should probably be provided by boost

boost is not needed for ordinary (currently the only available option in Homebrew) installation of LightGBM.

@SMillerDev
Copy link
Member

We run tests in LightGBM only against fixed commits of those external libs. New versions might be incompatible and broke the code. If Homebrew is able to specify exact commit hash for a dependency, it is not a problem, though.

That is conflicting with our requirements for formulae: https://docs.brew.sh/Acceptable-Formulae#stuff-that-requires-vendored-versions-of-homebrew-formulae

carlocab
carlocab previously approved these changes Apr 10, 2021
@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@BrewTestBot
Copy link
Member

⚠️ Bottle publish failed.

@BrewTestBot BrewTestBot dismissed carlocab’s stale review April 10, 2021 01:30

bottle publish failed

chenrui333 and others added 3 commits April 13, 2021 21:39
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@github-actions github-actions bot added the outdated PR was locked due to age label May 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021
@chenrui333 chenrui333 deleted the bump-lightgbm-3.2.0 branch December 18, 2022 05:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build failure CI fails while building the software outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants