Optional pooch dependency #4666

hmaarrfk · 2020-05-09T19:42:25Z

Description

Makes pooch an optional dependency. This was at least in my original design plan. Not sure why it never got in, I'll sift through the comments again.

TODO: Pooch contribution guide.

When merged backport with:

@meeseeksdev backport to v0.17.x

Note that this does NOT address #4660 or #4664

Closes #4665

Checklist

Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests
Clean style in the spirit of PEP8

For reviewers

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.

This reverts commit 137f144.

hmaarrfk · 2020-05-09T20:50:32Z

@jni was there a reason why we decided to make pooch a required dependency?

8e08610

I really don't remember.

pep8speaks · 2020-05-09T23:41:00Z

Hello @hmaarrfk! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file skimage/data/__init__.py:

Line 143:5: E303 too many blank lines (2)

Comment last updated at 2020-05-10 22:58:31 UTC

jni

@hmaarrfk I've suggested some typo fixes and some minor rephrasing. I also suggest keeping some of the tests I added recently, which still make sense even with pooch being optional. Otherwise this is good to go, approving so I don't hold this up for another round.

jni · 2020-05-10T07:55:44Z

skimage/data/__init__.py

+            "The requested file is part of the scikit-image distribution, "
+            "but requries the installation of an optional dependency, pooch, "
+            "to be used. To install pooch, use your preffered python "
+            "package manager. Follow installation instruction found at "
+            "https://scikit-image.org/docs/stable/install.html"


Suggested change

"The requested file is part of the scikit-image distribution, "

"but requries the installation of an optional dependency, pooch, "

"to be used. To install pooch, use your preffered python "

"package manager. Follow installation instruction found at "

"https://scikit-image.org/docs/stable/install.html"

"The requested file is part of the scikit-image distribution, "

"but requires the installation of an optional dependency, pooch. "

"To install pooch, use your preferred python package manager. "

"For example: `conda install -c conda-forge pooch`, or "

"`pip install -m pooch`."

I prefer not pointing to our instructions since they don't have any pooch-specific documentation. I would find this confusing.

Can we think a little longer term about this? We can work on adding instructions, lets make sure that we are doing good by future generations of developers.

I see two kinds of people:

Advanced users: will use shortcuts to get to google, and to search pooch python which will give them a package, that they will know to use conda/pip automatically. These don't need any instructions.

Novice users, that will be tempted to use sudo pip install pooch.

I am trying to help the novice users by pointing them to more fleshed out instructions than a one liner (that will likely not work and ensure their frustration).

Reading over this, I strongly disagree with the instructions provided.

For example, I strongly discourage

conda install -c conda-forge pooch

I really think there should be a single place where we provide install instructions, and not adhoc like an Error message.

I raised my concerns in:
#4672

jni · 2020-05-10T07:56:34Z

skimage/data/tests/test_data.py

-
-
-@skipif(OFFLINE, reason='No internet connection')
-def test_download_all():


Is there a good reason to remove this test?

This test really doen't play with parallel testing.... affecting the global filesystem state is not fun.

it also may induce a full download of all the files for the end users. not fun on our laptops when travelling with 4G connections.

jni · 2020-05-10T07:57:04Z

skimage/data/tests/test_data.py

 def test_cells_3d():
    """Needs internet connection."""
    path = fetch('data/cells.tif')
    image = io.imread(path)
    assert image.shape == (60, 256, 256)
-
-
-def test_data_dir():


Why remove this test? I feel like it's important.

Thanks, I added it back.

jni · 2020-05-10T07:59:45Z

@hmaarrfk it looks like I made that decision unilaterally and explicitly labeled it as such in my PR #4483, but that comment got lost in the rebase/cherry-pick dance. So it was never discussed properly. I'm happy for this PR to go in. =) Thank you!

jni · 2020-05-10T10:59:48Z

@hmaarrfk feel free to revert the last commit, I'm not super happy with it, but I think we should be able to make the initialisation lazy, and just fall back on our legacy dir in the meantime.

hmaarrfk · 2020-05-10T13:31:56Z

I wanted to leave the laziness to an other PR for now. Seems hard to get right

hmaarrfk · 2020-05-10T13:32:09Z

Super thanks for the rest!

skimage/data/__init__.py

emmanuelle

Thanks for the PR @hmaarrfk I left a tiny comment about one error message which was not completely clear to me. @jni you can merge in the Australian morning :-).

emmanuelle · 2020-05-10T21:55:22Z

One question related to pooch: I don't remember why we are deprecating data.load()? Because for image data which don't have their own function, like the cells.tif file, using data.load() will be the only way to get this image? The deprecation message mentions using skimage.io.load but this function does not exist. Am I missing something?

Co-authored-by: Emmanuelle Gouillart <emma@plot.ly>

hmaarrfk · 2020-05-10T23:00:40Z

I'm not too sure I remember why we deprecated load. I think the feeling was that it was a thin wrapper on imread and we preferred people not use it directly.

Having to use cell() will give you back the data, prelaoded, instead of asing you to read it again yourself.

For certain datasets, they are more complicated than "just a single image" so having a function that could return more flexible formats, like tuples was preferred.

jni · 2020-05-11T08:45:41Z

@emmanuelle well I think the "new way" is supposed to be something like:

data.download_all()
cells = io.imread(os.path.join(data.data_dir, 'data/cells.tif'))

which is nice because it looks more like normal IO than data.function(). Having said that, we should provide a public way to fetch individual images.

emmanuelle · 2020-05-11T17:01:55Z

@meeseeksdev backport to v0.17.x

lumberbot-app · 2020-05-11T17:02:02Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

$ git checkout v0.17.x
$ git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 14b521af8172ef3fad8dec289a796de004695f14

You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #4666: Optional pooch dependency'

Push to a named branch :

git push YOURFORK v0.17.x:auto-backport-of-pr-4666-on-v0.17.x

Create a PR against branch v0.17.x, I would have named this PR:

"Backport PR #4666 on branch v0.17.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

emmanuelle · 2020-05-11T17:04:47Z

@jni thanks for the explanations. Yes, I agree that we should also have a way to download and open individual files. Probably not high priority since we indeed have download_all (which at the moment is still reasonable).

hmaarrfk · 2020-05-11T18:07:41Z

i guess the easy way didn't work.

* Revert "Forget legacy data dir (#4662)" This reverts commit 137f144. * Make pooch an optional requirement * Make pooch an optional dependency * Add Pooch to documentation requirements * Fixup add documentation to the docstring * Add cell.png to the legacy registry * Update datadir order * Try to avoid circular stuff * Split up the init pooch function * Refine error message when pooch not found * Remove unused imports * Add the os directory test again * Add a test for download all * Add an error for installing Pooch. * Crosslink to our installation instructions * Update skimage/data/__init__.py Co-authored-by: Emmanuelle Gouillart <emma@plot.ly> Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu> Co-authored-by: Emmanuelle Gouillart <emma@plot.ly>

hmaarrfk added 3 commits May 9, 2020 11:34

Revert "Forget legacy data dir (scikit-image#4662)"

80f82a6

This reverts commit 137f144.

Make pooch an optional requirement

a907fbb

Make pooch an optional dependency

5083da2

hmaarrfk force-pushed the optional_pooch branch from bdb4108 to 5083da2 Compare May 9, 2020 20:17

hmaarrfk added 3 commits May 9, 2020 13:31

Add Pooch to documentation requirements

19725c0

Fixup add documentation to the docstring

21b3d96

Add cell.png to the legacy registry

f17f577

hmaarrfk added 2 commits May 9, 2020 14:45

Update datadir order

0aa462d

Try to avoid circular stuff

6bd8d93

Split up the init pooch function

26d5138

hmaarrfk force-pushed the optional_pooch branch from fff93a2 to 26d5138 Compare May 9, 2020 23:43

hmaarrfk changed the title ~~[WIP] Optional pooch~~ Optional pooch dependency May 10, 2020

jni approved these changes May 10, 2020

View reviewed changes

This was referenced May 10, 2020

pooch is a hard requirement #4665

Closed

MAINT: Remove conditional dependencies on Python <3.6 nipreps/niworkflows#517

Merged

Refine error message when pooch not found

f57ad3d

jni and others added 2 commits May 10, 2020 08:17

Remove unused imports

6d58f76

Add the os directory test again

26f5501

hmaarrfk force-pushed the optional_pooch branch 2 times, most recently from 10ed769 to 26f5501 Compare May 10, 2020 15:21

hmaarrfk added 3 commits May 10, 2020 08:33

Add a test for download all

98cb4f9

Add an error for installing Pooch.

74042bf

Crosslink to our installation instructions

d2c0b0a

emmanuelle reviewed May 10, 2020

View reviewed changes

skimage/data/__init__.py Outdated Show resolved Hide resolved

emmanuelle approved these changes May 10, 2020

View reviewed changes

Update skimage/data/__init__.py

62c9767

Co-authored-by: Emmanuelle Gouillart <emma@plot.ly>

emmanuelle merged commit 14b521a into scikit-image:master May 11, 2020

jni mentioned this pull request May 11, 2020

[WIP] Make Pooch lazily create directories for python 3.7+ #4670

Closed

8 tasks

lumberbot-app bot added the Still Needs Manual Backport MrMeeseeks-managed label label May 11, 2020

emmanuelle mentioned this pull request May 11, 2020

2020's calendar of community management #4486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional pooch dependency #4666

Optional pooch dependency #4666

hmaarrfk commented May 9, 2020 •

edited

Loading

hmaarrfk commented May 9, 2020 •

edited

Loading

pep8speaks commented May 9, 2020 •

edited

Loading

jni left a comment

jni May 10, 2020

jni May 10, 2020

hmaarrfk May 10, 2020

hmaarrfk May 10, 2020

hmaarrfk May 10, 2020

hmaarrfk May 10, 2020

jni May 10, 2020

hmaarrfk May 10, 2020

hmaarrfk May 10, 2020

jni May 10, 2020

hmaarrfk May 10, 2020

jni commented May 10, 2020

jni commented May 10, 2020

hmaarrfk commented May 10, 2020

hmaarrfk commented May 10, 2020

emmanuelle left a comment

emmanuelle commented May 10, 2020

hmaarrfk commented May 10, 2020

jni commented May 11, 2020

emmanuelle commented May 11, 2020

lumberbot-app bot commented May 11, 2020

emmanuelle commented May 11, 2020

hmaarrfk commented May 11, 2020



		@skipif(OFFLINE, reason='No internet connection')
		def test_download_all():

Optional pooch dependency #4666

Optional pooch dependency #4666

Conversation

hmaarrfk commented May 9, 2020 • edited Loading

Description

Checklist

For reviewers

hmaarrfk commented May 9, 2020 • edited Loading

pep8speaks commented May 9, 2020 • edited Loading

Comment last updated at 2020-05-10 22:58:31 UTC

jni 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jni commented May 10, 2020

jni commented May 10, 2020

hmaarrfk commented May 10, 2020

hmaarrfk commented May 10, 2020

emmanuelle left a comment

Choose a reason for hiding this comment

emmanuelle commented May 10, 2020

hmaarrfk commented May 10, 2020

jni commented May 11, 2020

emmanuelle commented May 11, 2020

lumberbot-app bot commented May 11, 2020

emmanuelle commented May 11, 2020

hmaarrfk commented May 11, 2020

hmaarrfk commented May 9, 2020 •

edited

Loading

hmaarrfk commented May 9, 2020 •

edited

Loading

pep8speaks commented May 9, 2020 •

edited

Loading