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

MNT: Remove double dependency on dask #33

Merged
merged 2 commits into from
Nov 7, 2018

Conversation

hmaarrfk
Copy link
Contributor

@hmaarrfk hmaarrfk commented Oct 2, 2018

  • Remove build dep on numpy doc
  • Add cytoolz as a dependency for speed
  • Update build command

Related to #32

- Remove build dep on numpy doc
- Add cytoolz as a dependency for speed
- Update build command
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@@ -37,12 +36,14 @@ requirements:
- matplotlib >=2.0.0
- networkx >=1.8
- pillow >=4.3.0
- dask-core >=0.15
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, in this iteration i decided that 0.9 was the correct version choice ;)

Copy link
Member

Choose a reason for hiding this comment

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

For reference, 0.15.2 was the first release that included dask-core IIRC. Hence why it was used as the lower bound here.

- toolz >=0.7.4
- pywavelets >=0.4.0
- dask-core >=0.9.0
- cloudpickle >=0.2.1
- imageio >=2.1.0
# scikit-image depends on dask-array
# which requires numpy, dask-core and toolz (cytoolz for speed)
- cytoolz >=0.7.3
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether it's a good idea include Cytoolz... Doesn't this go against your philosophy of making a minimal package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm mostly for "having choice to create a minimal package if I want to". I don't care for example if xarray installs 100 things, but I want to be able to depend on xarray-core

  1. cytoolz is basically faster toolz.
  2. The only dependency ctoolz has is toolz.

conda-forge is about having things work well out of the box. I think cytoolz hits that "just right"

Copy link
Member

Choose a reason for hiding this comment

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

This is probably my fault for not reviewing PR ( conda-forge/dask-feedstock#49 ) sooner. I've take a look and it seems basically fine. Maybe that will make things more straightforward here.

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Oct 2, 2018

@jni, maybe we should wait for conda-forge/matplotlib-feedstock#178 to get pulled in so we can depend on matplotlib-core?

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Oct 3, 2018

@jni, doesn't seem so obvious how we would depend on matplotlib-core >= 2.0.0 without depending on matplotlib 3.0.0. I suggest we move forward with this initial cleanup while others weigh in on this issue

conda-forge/matplotlib-feedstock#179

@rth
Copy link
Member

rth commented Nov 7, 2018

Is anything preventing merging this? It looks good to me FWIW.

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Nov 7, 2018

I think @jni is just swamped and this is low priority.

@rth
Copy link
Member

rth commented Nov 7, 2018

Sure, that not an issue, but I'm hoping one of the other recipes maintainers might be willing to press the green button :) It's a fairly non controversial PR..

@jni
Copy link
Contributor

jni commented Nov 7, 2018

It's not entirely non-controversial to add a new dependency. But whatever, by popular demand, I'm merging. =D

@jni jni merged commit dc98677 into conda-forge:master Nov 7, 2018
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.

5 participants