https://nf-co.re logo
Join Slack
Powered by
<@U069RTFA151> - I have two options for the last 1...
# createtaxdb
j
@Sam Wilkinson - I have two options for the last 10 minutes: So code looks good and I can follow very easily, so I'm pretty sure it's working. 1. I can double check with my own tests 2. I can copy over the logic for the amino acid equivalent, as I think this might be slightly out of date (unless you let me know otherwise) What do you think I should do?
OK hit first issue with testing, but partner ahs meeting now. Will look after.
s
sure, happy to talk things over if that would be helpful!
j
OK goblin is asleep
I'll continue testing and see if I Can fix the firs tissue
If you have time/are bored, you can check nr 2., otherwise I'll do it after testing
s
The batching problem? I'm a little confused by how that would lead to resume issues
j
So essentially, because Nextflow is very 'async' with the whole data flow thing
(The batching problem can come later though, I'm more concerned about the amino acid fastas)
Theoretically, when Nextflow splits up teh samplesheet rows, as they going into the 
collate
channel, if you resume the order may be different
Which means that even if the pipeline originally did the gzipping/unzipping etc successfull the first time, Nextflow will consider those processes are invalidated the cache (as the process recieved different input files in the batch)
I might be overthinking it, but the issue in taxprofiler was quite similar: we were batching FASTQ files as they went into Krakenuniq, and because on resume the order was different it would retrigger
(and FYI, the error I'm getting during testing is:
Copy code
ERROR ~ Unknown method invocation `endsWith` on ArrayList type

 -- Check script '/home/james/git/nf-core/createtaxdb/./workflows/createtaxdb.nf' at line: 78 or see '.nextflow.log' file for more details
ERROR ~ Pipeline failed. Please refer to troubleshooting docs: <https://nf-co.re/docs/usage/troubleshooting>
)
s
I'll see if I can reproduce
j
on
Copy code
ch_dna_refs_for_rematching = ch_samplesheet
            .map { meta, fasta_dna, _fasta_aa ->
                [
                    fasta_dna.getBaseName(fasta_dna.name.endsWith('.gz') ? 1 : 0),
                    meta,
                ]
            }
s
right I'll try to reproduce both problems
👍 1
My tests are all passing locally though but that's not actually using a batch obviously
👍 1
j
Copy code
bash
nextflow run ../main.nf -profile test,docker --outdir ./results -ansi-log false -dump-channels -resume --input samplesheet_multiplefaa.csv
Where I've added an extra amino acid FASTA
Copy code
id,taxid,fasta_dna,fasta_aa
Severe_acute_respiratory_syndrome_coronavirus_2,2697049,<https://raw.githubusercontent.com/nf-core/test-datasets/createtaxdb/data/fasta/sarscov2.fasta,https://raw.githubusercontent.com/nf-core/test-datasets/createtaxdb/data/fasta/sarscov2.faa>
Haemophilus_influenzae,727,,/home/james/Downloads/sequence.fa
Ahhhh wait
I know whby
You filtered the channel for refmatching (just the meta), but didn't do the same filtering for the channel going to unzipping
s
how does that explain the error being at the refmatching block?
j
Nope, that was me getting confused
refmatching was fine
No wait
One second 😆
Copy code
ch_dna_refs_for_singleref = ch_samplesheet
            .map { meta, fasta_dna, _fasta_aa -> [meta, fasta_dna] }
            .filter { _meta, fasta_dna ->
                fasta_dna
            }

        // Make channel to preserve meta for decompress/compression
        ch_dna_refs_for_rematching = ch_samplesheet
            .filter { _meta, fasta_dna, _fasta_aa ->
                fasta_dna
            }
            .map { meta, fasta_dna, _fasta_aa ->
                [
                    fasta_dna.getBaseName(fasta_dna.name.endsWith('.gz') ? 1 : 0),
                    meta,
                ]
            }
You were missing the 
.filter
on the refmatching, which I've now added as above
So the singleref one was correctly filtered to just those with DNA fastas
But the refmatching hadn't done that yet
s
Right
that makes sense
Copy code
jovyan:/shared/team/sam/ludicrous_db$ /shared/team/sam/nextflow -c no_aws_nextflow.config run nf-core/createtaxdb -r find_concatenate_patch -latest    -profile docker    --input samplesheet_small.csv    --outdir /shared/team/sam/ludicrous_db/output --build_kraken2 --dbname ludicrous_db --accession2taxid nucl_wgs.accession2taxid --nodesdmp /shared/public/db/taxonomy/latest/nodes.dmp --namesdmp /shared/public/db/taxonomy/latest/names.dmp --unzip_batch_size 100 -resume
Nextflow 24.10.4 is available - Please consider updating your version to it

 N E X T F L O W   ~  version 24.10.3

Pulling nf-core/createtaxdb ...
 Already-up-to-date
Launching `<https://github.com/nf-core/createtaxdb>` [nice_descartes] DSL2 - revision: 24ba8c6c1a [find_concatenate_patch]


------------------------------------------------------
                                        ,--./,-.
        ___     __   __   __   ___     /,-._.--~'
  |\ | |__  __ /  ` /  \ |__) |__         }  {
  | \| |       \__, \__/ |  \ |___     \`-._,-`-,
                                        `._,._,'
  nf-core/createtaxdb 1.0dev
------------------------------------------------------
Input/output options
  input              : samplesheet_small.csv
  outdir             : /shared/team/sam/ludicrous_db/output
  dbname             : ludicrous_db
  accession2taxid    : nucl_wgs.accession2taxid
  nodesdmp           : /shared/public/db/taxonomy/latest/nodes.dmp
  namesdmp           : /shared/public/db/taxonomy/latest/names.dmp

Database Building Options
  build_kraken2      : true

Generic options
  trace_report_suffix: 2025-02-06_11-16-07
  unzip_batch_size   : 100

Core Nextflow options
  revision           : find_concatenate_patch
  runName            : nice_descartes
  containerEngine    : docker
  launchDir          : /shared/team/sam/ludicrous_db
  workDir            : /shared/team/nxf_work/sajwilkinson.mscape/work
  projectDir         : /shared/team/nxf_work/sajwilkinson.mscape/assets/nf-core/createtaxdb
  userName           : jovyan
  profile            : docker
  configFiles        : /shared/team/nxf_work/sajwilkinson.mscape/assets/nf-core/createtaxdb/nextflow.config, /shared/team/sam/ludicrous_db/no_aws_nextflow.config

!! Only displaying parameters that differ from the pipeline defaults !!
------------------------------------------------------

* The nf-core framework
    <https://doi.org/10.1038/s41587-020-0439-x>

* Software dependencies
    <https://github.com/nf-core/createtaxdb/blob/master/CITATIONS.md>

executor >  k8s (1)
[fe/161927] NFCORE_CREATETAXDB:CREATETAXDB:UNPIGZ_DNA (ludicrous_db)                                  [100%] 5 of 5, cached: 5 ✔
[49/8ddbfb] NFCORE_CREATETAXDB:CREATETAXDB:FIND_CONCATENATE_DNA (ludicrous_db)                        [100%] 1 of 1, cached: 1 ✔
seems to be resuming fine
j
Left a comment with the fix
For the endsWidth thing
For the resuming though: just 5 test files?
s
yep, I commited it
j
Correct?
s
500, I just batched them in 100s
j
Ok cool
Then let's ignore for now
Goblin woke, once he's settled I'll check again
s
sure sure 🙂
j
Do you want to copy paste for the amino acid thing in the meantime?
If you have time ofc
s
I'm concerned that tests are passing without it
it might indicate that the DNA bits are pointless
j
(just careful with the seqkit thing for ksiju if you do, I've already done that once)
Hrm
I'll have a look
s
if it's just case of copy paste that's fine but I'd rather figure out why it's working when it possibly shouldn't hah
j
yeah that's fine
s
speaking of resume problems;
Copy code
Execution cancelled -- Finishing pending tasks before exit
-[nf-core/createtaxdb] Pipeline completed with errors-
ERROR ~ Error executing process > 'NFCORE_CREATETAXDB:CREATETAXDB:FASTA_BUILD_ADD_KRAKEN2_BRACKEN:KRAKEN2_ADD (ludicrous_db)'

Caused by:
  Process `NFCORE_CREATETAXDB:CREATETAXDB:FASTA_BUILD_ADD_KRAKEN2_BRACKEN:KRAKEN2_ADD (ludicrous_db)` terminated with an error exit status (1)


Command executed:

  mkdir ludicrous_db
  mv "taxonomy" ludicrous_db
  
  echo ludicrous_db.fna |\
  tr -s " " "\012" |\
  xargs -I {} -n1 kraken2-build \
      --add-to-library {} \
      --db ludicrous_db \
      --threads 6 \
  
  
  cat <<-END_VERSIONS > versions.yml
  "NFCORE_CREATETAXDB:CREATETAXDB:FASTA_BUILD_ADD_KRAKEN2_BRACKEN:KRAKEN2_ADD":
      kraken2: $(echo $(kraken2 --version 2>&1) | sed 's/^.*Kraken version //; s/ .*$//')
  END_VERSIONS

Command exit status:
  1
kraken2 add uses mkdir without a p
j
What do you mean?
(liking the database name btw 😉 )
s
it'll error on resume since the dir already exists
yeah hah
j
so it's trying to re-run in the same working directory/
s
that's standard resume behaviour no?
j
I thought it made a new working directory 😅
OK that explains A LOT
that's annoying
s
don't think so, it's based on hashes
or something
j
(yeah, I thought it incoporated checking of the exit code or something in them)
OK I guess time to update that module
by without a p I guess you mean 
mkdir -p
?
s
yah
I'll just test locally to make sure
j
s
it seems to be but I'll wait for the process to actually finish
👍 1
yep that finished fine on a resume
j
nice
if you can approve the PR above then we merge in and I can update the module in the pipeline
s
I don't think I have review rights
oh right I guess I do
j
😬
s
I thought it meant write access to the modules repo itself
j
Hrm so what happens again if we have two FASTA files named the same....
when it gets to joining back with the meta
We've had this conversation before :swaet
s
it'll fail due to duplication
j
Ah yes
s
but that shouldn't happen due to IDs being enforced unique
👍 1
unless someone does something very silly
which one can never account for fully
j
I'll put a note to document that
s
yeah good call
j
And I wonder if we can check that upfront somehow
s
We could error out descriptively once the fasta file / meta map is generated
j
Yes exactly
s
the join duplicate error is actually somewhat good on its own tbh
j
OK
s
it shows you the offending values
j
True but it's a bit late if it's after all the unzipping is my worry
s
yeah esp if it's a big batch
no harm in having a nice exception
j
ok I've made an issue
OK I'm hapy with the DNA!
Works nicely
While you check the Amino Acid thing I'll try doing the check we just mentioned
s
do you have say 100 AA examples I can test with?
I suspect that it would break if we hit AA fastas > batch size
j
noy on me no, sorry
What do you mean by it'll break?
(and would the same apply to DNA?)
s
so I think it's working because we aren't actually batching the AAs
j
Goblin is waking up again tho, sorry, got to run. Started the error message
Ah ok, so then copying makes sense?
s
I think so, I wanna check first though
j
(tbf, copy your FASTAS, rename to 
.faa
and as long as you get pasted gunzipping you're fine
s
yeah true
I'll just do that
j
Oorrrr
That describes how I manually built a refrence database from some metagenomics becnhmarking data paper for the taxprofiler full test
awk / xarg commands should download the proteins foryou
But now definitely going
👍 1
s
Yep it broke as anticipated, I've added the maps back
good spot 🙂
Right we need an AA join too
seqkit replace relies on tax ids
if we could add like 2 more files of fasta / aa fasta to the test profile it would make the tests much better also
since we could just drop the batch size down to 2
👍 1
seqkit replace definitely needs to do batches too
👍 1
which is probably going to need to be a local module, up to you really
👍 1
j
Yup, I've been thinking to extend test data too... will try and prioritise that
s
I think we should do that in this PR, so that the batching funcs are properly tested
what do you think?
j
Yes Good idea 💯
s
cool
but once that's done I think we're basically ready to go
j
🎉
For amino acid you've updated the code int he PR already, or?
(as far as you could)
s
yeah I have
j
grand
s
it's passing tests and my local run which does kaiju
👍 1
it's basically identical logic to the DNA bit so it was easy\
👍 1
j
I guess one thing that could happen in the meantime as we having to make larger changes
Is to move all of this prep stuff into a subworkflow
s
yeah that would defo make a lot of sense
another PR though 😛
j
Coudl do it now in this one I was thinking
Because it's jsut moving those sections into a new file
s
Yeah that's true enough
j
And basically a single input/4 output channels
it's not a terrible section to do that in
If you don't mind (while you wait for me to make the new test data)
But no worries if not
s
yeah alright I'll have a look
❤️ 1
Open in Slack
PreviousNext
Powered by