8000 [WIP] Make Pooch lazily create directories for python 3.7+ by hmaarrfk · Pull Request #4670 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

hmaarrfk
Copy link
Member

Description

@jni just opening this up as a potential solution for #4660

Checklist

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.

@pep8speaks
Copy link
pep8speaks commented May 10, 2020 •
edited
Loading

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

Line 138:1: E302 expected 2 blank lines, found 1

Comment last updated at 2020-05-11 13:37:23 UTC

emmanuelle
emmanuelle reviewed May 10, 2020
View reviewed changes
skimage/__init__.py
if sys.version_info < (3, 7):
from .data import data_dir

def __getattr__(name):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice use of py3.7+ lazy loading :-).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, lets work on this in a bit. I want to get this PR to be really tiny, since it is hard to test in a pytest way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly that was my quick and dirty attempt. I am convinced there is an elegant solution here without even global variables, but it's been a long week (gave a skimage+napari workshop today) and I'm just not seeing it. I'm happy to have another pass at this in a few days after recharging my batteries, but anyone should feel free to give it a go in the meantime!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think image_fetcher needs to be made a class.... Was thinking about this problem in the shower and you know that is where the best ideas come!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. The whole problem is that we have to keep track of a lot of state — that's what classes are for! I was stubbornly treating the whole module as a big class.

@jni
Copy link
Member
jni commented May 11, 2020

@hmaarrfk thanks! Can you rebase now that #4666 is merged?

lazy_pooch is a great branch name, reminds me of my greyhound. =D This is him right now:

IMG_4310

😃

jni added 2 commits May 11, 2020 06:37
@jni @hmaarrfk
Make data_dir in skimage lazy in Py3.7+
f37c6cc
@jni @hmaarrfk
Make pooch initialisation lazy in Py3.7+
220ea8a 
This commit attempts to make our pooch fetcher a singleton global
to the data module.
@hmaarrfk
Copy link
Member Author

ok i rebased, but i must admit it needs serious review in light of some organizational changes in the original branch. sorry about that one.

I'm also still not convinced that exist_ok=True doesn't fix things for the affected user in the original issue.

@emmanuelle emmanuelle mentioned this pull request May 11, 2020
Closed
@emmanuelle
Copy link
Member

@jni @hmaarrfk if you think this PR is going to take some time to get merged in I can also release 0.17.2 with pooch an optional dependency and 0.17.3 after this one is merged. Please tell me!

@jni
Copy link
Member
jni commented May 11, 2020

@emmanuelle please do it! Optional pooch will already solve the problem for the OP of #4660 since they are not using the sample data at all.

@hmaarrfk
Copy link
Member Author

Definitely do it. This will help people building on automated machines for sure. Unless they use conda...

This was referenced May 12, 2020
Closed
Closed
@sciunto
Copy link
Member
sciunto commented Aug 29, 2020

THe issue #4660 is solved. Is this still useful in any sense?

@hmaarrfk
Copy link
Member Author

I think the correct solution is a two pronged approach:

Leaving the data_dir pointing to the old datasets.

Allowing users that have pooch installed use the newer, larger datasets.

I'm not super convinced, I have all the use cases right, I'll try to write a spec.

@hmaarrfk hmaarrfk closed this Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmanuelle emmanuelle emmanuelle left review comments

@jni jni jni left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hmaarrfk @pep8speaks @jni @emmanuelle @sciunto
0