Skip to content

[MRG] Allow installation from conda #283

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

Merged
merged 7 commits into from
Mar 31, 2020

Conversation

bellet
Copy link
Member

@bellet bellet commented Mar 30, 2020

Anaconda is widely used so it would be nice to be able to install metric-learn with conda install. After a bit of research I think the best option is to add metric-learn to the popular conda-forge channel instead of creating a specific channel

I have created the recipe and PR there: conda-forge/staged-recipes#11168

@perimosocordiae @terrytangyuan @wdevazelhes I put you in the list of maintainers, so you have to post a comment on the above PR to let them know that you are willing to be listed there

Note: I also noticed that the PyPi entry is somewhat outdated, for instance the badges for travis and codecov do not work probably due to outdated URL. metric-learn appears in the "My projects" section but the "manage" button is greyed out. Can someone give me the management rights and/or fix the small problems in the PyPi entry? Thanks!

@perimosocordiae
Copy link
Contributor

I commented on the conda PR. Thanks for setting this up!

@terrytangyuan
Copy link
Member

Thanks. This looks great!

@bellet
Copy link
Member Author

bellet commented Mar 31, 2020

As requested in the conda-forge PR, I modified setup.cfg to include the license file to future source distributions. I checked locally and it works when running python setup.py sdist

@synapticarbors
Copy link

@bellet If you checked that the entry in setup.cfg works, that's great. I had a hard time finding explicit documentation. In the past I've always suggested including a MANIFEST.in with the proper entry:

https://packaging.python.org/guides/using-manifest-in/#using-manifest-in

but if the setup.cfg route works, that's obviously fine too.

@bellet
Copy link
Member Author

bellet commented Mar 31, 2020

@bellet If you checked that the entry in setup.cfg works, that's great. I had a hard time finding explicit documentation. In the past I've always suggested including a MANIFEST.in with the proper entry:

https://packaging.python.org/guides/using-manifest-in/#using-manifest-in

but if the setup.cfg route works, that's obviously fine too.

Yes, I read about both solutions. I checked on my local machine and the added line in setup.cfg works (the tarball does contain the license file after the change). But I don't know how robust this behavior is. @perimosocordiae @terrytangyuan @wdevazelhes maybe you guys can give it a try on your local machine? Otherwise we can also add a MANIFEST.in file with the appropriate entry

@terrytangyuan
Copy link
Member

@bellet @synapticarbors I think adding it in setup.cfg has the same effect (reference). It works for me locally.

README.rst Outdated
@@ -33,6 +33,8 @@ metric-learn contains efficient Python implementations of several popular superv

Run ``pip install metric-learn`` to download and install from PyPI.

Run ``TO COMPLETE`` to download and install in conda.
Copy link
Member

Choose a reason for hiding this comment

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

We can link to instructions here so it's less verbose in our README? https://github.com/conda-forge/metric-learn-feedstock#installing-metric-learn

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I have added the simple instructions to install and pointed to this page for more options

README.rst Outdated

Run ``python setup.py install`` for default installation.
- To install from PyPi: ``pip install metric-learn``.
Copy link
Member

Choose a reason for hiding this comment

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

PyPi -> PyPI

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, fixed!

@bellet bellet changed the title Allow installation from conda [MRG] Allow installation from conda Mar 31, 2020
@bellet
Copy link
Member Author

bellet commented Mar 31, 2020

Just tested the conda recipe for both Python 2 & 3 and it works well

@perimosocordiae perimosocordiae merged commit 8a02af7 into scikit-learn-contrib:master Mar 31, 2020
@perimosocordiae
Copy link
Contributor

Thanks again, merged.

@bellet bellet deleted the conda-forge branch March 31, 2020 16:43
@bellet
Copy link
Member Author

bellet commented Apr 3, 2020

Note: I also noticed that the PyPi entry is somewhat outdated, for instance the badges for travis and codecov do not work probably due to outdated URL. metric-learn appears in the "My projects" section but the "manage" button is greyed out. Can someone give me the management rights and/or fix the small problems in the PyPi entry? Thanks!

@perimosocordiae @terrytangyuan @wdevazelhes can one of you check this? It looks like at least the CI badge urls need to be updated

@terrytangyuan
Copy link
Member

@bellet It seems like I cannot update the description manually. The issue is that our last release's README has the outdated URLs. The only solution would be release a new version. Also we need to make sure our README is correct and up-to-date before submitting release to PyPI next time.

@bellet
Copy link
Member Author

bellet commented Apr 3, 2020

OK, I did not realize this was directly rendering the README file!

@terrytangyuan
Copy link
Member

terrytangyuan commented Apr 3, 2020

@bellet Just added you as owner on PyPI in case I am away. Hope we'll all be safe around COVID-19 situation. Take care!

@bellet
Copy link
Member Author

bellet commented Apr 3, 2020

Thanks!

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.

4 participants