GloSED database incorporation

[tom-brekke]

PR checklist

• This comment contains a description of changes (with reason).
• If you've fixed a bug or added code that should be tested, add tests!
• If you've added a new tool - have you followed the pipeline conventions in the contribution docs
• [ not needed ] If necessary, also make a PR on the nf-core/ampliseq branch on the nf-core/test-datasets repository.
• Make sure your code lints (nf-core pipelines lint).
• Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
• Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
• Usage Documentation in docs/usage.md is updated.
• [ not needed ] Output Documentation in docs/output.md is updated.
• CHANGELOG.md is updated.
• [ not needed ] README.md is updated (including new tool citations and authors/contributors).

Issues closed

would close #1008 adding a new database to the list of approved databases.

Change notes

I've added in a new database option: GloSED. This is mainly through the addition of two new scripts, one to download the required files and one to reformat them appropriately. I have also made minor edits to the main pipeline to allow GloSED as an acceptable database option, integrate it if chosen, and cite it appropriately.

I have added a test profile for glosed to test the database.

Though GloSED does include SH numbers I have not included the code to process them and so the GloSED database does not currently work with addSH. Though this is reasonably possible to do if the need arises.

[d4straub]

Thanks for contributing!
For completeness, a test_glosed.nf-test.snap would be awesome.

[tom-brekke]

I've added the test_glosed.nf-test.snap file and addressed the other comments. Let me know if you see anything else. Thanks

[d4straub]

Great, almost there!

[d4straub]

Thanks!

[d4straub]

@nf-core-bot fix linting

[d4straub]

So now that the nf-core bot also fixed linting, the last issue is the test_glosed.
The CI runner fails with

    > ERROR ~ Error executing process > 'NFCORE_AMPLISEQ:AMPLISEQ:DADA2_DENOISING (1)'
    > 
    > Caused by:
    >   Process requirement exceeds available memory -- req: 20 GB; avail: 15.3 GB

I am not sure if that isnt a red herring. Because DADA2_DENOISING is running on the samplesheet that is also used in other tests.
I rather have the suspicion that the glosed database is possibly too large for the CI runner. The perfect way would be to make a small database subset and run it. However, I am not sure that worth the effort. You tested with the database right, how much RAM was required to run the test?

[tom-brekke]

yeah, I actually had to add in a config file for the glosed test (conf/test_glosed.config) and bump the RAM way up:

process {
resourceLimits = [
cpus: 4,
memory: '256.GB',
time: '2.h'
]
}

It may not need quite 256, but it was struggling with 32 and I set it high to get it to pass.
It wasn't the denoising step I had problems with though - I kinda think it was dada2_taxonomy though I can't be sure.

I could pull a couple lines from the database, but yeah it sounds like a lot of effort to wire that all in just for the test.
Are other databases really so small that this isn't an issue? I can't think that the GloSED database is all that much bigger than any other one.

[d4straub]

I see. The CI runners have only 15GB. And I dont think it makes sense to use larger runners (costs, energy...).
The test profiles do not cover all databases, most are too large. However, nearly all processes are tested with small databases (older releases, otherwise tiny). If that isnt feasible with glosed, maybe we have to omit the test sadly. That makes it less reliable (someone might break it by modifying code and it might be obvious only much later), but so be it.

[tom-brekke]

If I were to make a small test database, where would I host it? In the ampliseq repo?

[d4straub]

The test database could go to https://github.com/nf-core/test-datasets/tree/ampliseq, maybe https://github.com/nf-core/test-datasets/tree/ampliseq/testdata (there are already files with 85_greengenes* in there).

[tom-brekke]

I'm going to struggle to get the test database made in a timely fashion unfortunately.
Is that all that's holding back the PR? Is there anything else I can do to help move it forward?

[d4straub]

Usually all CI tests must pass.
The PR here is an exception though:

• I broke tests 12/16 & 13/16 in ampliseq: add nanopore data test-datasets#2138, so that doesnt need to pass.
• test 14/16 failed due to connectivity issues with a database, that is no problem.

However, the PR is blocked by the non-functioning CI test test_glosed (5/16). Either this needs to make work or it needs to be omitted for CI tests (i.e. remove the tests/glosed.nf.test & tests/glosed.nf.test.snap). Which one shall it be then?

[tom-brekke]

yeah I see. I've just made a PR with nf-core/test-datasets on against the ampliseq branch that has the first 1000 entries in the glosed database - should be enough for the CI testing I hope. I'll wait until that gets merged and then I'll update the glosed test here with that test file location and we'll see what happens.

[d4straub]

Thats ok i think. An alternative could be to add an entry such as "glosed_1k" or such. Similar to

ampliseq/conf/ref_databases.config

Lines 881 to 889 in 2723d4c

	'greengenes88' {
	title = "Greengenes - Version 13_8 - clustered at 88% similarity - for testing purposes only"
	file = [ "ftp://greengenes.microbio.me/greengenes_release/gg_13_5/gg_13_8_otus.tar.gz" ]
	citation = "McDonald, D., Price, M., Goodrich, J. et al. An improved Greengenes taxonomy with explicit ranks for ecological and evolutionary analyses of bacteria and archaea. ISME J 6, 610–618 (2012). https://doi.org/10.1038/ismej.2011.139"
	fmtscript = "taxref_reformat_sidle.sh"
	dbversion = "Greengenes 13.8 (https://ftp.microbio.me/greengenes_release/gg_13_5/)"
	taxlevels = "D,P,C,O,F,G,S"
	derep = "88"
	}

. But I dont see really a benefit to change it. Just wanted to mention it in case you have a preference.

[d4straub]

@nf-core-bot fix linting

[tom-brekke]

Ok, I think I've gotten is sorted so that test 5 has passed - the mini-database hosted on nf-core/test-datasets/ seems to be working well.

So I think this is nearly ready to merge, but I wanted to flagpost one more issue: I changed up nextflow.config a bit and I think one of you should have a close look to make sure it's an ok change:

I moved the line: includeConfig 'conf/ref_databases.config' up, to before the profiles are loaded so that the glosed test profile would end up overwriting the defaults from ref_database.config if the test is run. Works nicely for the glosed test, but if there are any specific things set in the ref_databases.config file that need to override the profiles, then this isn't going to work and I'll need to find another solution.
Hopefully it'll be ok - it seems logical that the profiles could be loaded after the ref_databases but please have a look before we merge the branch.

[d4straub]

I changed up nextflow.config a bit and I think one of you should have a close look to make sure it's an ok change:

Afaik we never had the case that we wanted to override the ref_databases.config values. ref_databases.config should only provide a base of readily available databases that can be modified. The change of the sequence of config loading is fine I think. So all good to go!

Do you see here the green Merge button? If not, you could join the nf-core community via https://nf-co.re/join#github or let me know and I merge this PR.