Skip to content
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.

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

Update minimum versions in dependencies, remove scikit-image & slicerator dependency #374

Merged
merged 3 commits into from
May 21, 2024

Conversation

jakirkham
Copy link
Member

It looks like a few dependencies had higher minimum versions when using setup.py

dask-image/setup.py

Lines 31 to 39 in 243fb0a

requirements = [
"dask[array] >=2024.4.1",
"dask[dataframe] >=2024.4.1",
"numpy >=1.18",
"scipy >=1.7.0",
"pims >=0.4.1",
"tifffile >=2018.10.18",
"pandas >=2.0.0",
]

Though we wound up with older versions after switching to pyproject.toml

dependencies = [
"dask[array,dataframe] >=2024.4.1",
"numpy >=1.11.3",
"scikit-image >=0.19.3",
"scipy >=0.19.1",
"pandas >=2.0.0",
"pims >=0.4.1",
"slicerator >= 1.1.0",
"tifffile >=2018.10.18",
]

Guessing this is just because the pyproject.toml PR has fallen out of date in this area and we missed it

So went ahead and bumped these. Though please let me know if I'm missing something

@jakirkham
Copy link
Member Author

Also assuming scikit-image and slicerator were added intentionally. Please let me know if not and can update accordingly

@jakirkham
Copy link
Member Author

@GenevieveBuckley could you please review?

@GenevieveBuckley
Copy link
Collaborator

Guessing this is just because the pyproject.toml PR has fallen out of date in this area and we missed it
So went ahead and bumped these. Though please let me know if I'm missing something

Yes, I think the PR just fell out of date between it being opened and merged.

Also assuming scikit-image and slicerator were added intentionally. Please let me know if not and can update accordingly

I added them because they are included in the environment yaml files. I was trying to make sure it wouldn't matter which method someone used to install it, you'd still get something that matches. That might not have been the best thing to do though.

I also thought for some reason that scikit-image was required for a new-ish function, but I can't actually see it being imported anywhere (including the tests). So maybe it shouldn't be there at all.

I think slicerator is a dependency of one of our other dependencies, so it might not be a thing we need to worry about in the pyproject.toml at all. Like I said, I was just trying to match the environment files, but that's maybe not the best strategy.

@GenevieveBuckley
Copy link
Collaborator

I also saw this issue from Mark about switching from dask to dask-core: conda-forge/dask-image-feedstock#16

* `scikit-image` is a test-only dependency
* `slicerator` is already pulled in by `pims`

So drop both of these as they shouldn't be needed at runtime.
@jakirkham
Copy link
Member Author

No worries. It's better to move forward with changes (like pyproject.toml) and fix a few things afterwards than let it linger indefinitely. So think you did the right thing 🙂

Think scikit-image was added for testing originally. If that is all we are using it for still, it should be safe to drop. So went ahead and did that. If we want to use things from scikit-image in the future, we can always add it back then

Regarding slicerator that was added as a workaround ( #17 ). Think we are safe to drop it at this point. So went ahead and did that

The dask-core bit is only relevant for the conda-forge feedstock. So let's save that discussion for there

Maybe there is a better way to manage the dependencies here and call out what is needed for testing to make it clearer. Open to thoughts on that, but maybe we can discuss in a new issue?

@GenevieveBuckley GenevieveBuckley merged commit 8393093 into dask:main May 21, 2024
17 of 18 checks passed
@GenevieveBuckley
Copy link
Collaborator

Thank you for doing this @jakirkham

@jakirkham
Copy link
Member Author

Happy to help 🙂

@jakirkham jakirkham deleted the update_deps branch May 21, 2024 04:07
@GenevieveBuckley GenevieveBuckley changed the title Update minimum versions in dependencies Update minimum versions in dependencies, remove scikit-image & slicerator dependency May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants