modified to define default moltype for functions used in QIIME #19
Conversation
|
@gregcaporaso I don't have a system with all the applications installed (I'm missing clustalw, fasttree v1, mafft and raxml; I failed to install these ones during the code sprint....) |
|
@ElBrogrammer can we set up Travis to download the applications and/or use some Amazon EC2 instance in which all the applications are already there? I think not being able to test brokit is a problem, since if we merge something here, it can completely break QIIME... |
|
I don't think we'll be able to use Travis to test brokit since there are so many dependencies, some of which are proprietary. Travis doesn't allow us to point it to an EC2 instance (or some other server) with dependencies that are already installed :(. If we use qiime-deploy to install deps on Travis, we'll likely run into issues with build timeouts since Travis has a hard time limit of 50 minutes. If we're willing to pay them, that'll bump us to 70 minutes and we might be able to cache our qiime-deploy builds, though we still wouldn't be able to test everything (e.g., usearch). I could set up Jenkins to test brokit. This would at least allow brokit pull requests to be tested. Would that be useful? @gregcaporaso what do you think? |
|
Another option is to decouple brokit app controller tests from actually running the wrapped application. The tests would instead check that the built command is correct, but never actually run it. This way brokit wouldn't rely on any external executables, we could test via Travis, and the tests would run very quickly. This option would require more work to convert the existing tests though... |
|
That would be an awesome model to follow. One other thing we could do is use NumPy's skip_if decorator to skip tests that check for a program's output if said program is not installed. |
|
I think before we get into modifying tests we need to figure out which of these app controllers we plan to continue to use going forward - remember the motivation with brokit was to have to make minimal changes to these because we know we're not going to continue to maintain all (even most) of them. Maybe for the time-being I should just try to test in the QIIME VM - I think all of these are installed in there. Let me try that and I'll follow up, I really want to minimize the time we spend on this particular package. |
|
I do agree that that model could be good going forward with the ones that we do want to maintain though. |
|
Not all apps installed in the VB either... argh... I'll continue working on a solution for this. |
|
I've had all of the tests pass on different machines (combination of the QIIME VM and compy), with the exception of test_fasttree_v1.py, but I get the same error in that case with master and my branch. I think we should merge this PR since my change is very unlikely to break the fasttree_v1 wrapper, and tag fasttree_v1 for deletion from QIIME's make_phylogeny.py (update: done, now QIIME's #1516) (users who want that could always build the tree directly with FastTree v1, or get an older version of QIIME). @josenavas, do you want to review/merge? I posted some related notes on install issues for application controllers on #20. |
| # generate temp filename for output | ||
| params["-w"] = "/tmp/" | ||
| # generate temp filename for output | ||
| params["-w"] = "/tmp/" |
There was a problem hiding this comment.
This forces to use always the /tmp directory as a temporary directory. In some systems, this is problematic and it looks like we have seen some issues in other app controllers (see biocore/qiime#1515). Probably is out of the scope of this PR, so adding an issue so we can keep track of it will be enough. What others think?
There was a problem hiding this comment.
Out of the scope of this PR, but I agree that it should be tracked and fixed for the application controllers that we decide to keep going forward.
|
Note: ignoring whitespace changes in the PR review (via this link) is really handy for this PR. Thanks @ElDeveloper for the "cheat sheet" link. |
|
Thanks @gregcaporaso This looks good. Before merging, did you run the QIIME tests using this version of brokit? |
|
I didn't, but it would be hard because I don't have all of the apps What do people think? Since these changes are really small, is it On Mon, Apr 21, 2014 at 11:29 AM, josenavas notifications@github.comwrote:
|
|
I don't think they're going to broke anything, so I'm ok with merging it as it is. |
|
I think we should merge too. |
modified to define default moltype for functions used in QIIME
|
Thanks @gregcaporaso |
|
Just FYI, the QIIME tests are passing with the latest version of brokit (mentioning due to the testing issue discussed above). |
fixes #17
@josenavas, do you have a system with all of the applications installed? I'm not able to confirm that all tests pass with this PR because I don't have a system with all of the wrapped applications installed. If you have this, can you pull this branch and test? If you don't I can work on it, but won't be able to get to it right away.