[MRG + 2] Removing generated C files, adding build cache to Travis #5492
Conversation
| @@ -189,8 +219,8 @@ def setup_package(): | |||
| **extra_setuptools_args) | |||
|
|
|||
| if (len(sys.argv) >= 2 | |||
| and ('--help' in sys.argv[1:] or sys.argv[1] | |||
| in ('--help-commands', 'egg_info', '--version', 'clean'))): | |||
| and ('--help' in sys.argv[1:] or sys.argv[1] | |||
There was a problem hiding this comment.
Pretty sure pep8 complains about this ;)
|
Looks like it's working |
arthurmensch
force-pushed
the
remove_cython
branch
from
5eaf430
to
dff6d77
Compare
| chmod +x miniconda.sh && ./miniconda.sh -b | ||
| export PATH=/home/travis/miniconda/bin:$PATH | ||
| conda update --yes conda | ||
| if [[ "$COVERAGE" == "true" ]]; then |
There was a problem hiding this comment.
why do we need this but not the other pip installs (nose, cython)
|
hm travis shows me nothing when I try to see the logs .. is that just me? |
arthurmensch
changed the title
|
Cythonization + cache is now working on my branch. I am not sure it is 100% reliable but it seems to do the job. Adding .cache/pip + anaconda folder to cached folder should improve performance as well (see #5455 ) |
|
It would be nice to have travis actually run. not sure that is feasible right now ^^ |
arthurmensch
changed the title
|
Working on my branch, I guess this need review. I go from 294 s to 104 s of install.sh with build cache. |
| import subprocess | ||
|
|
||
| HASH_FILE = 'cythonize.dat' | ||
| DEFAULT_ROOT = 'statsmodels' |
There was a problem hiding this comment.
This should change, and shouldn't we also include the their license ?
There was a problem hiding this comment.
They are cited at the beginning of the file
arthurmensch
force-pushed
the
remove_cython
branch
from
9f48e36
to
f6ea7f8
Compare
|
Can you use flake8 on these files as to clean the style issues? |
|
|
||
| elif [[ "$DISTRIB" == "ubuntu" ]]; then | ||
| # At the time of writing numpy 1.9.1 is included in the travis | ||
| # virtualenv but we want to used numpy installed through apt-get | ||
| # virtualenv but we want to used nNumpy installed through apt-get |
| @@ -13,6 +13,7 @@ dependencies: | |||
| - sudo apt-get install python-numpy python-scipy python-dev python-matplotlib | |||
| - sudo apt-get install python-nose python-coverage | |||
| - sudo apt-get install python-sphinx | |||
| - pip install --find-links http://wheels.astropy.org/ --find-links http://wheels2.astropy.org/ --use-wheel Cython | |||
There was a problem hiding this comment.
Sorry if this is obvious, but why are we depending on astropy's wheels to install Cython?
There was a problem hiding this comment.
That's not obvious, that's because they are the only ones available (they
do not exist in Pypi) - pandas use them as well. I should add a comment
there
Le 3 nov. 2015 18:32, "Manoj Kumar" notifications@github.com a écrit :
In circle.yml
#5492 (comment)
:@@ -13,6 +13,7 @@ dependencies:
- sudo apt-get install python-numpy python-scipy python-dev python-matplotlib
- sudo apt-get install python-nose python-coverage
- sudo apt-get install python-sphinx
- pip install --find-links http://wheels.astropy.org/ --find-links http://wheels2.astropy.org/ --use-wheel Cython
Sorry if this is obvious, but why are we depending on astropy's wheels to
install Cython?—
Reply to this email directly or view it on GitHub
https://github.com/scikit-learn/scikit-learn/pull/5492/files#r43779221.
There was a problem hiding this comment.
I don't understand. It's right here https://pypi.python.org/pypi/Cython/ so why not just pip install the latest version?
Also out of the two links, one has the Cython version 0.19.1 and the other link raises a 404 error.
(Sorry, I don't know the internals of these well.)
There was a problem hiding this comment.
We want to use wheels for alleviating computation
Le 3 nov. 2015 19:48, "Manoj Kumar" notifications@github.com a écrit :
In circle.yml
#5492 (comment)
:@@ -13,6 +13,7 @@ dependencies:
- sudo apt-get install python-numpy python-scipy python-dev python-matplotlib
- sudo apt-get install python-nose python-coverage
- sudo apt-get install python-sphinx
- pip install --find-links http://wheels.astropy.org/ --find-links http://wheels2.astropy.org/ --use-wheel Cython
I don't understand. It's right here https://pypi.python.org/pypi/Cython/
so why not just pip install the latest version?Also out of the two links, one has the Cython version 0.19.1 and the other
link raises a 404 error.—
Reply to this email directly or view it on GitHub
https://github.com/scikit-learn/scikit-learn/pull/5492/files#r43788755.
There was a problem hiding this comment.
But this is linux, right? There are no binary wheels, are there?
|
I think we can just use |
|
Travis is green and cache is working on all builds -- I'd like to check appveyor though . The only glitch is that on vanilla ubuntu, installing cython through pip takes a little time, but I don't think it is worth fighting for ~15s. To install a non dev version, cython is not necessary as .c files are provided in the sdist package. So I will just update the developer section. |
arthurmensch
force-pushed
the
remove_cython
branch
from
a9140bf
to
986fb60
Compare
|
Maybe I'm stupid but why does pip installing take longer than wheel installing on linux? |
|
With this PR you'll be the king of "removed lines" ^^ |
|
LGTM ! |
MechCoder
changed the title
|
Build failed on appveyor. The fail doesn't look completely spurious to me. |
|
|
|
That's strange. Would be great if someone could reproduce in a Windows environ |
|
hm sorry stopped the build |
|
Sorry appveyor was failing because I did not register _build_utils submodule in |
|
@amueller brave enough to merge? |
|
Merging, since there are 3 +1's :) |
[MRG + 2] Removing generated C files, adding build cache to Travis
|
Thanks ! @arthurmensch :-) |
|
On a lighter note,
|
|
lol! |
|
Great!! thanks @arthurmensch ! |
|
awesome, thanks so much for working on this! |
pip installs would fail when compiling against ATLAS. This is due to a distutils-setuptools issue with monkeypatching that is triggered if Cython is imported. See numpy/numpy#7235 for details.
pip installs would fail when compiling against ATLAS. This is due to a distutils-setuptools issue with monkeypatching that is triggered if Cython is imported. See numpy/numpy#7235 for details.
pip installs would fail when compiling against ATLAS. This is due to a distutils-setuptools issue with monkeypatching that is triggered if Cython is imported. See numpy/numpy#7235 for details.
Still to do :