Improvement: Add doc notes on setting up the build environment by hmaarrfk · Pull Request #3472 · scikit-image/scikit-image

hmaarrfk · 2018-10-15T04:05:13Z

For reviewers

(Don't remove the checklist below.)

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

jni

I've made a couple of fixes, but 👍 !

Probably we should wait for #3469 before merging this though. =)

hmaarrfk · 2018-10-15T11:51:46Z

why did all the builds fail?

jni · 2018-10-15T13:43:59Z

Hmmm I saw the same failure in #3469. skimage.lookfor or np.lookfor is broken. But there hasn't been a new NumPy release for weeks.

=================================== FAILURES ===================================
____________________ [doctest] skimage.util.lookfor.lookfor ____________________
007 
008     Parameters
009     ----------
010     what : str
011         Words to look for.
012 
013     Examples
014     --------
015     >>> import skimage
016     >>> skimage.lookfor('regular_grid')
UNEXPECTED EXCEPTION: TypeError("'NoneType' object is not iterable",)
Traceback (most recent call last):
  File "/opt/python/3.6.3/lib/python3.6/doctest.py", line 1330, in __run
    compileflags, 1), test.globs)
  File "<doctest skimage.util.lookfor.lookfor[1]>", line 1, in <module>
  File "/home/travis/build/scikit-image/scikit-image/skimage/util/lookfor.py", line 24, in lookfor
    return np.lookfor(what, sys.modules[__name__.split('.')[0]])
  File "/home/travis/venv/lib/python3.6/site-packages/numpy/lib/utils.py", line 754, in lookfor
    cache = _lookfor_generate_cache(module, import_modules, regenerate)
  File "/home/travis/venv/lib/python3.6/site-packages/numpy/lib/utils.py", line 906, in _lookfor_generate_cache
    for pth in item.__path__:
TypeError: 'NoneType' object is not iterable

sciunto · 2018-10-15T13:44:11Z

CONTRIBUTING.txt

+  source env/bin/activate
+  pip install -r requirements/default.txt
+  pip install -r requirements/build.txt
+  pip install -r requirements/test.txt


How about the documentation? We insist that all new feature must have an example, so it's important to consider it as part of the build env. setup.

My thoughts are that new devs don't necessary want to run all the examples. They will likely just install the deps they need for the examples they are developing. I'm trying to help people to push PR for the first time, not necessarily make the best example right off the bat.

We might also need:

Beginners contribution guide

Advanced contribution guide

My thought is that some new devs start with a new feature as a first PR. It not negligible in scikit-image.

Alright. Do you think it is appropriate to bundle it all in the main requirements.txt file? having 4 commands is a little annoying.

Sadly not: I don't think we want requirements.txt to contain everything, since most people look there for "minimal requirements". I just found this workaround:

pip install -r <(cat requirements/*.txt)

pypa/pip#3880

Be sure to add a comment to that line to the effect of, "install all development, runtime, and documentation requirements of scikit-image."

Any workaround for conda? I don't want to maintain a yaml file...

jni · 2018-10-15T14:22:30Z

Re builds... Could it be something changed in Cython? New version released 22h ago. BUT the failure happens on my 28.2 version... But it's with a .so file:

 $ pytest --pdb --doctest-modules skimage/util/lookfor.py 
==================================================================================================================== test session starts =====================================================================================================================
platform linux -- Python 3.6.3, pytest-3.5.1, py-1.5.3, pluggy-0.6.0
rootdir: /home/jni/projects/scikit-image/skimage, inifile:
plugins: cov-2.5.1, hypothesis-3.56.9
collected 1 item                                                                                                                                                                                                                                             

skimage/util/lookfor.py F
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
007 
008     Parameters
009     ----------
010     what : str
011         Words to look for.
012 
013     Examples
014     --------
015     >>> import skimage
016     >>> skimage.lookfor('regular_grid')
UNEXPECTED EXCEPTION: TypeError("'NoneType' object is not iterable",)
Traceback (most recent call last):

  File "/home/jni/miniconda3/envs/cf/lib/python3.6/doctest.py", line 1330, in __run
    compileflags, 1), test.globs)

  File "<doctest skimage.util.lookfor.lookfor[1]>", line 1, in <module>

  File "/home/jni/projects/scikit-image/skimage/util/lookfor.py", line 24, in lookfor
    return np.lookfor(what, sys.modules[__name__.split('.')[0]])

  File "/home/jni/miniconda3/envs/cf/lib/python3.6/site-packages/numpy/lib/utils.py", line 754, in lookfor
    cache = _lookfor_generate_cache(module, import_modules, regenerate)

  File "/home/jni/miniconda3/envs/cf/lib/python3.6/site-packages/numpy/lib/utils.py", line 906, in _lookfor_generate_cache
    for pth in item.__path__:

TypeError: 'NoneType' object is not iterable

/home/jni/projects/scikit-image/skimage/util/lookfor.py:16: UnexpectedException
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> /home/jni/miniconda3/envs/cf/lib/python3.6/site-packages/numpy/lib/utils.py(906)_lookfor_generate_cache()
-> for pth in item.__path__:
(Pdb) p item
<module 'skimage._shared.geometry' from '/home/jni/projects/scikit-image/skimage/_shared/geometry.cpython-36m-x86_64-linux-gnu.so'>
(Pdb) p item.__dir__()
['__name__', '__doc__', '__package__', '__loader__', '__spec__', '__file__', '__path__', '__builtins__', '__pyx_capi__', '__test__']
(Pdb) item.__file__
'/home/jni/projects/scikit-image/skimage/_shared/geometry.cpython-36m-x86_64-linux-gnu.so'
(Pdb) list
901  	            except AttributeError:
902  	                _all = None
903  	
904  	            # import sub-packages
905  	            if import_modules and hasattr(item, '__path__'):
906  ->	                for pth in item.__path__:
907  	                    for mod_path in os.listdir(pth):
908  	                        this_py = os.path.join(pth, mod_path)
909  	                        init_py = os.path.join(pth, mod_path, '__init__.py')
910  	                        if (os.path.isfile(this_py) and
911  	                                mod_path.endswith('.py')):

(And it's on #3469)

Ok going to bed now, more in the morning I guess... =\

hmaarrfk · 2018-10-15T14:24:09Z

Yeah, I pinned it to an so file as well. Seems like it could be cython stripping some info? We didn't add the binary stripping patch.

sciunto · 2018-10-20T09:24:40Z

@hmaarrfk Could you rebase please? :)

stefanv · 2018-10-21T00:49:03Z

Unless a rebase is difficult due to conflict, I strongly suggest we rebase on contributors' behalves. The commit-review-commit cycle is slow enough as it is!

hmaarrfk · 2018-10-21T01:08:13Z

Cython tips were moved to: #3253

@stefanv I think it depends on how much we need the PR. I wanted tow remove the cython tips before rebasing so it was going to take longer.

Cython tips have been moved to: #3253

CONTRIBUTING.txt

stefanv · 2018-10-22T20:22:55Z

CONTRIBUTING.txt

+
+pip+virtualenv
+==============
+If you choose to use pip and virtualenv, then you may find the following bash


Typo in this sentence, and would remove "if you choose" part.

stefanv · 2018-10-22T20:23:08Z

CONTRIBUTING.txt

+If you choose to use pip and virtualenv, then you may find the following bash
+commands::
+
+  virtualenv env


Should probably be python -m venv skimage-dev

stefanv · 2018-10-22T20:23:50Z

CONTRIBUTING.txt

+  source env/bin/activate
+  # Install all development and runtime dependencies of scikit-image
+  pip install -r <(cat requirements/*.txt)
+


Add some text that explains that the Python installation lives in the directory env (or, skimage-dev, as suggested above).

stefanv · 2018-10-22T20:24:14Z

CONTRIBUTING.txt

+conda
+=====
+
+If you prefer to use conda (Anaconda or Miniconda) for Python environment


Move this text to the top, and explain that you may use either way to deploy a dev env.

stefanv · 2018-10-22T20:24:37Z

CONTRIBUTING.txt

+If you prefer to use conda (Anaconda or Miniconda) for Python environment
+management, then you may find the following bash commands useful::
+
+  conda create --name skdev python=3.6


Use the same environment name.

stefanv · 2018-10-22T20:25:09Z

CONTRIBUTING.txt

+  # Install all development and runtime dependencies of scikit-image
+  conda install `for i in requirements/{default,build,test,docs}.txt; do echo -n " --file $i "; done`
+
+You can choose a different name or a different Python version,


This is vague.

stefanv · 2018-10-22T20:25:23Z

CONTRIBUTING.txt

+
+You can choose a different name or a different Python version,
+depending on your preferences.
+If you are using the conda-forge channel and NumPy seems to be changing


This is also too vague.

Removed, people can searech on conda-forge to find out how to solve this issue themselves.

The other PR to remove requirements-parser was to ensure that it could build from anaconda defaults as well. So we are good on that front :D.

The documentation dependencies aren't available on anaconda defaults.

stefanv · 2018-10-22T20:25:47Z

CONTRIBUTING.txt

+Once you have installed all dependencies, you can install the development
+version of scikit-image using the command::
+
+  pip install -e . --no-deps


Is the --no-deps required?

It is for conda I think

jni · 2019-01-15T09:57:27Z

@scikit-image/core this one looks ready to go for me. It was held up by CI but @hmaarrfk rebased, everything is green, and it's an easy win at this point!

CONTRIBUTING.txt

Co-Authored-By: hmaarrfk <mark.harfouche@gmail.com>

soupault · 2019-02-09T01:16:52Z

CONTRIBUTING.txt

+  # Activate it
+  conda activate skimage-dev
+  # Install all development and runtime dependencies of scikit-image
+  conda install `for i in requirements/{default,build}.txt; do echo -n " --file $i "; done`


It will, probably, be better to harmonize the dependency installation instructions for pip and conda sections (i.e. consider either all or (default,build) req-files).

everything installs on conda-forge, but not on defaults. Many people aren't on conda forge, so I feel like this is a just middle to hit, without asking people to go bleeding edge (on conda-forge) and messing the rest of their conda config.

Maybe give a hint then that these dependency lists are not actually complete? E.g., change:

# Install all development and runtime dependencies of scikit-image

to

# Install major development and runtime dependencies of scikit-image # (the rest can be installed from conda-forge or pip, if needed)

Thanks for the suggestion!

soupault · 2019-02-09T19:08:27Z

Awesome! Thanks Mark!

hmaarrfk · 2019-02-09T20:26:14Z

There are a few things left to consolidate the documentation regarding setting up the build env, but yeah.

jni approved these changes Oct 15, 2018

View reviewed changes

sciunto reviewed Oct 15, 2018

View reviewed changes

hmaarrfk mentioned this pull request Oct 15, 2018

lookfor is broken with cython == 0.29.0 #3475

Closed

sciunto added 📄 type: Documentation Updates, fixes and additions to documentation 🧐 Needs review labels Oct 20, 2018

hmaarrfk force-pushed the doc_contributing branch 2 times, most recently from 99e7568 to b8707ca Compare October 21, 2018 01:07

hmaarrfk changed the title ~~DOC: Add some short docs about setting up the build environment and Cython~~ DOC: Add some short docs about setting up the build environment Oct 21, 2018

stefanv reviewed Oct 22, 2018

View reviewed changes

DOC: Add some instructions about the build environment setup.

7942939

hmaarrfk force-pushed the doc_contributing branch from 3b3650f to 7942939 Compare November 3, 2018 16:22

soupault reviewed Jan 15, 2019

View reviewed changes

CONTRIBUTING.txt Outdated Show resolved Hide resolved

CONTRIBUTING.txt Outdated Show resolved Hide resolved

CONTRIBUTING.txt Outdated Show resolved Hide resolved

CONTRIBUTING.txt Outdated Show resolved Hide resolved

soupault and others added 4 commits January 15, 2019 07:53

Update CONTRIBUTING.txt

0eec948

Co-Authored-By: hmaarrfk <mark.harfouche@gmail.com>

Update CONTRIBUTING.txt

da037bc

Co-Authored-By: hmaarrfk <mark.harfouche@gmail.com>

Update CONTRIBUTING.txt

17609bd

Co-Authored-By: hmaarrfk <mark.harfouche@gmail.com>

Update CONTRIBUTING.txt

ae291a0

Co-Authored-By: hmaarrfk <mark.harfouche@gmail.com>

soupault reviewed Feb 9, 2019

View reviewed changes

soupault added this to the 0.15 milestone Feb 9, 2019

soupault self-assigned this Feb 9, 2019

soupault added action: mrg+1 and removed 🧐 Needs review labels Feb 9, 2019

Hinted where the remaining deps could be obtained for conda

238f9b8

soupault changed the title ~~DOC: Add some short docs about setting up the build environment~~ DOC: Add notes on setting up the build environment Feb 9, 2019

soupault merged commit 64103e6 into scikit-image:master Feb 9, 2019

hmaarrfk deleted the doc_contributing branch February 9, 2019 20:25

hmaarrfk changed the title ~~DOC: Add notes on setting up the build environment~~ Improvement: Add doc notes on setting up the build environment Mar 4, 2019

Uh oh!

Conversation

hmaarrfk commented Oct 15, 2018

For reviewers

Uh oh!

jni left a comment

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Oct 15, 2018

Uh oh!

jni commented Oct 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jni commented Oct 15, 2018

Uh oh!

hmaarrfk commented Oct 15, 2018

Uh oh!

sciunto commented Oct 20, 2018

Uh oh!

stefanv commented Oct 21, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmaarrfk commented Oct 21, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jni commented Jan 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soupault commented Feb 9, 2019

Uh oh!

stefanv commented Oct 21, 2018 via email •

edited

Loading