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

[docs][python] add conda-forge install instructions #3544

Merged
merged 14 commits into from
Jan 11, 2021

Conversation

raybellwaves
Copy link
Contributor

closes #3542

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

thanks!

Just a small suggestion. I'll let other reviewers comment on whether we want to document this here before I approve, but in my opinion this is a good addition.


conda install -c conda-forge lightgbm

**Note**: The `lightgbm-feedstock <https://github.com/conda-forge/lightgbm-feedstock>`_. is not maintained by LightGBM maintainers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
**Note**: The `lightgbm-feedstock <https://github.com/conda-forge/lightgbm-feedstock>`_. is not maintained by LightGBM maintainers.
**Note**: The `lightgbm conda-forge package <https://github.com/conda-forge/lightgbm-feedstock>`_ is not maintained by LightGBM maintainers.

Copy link
Collaborator

@StrikerRUS StrikerRUS Nov 9, 2020

Choose a reason for hiding this comment

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

I'm doubt that we should document conda installation at all, because we do not maintain it and hence cannot guarantee the quality (#3414) and frequency of version updates. We don't aim to document all available ways to install LightGBM via all available package managers in the world.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually didn't even know about https://anaconda.org/anaconda/lightgbm that you just mentioned in #3544 (comment) . I think that's reason enough to give some guidance in our docs about how to install with conda.

We don't aim to document all available ways to install LightGBM via all available package managers in the world.

Ok but this isn't an obscure package manager like nix or snap. I think it's common, in machine learning projects in Python, to prefer conda install to pip install and I think that explicitly stating "yes this exists but we don't maintain it" in our documentation is useful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm not 100% sold on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, I'll defer to what you decide.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should ask other maintainers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. @henry0312 or @guolinke what do you think? The question is:

should the LightGBM Python documentation explicitly mention installing with conda install, even though we don't maintain the conda packages?

Copy link
Collaborator

Choose a reason for hiding this comment

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

gently ping @henry0312 or @guolinke for an opinion

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry for missing this.
I think pip is the "lighter" way, as it requires fewer dependencies. So we prefer to install via pip.
Many machine learning tools have heavy dependencies, such as cuda, so they prefer conda, which pre-installs these dependencies.

However, I think we can still provide the conda install command, but clarify that we don't maintain it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks! Ok I agree with that. I can review from here.

@StrikerRUS
Copy link
Collaborator

Why conda-forge but not official conda channel?

@raybellwaves
Copy link
Contributor Author

Why conda-forge but not official conda channel?

In my experience conda-forge is more up to date than the anaconda channel.

e.g. conda-forge is at v3.0.0 (last update 8 days ago) and anaconda is at v2.3.0 (last update 8 months and 11 days ago)

@raybellwaves
Copy link
Contributor Author

I apologize for having some of the convo regarding this in the issue chat here

The other thing to discuss is where to put this. I feel it should go in https://github.com/microsoft/LightGBM/blob/master/python-package/README.rst#installation and the install instructions in https://lightgbm.readthedocs.io/en/latest/Python-Intro.html#install are updated from

You can install LightGBM via pip

pip install lightgbm
Refer to Python-package folder for the detailed installation guide.

To verify your installation, try to import lightgbm in Python:

import lightgbm as lgb

to

Refer to Python-package folder for the installation guide.

To verify your installation, try to import lightgbm in Python:

import lightgbm as lgb

@ghost
Copy link

ghost commented Nov 10, 2020

CLA assistant check
All CLA requirements met.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

@raybellwaves sorry for our delay on reviewing this! Thanks for working on it. Can you please see my recent review comments?

docs/Python-Intro.rst Outdated Show resolved Hide resolved
python-package/README.rst Outdated Show resolved Hide resolved
python-package/README.rst Outdated Show resolved Hide resolved
@StrikerRUS StrikerRUS requested review from StrikerRUS and removed request for StrikerRUS January 9, 2021 14:45
raybellwaves and others added 6 commits January 9, 2021 21:24
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Copy link
Collaborator

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

@raybellwaves Thanks for your contribution! I have just few minor comments.

Comment on lines 144 to 145
Install from `conda-forge <https://anaconda.org/conda-forge/lightgbm>`_
'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Install from `conda-forge <https://anaconda.org/conda-forge/lightgbm>`_
'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
Install from `conda-forge channel <https://anaconda.org/conda-forge/lightgbm>`_ Using ``conda``
'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''

Copy link
Collaborator

Choose a reason for hiding this comment

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

@StrikerRUS I previously recommended removing the "Using" part (#3544 (comment)). I think it makes the title more verbose but doesn't provide any additional information

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see, OK!

Then please partly ignore my previous suggestion. And I think it will make sense to remove the "Using" part from Install from PyPI Using pip heading above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the discussion. I'm unsure on what the suggestion is here. You are welcome to push to my branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry for the confusion. I just pushed 5a91ca1 to resolve this thread.

Copy link
Collaborator

Choose a reason for hiding this comment

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

and ffd78bd to address @StrikerRUS 's suggestion.

python-package/README.rst Outdated Show resolved Hide resolved
python-package/README.rst Outdated Show resolved Hide resolved
raybellwaves and others added 2 commits January 11, 2021 09:54
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

thanks Ray, looks good to me!

Copy link
Collaborator

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

@raybellwaves Thanks a lot for your contribution!
@jameslamb Thanks for the help!

@StrikerRUS StrikerRUS changed the title DOC: add conda-forge install instructions [docs][python] add conda-forge install instructions Jan 11, 2021
@StrikerRUS StrikerRUS merged commit 78d31d9 into microsoft:master Jan 11, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add conda-forge as option in Installation guide?
4 participants