Making ramp_workflow pip installable #50

aboucaud · 2017-09-15T18:55:20Z

This PR intends to clean up the dependency tree of ramp_workflow.

The idea is to trim off all dependencies related to a particular ramp-kit.

The ramp-kits will therefore need to have a specific requirement.txt file listing the dependencies, which will be installed then using pip or conda.

jorisvandenbossche

Ah, I was also working on this (I thought I had mentioned that), but did not yet do the requirements parsing of starting kits, so this is better

jorisvandenbossche · 2017-09-15T19:41:52Z

.travis.yml

-    - source install_requirements.sh
-    - pip install codecov
-    - pip install -q flake8
+    - pip install -r testing-requirements.txt


since we are using conda envs, let's install with conda here?

What is the command ? I don't use conda..

ok, bad idea. If a single library is not installable by conda the whole thing crashes.

Maybe use an environement.yml file then (example here) that can be used to update an existing conda environment and can specify both conda and pip packages ?

Also as a side note, I find https://github.com/astropy/ci-helpers very useful for setting up continuous integration.

Which one is not installable? All in that list are for me installable with conda

Ah, but that is probably because I have conda-forge channel added. OK, then leave it be.

Regarding jupyter, what is this used for? Is it only for nbconvert? (then can also only install that)

@rth How do you use an environment.yml to specify pip packages ?

IMO ci-helpers is quite a big machinery for pure python packages. However we may use it in the future, especially for the very simple way to define build matrices.

Yes, jupyter is only for nbconvert, I only call it in the backend to generate the page on ramp.studio. So it should be tested in the travis of the kit, but not in rampwf.

@aboucaud Well one could do, conda env create [or update] -f environement.yml where, environement.yml looks like, e.g.,

name: test-env channels: - defaults - conda-forge dependencies: - python=3.5 - numpy=1.13.1 - scipy=0.19.1 - conda_package_1 - conda_package_2 - pip : - pip_package_1 - ./ # install the current package

but in the end it's not that different from just running conda install and pip install separately..

Seems convenient indeed. I'll keep that in mind.

jorisvandenbossche · 2017-09-15T19:42:00Z

setup.py

+            'Operating System :: MacOS'],
+        install_requires=[
+            'numpy>=1.13',
+            'scipy>=0.19',


scipy is not needed I think?

indeed, but scikit-learn expects it and it is not installed as dependency

Maybe better to read from requrements.txt here? If you install scikit-learn with conda, scipy will get installed... Also, joblib is imported but is not in the requirements...

I agree its a duplication. But I don't know what is best regarding pypi.

Concerning joblib, I moved the imports to get rid of the dependency.

let's leave them separate, the small duplication is not that bad. There is some slight ideological difference between requirements.txt (deploy specific versions) and install_requires (specify minimum dependencies the deployed environment should adhere to). See https://packaging.python.org/discussions/install-requires-vs-requirements/

jorisvandenbossche · 2017-09-15T19:55:19Z

setup.py

+        install_requires=[
+            'numpy>=1.13',
+            'scipy>=0.19',
+            'scikit-learn>=0.18'],


although pandas is not a strict requirement (it is currently not directly imported), it is still used a lot in the code (code that assumes pandas data structures are used), so therefore would add it here

fair enough

what should be the version required ?

Hmm, not sure. Let's maybe take 0.19 for now ?
(actually, we should in principle also test for all minimal dependencies, but can be for other PR)

jorisvandenbossche · 2017-09-15T20:01:02Z

I also think it might be better to split the requirements parsing into another PR, so the actual setup.py updates can already be merged (or, for now still install some of the extra deps in travis.yml until all starting_kits have gotten a requirements file)

aboucaud · 2017-09-15T20:04:08Z

I pushed a requirements.txt in every starting kit tested so far

aboucaud · 2017-09-15T21:09:44Z

Only the python2 build fails because of the following import error

from tensorflow.core.framework.graph_pb2 import *
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
    import sys
    _b=sys.version_info[0]<3 and (lambda x:x) or (lambda x:x.encode('latin1'))
>   from google.protobuf import descriptor as _descriptor
E   ImportError: No module named google.protobuf

/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/tensorflow/core/framework/graph_pb2.py:6: ImportError

This is specific to tensorflow and could maybe be solved easily by installing the protobuf lib, but since the former builds seems to pass and you did not have that dependency, I find it weird.

Any idea @kegl ?

kegl · 2017-09-15T21:46:48Z

Have never seen it. Pinging @mehdidc :)

codecov · 2017-09-15T23:12:56Z

Codecov Report

Merging #50 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   92.13%   92.14%   +0.01%     
==========================================
  Files          51       51              
  Lines        1195     1197       +2     
==========================================
+ Hits         1101     1103       +2     
  Misses         94       94

Impacted Files	Coverage Δ
rampwf/utils/testing.py	`100% <ø> (ø)`	⬆️
rampwf/kits/ramp_kit.py	`95.23% <100%> (+0.79%)`	⬆️
rampwf/workflows/image_classifier.py	`75.24% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b57cbf2...bddbf52. Read the comment docs.

jorisvandenbossche · 2017-09-16T11:04:37Z

This is specific to tensorflow and could maybe be solved easily by installing the protobuf lib, but since the former builds seems to pass and you did not have that dependency, I find it weird.

Strange thing is that protobuf is actually installed in the failing travis build: https://travis-ci.org/paris-saclay-cds/ramp-workflow/jobs/276098845#L1314.

aboucaud · 2017-09-16T12:57:03Z

Mmmmh I find several complains online regarding protobuf and tensorflow (like this one) but only fixups, no actual recipe to make it work every time.

Both libraries versions seem to be tightly linked so what I can think of is:

protobuf released a new version 14h ago (see here) so it could just be bad luck that a build or a wheel was updated for py3.x but not py2.7 on pypi => the build may pass or fail erratically
we should (as was said earlier) specify versions for these particular packages. Especially the ones with incompatible API's (like keras of tf). @mehdidc & @kegl, what versions do you usually run MNIST with ?

mehdidc · 2017-09-16T13:01:30Z

@aboucaud in pollenating insects 3, we use tensorflow 1.3.0 and protobuf 3.4.0

aboucaud · 2017-09-18T09:58:24Z

The MNIST ramp-kit build and the ramp-workflow build are identical but one fails and the other passes..

Now I'm really confused.. 😩

jorisvandenbossche · 2017-09-18T11:17:41Z

Maybe try to list protobuf explicitly in the requirements for MNIST ? (don't know why that would matter, but in all the issues/questions about this issue online I saw a lot of 'solution' (like just reinstalling) for which I don't know why it would matter ...)

aboucaud · 2017-09-19T09:43:01Z

At this point I really don't understand the difference between the two builds (MNIST alone or called by the ramp-workflow tests) on python2.7
Pinging @glemaitre @agramfort for new insights..

jorisvandenbossche · 2017-09-19T09:53:57Z

Comaparing the two last builds, there is a difference in the protobuf version (3.3 vs 3.4)

jorisvandenbossche · 2017-09-19T09:56:30Z

Also, to really see whether the same works on MNIST, I think we need to adapt the travis.yml file on MNIST to use the requirements.txt file

aboucaud · 2017-09-19T09:58:52Z

Comaparing the two last builds, there is a difference in the protobuf version (3.3 vs 3.4)

I tried this change this morning to see if that specific version was troubling. Apparently not.

Also, to really see whether the same works on MNIST, I think we need to adapt the travis.yml file on MNIST to use the requirements.txt file

I agree that's what we should do, but it also means taking care of installing the proper requirements for ramp-workflow as long as this PR is not merged.

jorisvandenbossche · 2017-09-19T10:01:09Z

Also, to really see whether the same works on MNIST, I think we need to adapt the travis.yml file on MNIST to use the requirements.txt file

Trying that in ramp-kits/MNIST#3

jorisvandenbossche · 2017-09-19T11:57:51Z

@aboucaud you can remove that last commit again. I just tried to make it even more comparable to how the env is set up in the MNIST repo, but also this attempt still fails with the same problem.

aboucaud added 9 commits September 15, 2017 20:09

Put test libraries dependencies in travis

a39a624

Move skimage and joblib imports to remove dependency

bb4cd89

Remove unused dependency

7645cd6

Remove unimported libraries

597b8e1

Move gitpython and jupyter dependency to travis (test only)

34861f7

Add hard requirements to setup.py

1b33703

Add pip requirement files and clean up travis

f5e8be2

Add scipy and change version

9aa24db

Add automatic pip installation of ramp-kits dependencies

ad1b06a

jorisvandenbossche reviewed Sep 15, 2017

View reviewed changes

Add pandas as hard requirement

bf1505e

aboucaud force-pushed the prep-pip-install branch from 2602163 to bf1505e Compare September 15, 2017 20:22

Replace jupyter by nbconvert to limit imports

c340de3

jorisvandenbossche mentioned this pull request Sep 18, 2017

Pip installable #51

Closed

Move MNIST kit first in the test list

5307547

jorisvandenbossche mentioned this pull request Sep 19, 2017

Pip install ramp-workflow and use requirements.txt file for local deps ramp-kits/MNIST#3

Merged

aboucaud force-pushed the prep-pip-install branch from 7cfdd0d to 5307547 Compare September 19, 2017 10:01

temp: try conda

a085687

aboucaud and others added 5 commits September 20, 2017 11:22

Add tensorflow to the travis file

4e7caca

Merge branch 'master' into prep-pip-install

29bd4c0

Add ipython kernel specification

9c13dd8

Restore coverage in travis

9af7399

restore codecov

bddbf52

jorisvandenbossche merged commit b97dd21 into master Sep 20, 2017

aboucaud mentioned this pull request Sep 25, 2017

requirements.txt in starting kits to specify dependencies #25

Closed

aboucaud deleted the prep-pip-install branch October 31, 2017 17:56

Making ramp_workflow pip installable #50

Making ramp_workflow pip installable #50

Conversation

aboucaud commented Sep 15, 2017

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rth Sep 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Sep 15, 2017

aboucaud commented Sep 15, 2017

aboucaud commented Sep 15, 2017

kegl commented Sep 15, 2017

codecov bot commented Sep 15, 2017 • edited

Codecov Report

jorisvandenbossche commented Sep 16, 2017

aboucaud commented Sep 16, 2017 • edited

mehdidc commented Sep 16, 2017 • edited

aboucaud commented Sep 18, 2017

jorisvandenbossche commented Sep 18, 2017

aboucaud commented Sep 19, 2017

jorisvandenbossche commented Sep 19, 2017

jorisvandenbossche commented Sep 19, 2017

aboucaud commented Sep 19, 2017 • edited

jorisvandenbossche commented Sep 19, 2017

jorisvandenbossche commented Sep 19, 2017

rth Sep 16, 2017 •

edited

codecov bot commented Sep 15, 2017 •

edited

aboucaud commented Sep 16, 2017 •

edited

mehdidc commented Sep 16, 2017 •

edited

aboucaud commented Sep 19, 2017 •

edited