bioconvert / bioconvert

Bioconvert is a collaborative project to facilitate the interconversion of life science data from one format to another.
http://bioconvert.readthedocs.io
GNU General Public License v3.0
368 stars 44 forks source link

assert dependencies of a method are here before making it available #136

Closed bryan-brancotte closed 6 years ago

bryan-brancotte commented 6 years ago

As an example fastq2fasta has a method GATB which needs gatb.Bank. If you don't have the dependency on your machin, using this method will fail. Why couldn't we remove this method from the proposed methods (displayed in -h, and acceptable with -c)


    @classmethod
    def _isusable_method_GATB(cls):
        try:
            # Let us make this optional for now because
            # GATB cannot be install on RTD
            from gatb import Bank
            return True
        except:
            pass
        return False

The drawback being that methods _isusablemethod are run at each startup of bioconvert, and for method relying on shell we thus need to evaluate if the binary is in the path which could increase the booting time of bioconvert.

bryan-brancotte commented 6 years ago

branch made : https://github.com/biokit/bioconvert/tree/issue_136_assert_dependency

cokelaer commented 6 years ago

@bneron

Instead of class method, another option (do not know right whether it is better or not) is to set a decorator ( @ requires("gatb") ) but here we need to use it for executables only (not python libraries that should be in the requirements). However, other dependencies (compiled ones) should be handled that way most probably.

by the way, we should drop GATB because it has different names on pypi and bioconda and has caused problems since the beginning. However, the idea raised in this issue is still valid for all other methods. Let us keep this example for now and see how we can implement it.

This raises a general question though. Do we want to include non-python libraries on the master. The answer is yes. For instance, we do not want to recode samtools. So at the end we will need to install third-party tools. I do not see any alternatives right now.

Some recommendations for now:

bryan-brancotte commented 6 years ago

I proposed in the branch a decorator, but for now it will only works correctly on unix like system : I use which to test if the binary is in the path or not. How about writing test on this decorator ? I for now have no idea

bryan-brancotte commented 6 years ago

Following a discution with @bneron

cokelaer commented 6 years ago

I've never used the distutils.spawn.find_executable.

which is a unix command so indeed using a more generic command (moreover faster) is a good idea indeed.

bryan-brancotte commented 6 years ago

Refactoring done. Decorator is tested in ci.

cokelaer commented 6 years ago

Seems good to me. I'm closing this issue. It works in the dev branch. More tests will come but the implementation works.