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

[WIP] Which dependencies are really optional? #770

Open
wants to merge 1 commit into
base: master
from

Conversation

@luizirber
Copy link
Member

luizirber commented Nov 6, 2019

Turns out our optional dependencies are... not so optional. If sourmash is installed from PyPI it doesn't even manage to print the help message when executed (because numpy is being imported but is not listed as a required dependency).

Dependencies needed to get to help message for each subcommand

  • compute: no deps for regular input, need bam2fasta if input is 10x.
  • compare: depend on numpy right at the beginning.
  • plot: numpy, matplotlib and scipy.

Should we require numpy, scipy and matplotlib? They are already all base requirements in the bioconda recipe.

Checklist

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?
@luizirber luizirber force-pushed the fix/imports branch from c760c16 to cfdac22 Nov 6, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 6, 2019

Codecov Report

Merging #770 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #770      +/-   ##
==========================================
+ Coverage   89.48%   89.48%   +<.01%     
==========================================
  Files          29       29              
  Lines        4554     4555       +1     
  Branches       46       46              
==========================================
+ Hits         4075     4076       +1     
  Misses        476      476              
  Partials        3        3
Impacted Files Coverage Δ
sourmash/compare.py 89.7% <100%> (+0.15%) ⬆️
sourmash/np_utils.py 100% <100%> (ø) ⬆️
sourmash/command_compute.py 96.55% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92c8bf9...cfdac22. Read the comment docs.

@luizirber luizirber changed the title [WIP] What dependencies are really optional? [WIP] Which dependencies are really optional? Nov 6, 2019
@olgabot

This comment has been minimized.

Copy link
Collaborator

olgabot commented Nov 6, 2019

Should we require numpy, scipy and matplotlib? They are already all base requirements in the bioconda recipe.

If they're required for bioconda, why not for pip? That makes the most sense to me

@luizirber

This comment has been minimized.

Copy link
Member Author

luizirber commented Nov 7, 2019

If they're required for bioconda, why not for pip? That makes the most sense to me

That's a nice hack on their part: we didn't write the original recipe for sourmash in bioconda, but since it is a sort of canonical source for installing sourmash now it also becomes what's expected to be required for other sources too (like PyPI) =]

Supporting many dependencies with conda is fine because they are all pre-built, but it is harder for PyPI because not every operating system (or even alternative Python implementations) has numpy/scipy/matplotlib wheels, and compiling for source is excruciating. Might just be me being an old timer, but I would like to keep required external dependencies to a minimum.

I think it also boils down to use cases:

  1. compute -> index -> search doesn't need any extras
  2. compute -> compare -> plot needs numpy, scipy and matplotlib
  3. compute --input-is-10x -> compare needs bam2fasta and numpy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.