search

bioboxes / bioboxes-py

Python library for creating and running bioboxes
2 stars 1 forks source link

feature(container): allow key values in biobox file and configurable version number #34

Closed pbelmann closed 7 years ago

pbelmann commented 7 years ago

according to the new bioboxes profiling interface and issue bioboxes/rfc#207 bioboxes-py should be able to process items that are not lists like the example below 

   - database:
         value: /path/database
         type: database type

Also bioboxes.yaml files produced by bioboxes-py should must have a configurable version number.

@michaelbarton could you review my PR please?

pbelmann commented 7 years ago

test seem to fail because it references/imports the command-line-tool which of course does not use the current bioboxes-py library. It is a circular dependency in my opinion.

michaelbarton commented 7 years ago

test seem to fail because it references/imports the command-line-tool which of course does not use the current bioboxes-py library. It is a circular dependency in my opinion.

Which part of bioboxes-py imports bioboxes/command-line-tool? I looked through the code and could not see any part that did. The pip dependency list does not appear to import this.

From looking at the tests on the CI server this fails not because of the command line tool library being imported, but because some of the funcions are being given incorrect arguments. Here is one case:

s = {'output': '/home/ubuntu/bioboxes-py/tmp/tests/tmpCrVAOT'}

    def isabs(s):
        """Test whether a path is absolute"""
>       return s.startswith('/')
E       AttributeError: 'dict' object has no attribute 'startswith'

Looking at the pull request, it looks like all the documentation for the create_container function was removed.. I would rather we update the documentation rather than delete it when a function's arguments are changed.

In the prepare_volumes function and the create_container function the ability to mount a metadata directory has been removed. I believe we agreed that a metadata directory should be mounted for containers to log output files to when necessary. This argument is optional so it is not necessary that it has to be used.

Networking enabled by default was also [removed from the create_container function][]. I added this because of an upstream issue in docker-py. I described this in the CHANGELOG however I should have probably added a comment in the code also. I would prefer to keep this in the code until upstream issue is fully resolved.

pbelmann commented 7 years ago

@michaelbarton thanks for the review. I updated the tests. It seems that command-line-tool is using version 0.3.0 instead of the current one. Thats why I accidently removed the code description introduced in the newest version https://github.com/bioboxes/command-line-interface/blob/master/requirements/default.txt#L1. Sorry for that mistake.

michaelbarton commented 7 years ago

Thanks Peter. Yes, that makes sense. Thanks for updating the PR. One additional suggestion would be to add tests for the extra functionality. I say this to ensure that it doesn't accidentally get broken by a future change. For example ensuring that a single entry also gets remapped correctly.

pbelmann commented 7 years ago

@michaelbarton I added a test