Attempt to run humann (3) with the official container#53
Conversation
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 3.5.2. For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation. |
|
I promise I ran these tests locally first y'all lol. I actually think this might not be an issue with my changes (?!?) but a nextflow version issue? A bit confused because I think they are all supposed to be running |
nickp60
left a comment
There was a problem hiding this comment.
A few things
-- this reverts to the old versions.yml instead of the updated versions topic, so all the versions logic changed here needs to be reverted
-- the meta should all be tagged as single_end at this point, as the tool does not accept paired data. The merging happens upstream and should be changing the single_end tag to true accordingly. Not sure why this is popping up now though...
-- the wrapper script is a nice idea! I hope it works. We might be back into headache territory due to the larger container size, but maybe we cross that bridge later
|
|
Updated the versions and marked everying as single ended. I think the linting failure is coming again from a nextflow version update issue, but attempts to set the nextflow version to a lwoer version also cause the linter to fail (because the linting.yaml file is no longer the same as the template lol). Bit of a catch 22 so maybe we take this as a prodding to comply idk. |
|
Thanks! LGTM. Feel free to bump the template if that makes merging easier. |
|
@miraep8 @nickp60 could we close this in favour of nf-core/modules#11201? |
|
@vinisalazar What @miraep8 is proposing here is a way to avoid having to use the patched versions that allow for the config location variable. If this works (and it seems to be!) then we don't have to ask people to use our custom docker images, which should make the review process easier. |
|
I think this should be incorporated into your module PR for humann @vinisalazar. Then we can use the official conda/docker containers and be off the hook for supporting them in the future. |
|
I think that Vini's solution in #11201 also makes use of the official docker images actually! His solution is different than mine ie: If this approach works for humann regroup (and it seems to be) then I would definitely be in favor of closing this in favor of your PR Vini as I think your solution seems more elegant than mine! I have just wanted to try your version out first and haven't had a chance to yet :) but I suspect it will be fine if its working in your hands. |
|
The HUMANN_CONFIG is the stuff I added in the patched version of humann -- that will not work in non-patched containers. The tests pass because (A) its only being run in -stub mode and (B) it regrouping to one of the tables included in the package data: its not using the data in the untarred utility database output from the UNTAR rule. you can confirm this by disabling the you can also add a line to the script section and look for which group options are available: which is missing all the ones from the utility database |
|
Thanks Nick! I had thought for some reason that Vini was doing something slightly different from what you had before. In that case Vini - maybe it would be worth integrating my changes into your human module pull instead. (At least for the regroup module)...sorry for the confusion on my part! And thank you both for your work on this :) |
HUMANN_CONFIG env var only works in patched containers. Use Python to set config.utility_mapping_database in-memory before calling humann.tools.regroup_table.main() directly. Also remove HUMANN_CONFIG block from humann/humann (no-op in official containers). Refs nf-core/funcprofiler#53 Co-authored-by: Mirae Baichoo <miraep8@gmail.com>
Add test_genefamilies.tsv with UniRef90 IDs present in utility_nfDEMO mapping. Add non-stub test to verify utility_db is actually used. Fix stub test input (was fastq, now correct TSV). Refs nf-core/funcprofiler#53 Co-authored-by: Mirae Baichoo <miraep8@gmail.com>
|
Just updated nf-core/modules#11201, please have look if possible |
Hi team!
Given the recent feedback we got I thought it might be worth taking a stab at running humann3 with the official container. Initially had been planning to upgrate to 3.9.1, but the signularity index for the galaxy projects only goes up to 3.9, so using that for now.
For the main step that required our hacky patch in the home-baked container (the humann_regroup submodule) I now have a hacky python script that tries to modify the config python object (ie doesn't need to actually change anything written in the container) before calling humann regroup. I think it works ok.... lets definitely do a full test before merging, just adding for now to share what I have been working on with this.