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

Pip install, basic support #597

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

JeanPaulShapo
Copy link
Contributor

Right now we have test package on https://testpypi.python.org/pypi.
You can try to install package using typing pip install -i https://testpypi.python.org/pypi.
However, you still must install all dependencies except git (python, cmake, boost etc.)
Right now it doesn't work on Windows.
On my machine with Fedora 22 installation works, but I'd like to receive comments from users with other Linux distributions and Mac OS'es.
Feel free to write about any problems encountered during testing.

@sashafrey
Copy link
Contributor

@JeanPaulShapo I don't have linux machine at hand to test this.. Will try to get a clean one on AWS EC2 and play with this over the weekend.

Could you split this pull request into two parts -- the one about 3rdparties removal, and the rest? Currently it is hard to review, because so many files had been deleted.

The part about 3rdparty removal you may just go ahead and merge into master (as long as travis-ci is happy :))

@JeanPaulShapo
Copy link
Contributor Author

@sashafrey Ok, i'll split this PR.

@sashafrey
Copy link
Contributor

@JeanPaulShapo If it takes long time than I'll just review your commits one by one... So just let me, don't waste too much time.

To me it would be really great to have a clean commit (or a clean pull request) that contains everything needed for pypi package. I'll learn a lot from this myself --- I've never did it before, and it is very very nice to know how to create those pypi packages! :)

@JeanPaulShapo
Copy link
Contributor Author

@sashafrey So, it'll be helpful to get from this PR part related to CMake fixes and enhancement.
I'll do it ASAP.

@sashafrey
Copy link
Contributor

@JeanPaulShapo Thanks!

@JeanPaulShapo
Copy link
Contributor Author

I've got rid of "Removing 3rd-party" part.
Currently this PR consists of CMakeLists enhancements and pip install feature using some of them.

@sashafrey
Copy link
Contributor

@JeanPaulShapo I've tried pip install bigartm on clean Ubuntu machine, and it worked very well! I'm able to run python, import artm, and also build models. So at least for ubuntu I'm a happy user.

Here is my full story:

  1. Some prerequisites I decided to install myself even before trying pip install
sudo apt-get install git make cmake build-essential libboost-all-dev
  1. My first try failed because pip install by default refuses to install a pre-release version
pip install -i https://testpypi.python.org/pypi bigartm
  1. My second try was with --pre added to the command. It seems to work, but I didn't like that it starts building numpy and pandas... and on Ubuntu I know a better way of doing this via sudo apt-get. So I run this:
sudo apt-get install python-numpy python-pandas

and then retry previous command. This time it recognized numpy and pandas are in place. The command started to build bigartm, which went very good! However almost at the end it failed:

pip install -i https://testpypi.python.org/pypi bigartm
...
error: could not create '/usr/local/lib/python2.7/dist-packages/artm': Permission denied
  1. Then I added "sudo", and vuala - it worked:
sudo pip install -i https://testpypi.python.org/pypi bigartm --pre
Successfully installed bigartm 

@sashafrey
Copy link
Contributor

sashafrey commented Jun 18, 2016

There is also one minor change that I did to build this change on Windows: 1d9bca0
Without this it fails because Windows doesn't recognize cp. Changing it to copy didn't help... and also I don't understand why artm.dll needs to be copied into /python/artm/wrapper/. Is it OK to exclude this step from Windows?

@JeanPaulShapo
Copy link
Contributor Author

@sashafrey I answered to your last question in comment to PR #601.
As for trying pip install you must always do it with sudo if you try to install to system-wide directory
(for virtualenv environment there is no need in sudo).

@sashafrey
Copy link
Contributor

@JeanPaulShapo cmake -E copy works well on my windows machine, thanks!

@sashafrey
Copy link
Contributor

@JeanPaulShapo I think we should merge this PR. It enables pip install on Linux, and it doesn't break build -- both Travis and my Windows machines look well.

@JeanPaulShapo
Copy link
Contributor Author

@sashafrey Ideally I'd like to hear some comment from Mac OS users, that's why I ask @MelLain to test this PR.

@sashafrey
Copy link
Contributor

@JeanPaulShapo Ok, I agree. The plan is to make a new v0.8.1 release tomorrow, as requested by @MelLain . At the same time I'll bring stable branch forward. So perhaps it's better for this PR to be included in v0.8.2.

I did rebase this branch (JeanPaulShapo/pip_install) on top of bigartm/master, and the merge is very simple. There is just one conflict in python/artm/wrapper/api.py. @MelLain believes that the name of the binary for OS X should be libartm.dylib, while your change suggests artm.dylib.

@JeanPaulShapo
Copy link
Contributor Author

JeanPaulShapo commented Jun 18, 2016

@sashafrey As for libartm.dylib - I've already fixed this problem.
As for release - OK, no problem :)

@ofrei
Copy link
Contributor

ofrei commented Feb 14, 2018

Keep this PR open, despite the fact that it is very old. This PR it is related to pip install feature - which has highest priority among all users requests to BigARTM.

bt2901 added a commit to bt2901/bigartm that referenced this pull request Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants