https://nf-co.re logo
Join Slack
Powered by
I have received a bug report which I do not really...
# pipeline-maintainers
n
I have received a bug report which I do not really understand, could someone take a look at this github.com/nf-core/genomeassembler/issues/238 and let me know what you think? My general impression is that the suggested cause for the bug is misleading, but I also do not really have a good understanding of what is causing concurrent modificiation exceptions, I would generally assume that the tests should catch that, but maybe this is some edge-case?
j
That definitely seems like a plausible cause. You have to be really careful with maps in Nextflow, as they are not copied - you have to create a new map each time, or multiple channels end up with pointers to the same map. This can definitely lead to ConcurrentModificationExceptions.
n
Hm, but the shortread function, which is reported as the cause, does this:
Copy code
def create_shortread_channel(row) { // This function expects a meta map as input
    // create meta map
    def meta = row
    meta.paired = row.paired
    meta.single_end = !meta.paired
.. <stuff>
Does this not prevent concurrent modifications?
It is also really confusing to me why this would only occur in the particular case the user suggests here, as the ONT and shortread prep workflows are distinct swfs, and their inputs are:
Copy code
shortreads = ch_main
        .filter {
            it -> ((it.meta.shortread_F && it.meta.use_short_reads) || (it.meta.hic_F && it.meta.scaffold_hic)) ? true : false
        }

    ontreads = ch_main
        .filter {
            it -> (it.meta.ontreads) ? true : false
        }
Which does, of course, not prevent one sample from going into both channels, but that would imply that the 
shortreads
and 
ontreads
are both just 
ch_main
and could modify that concurrently, which should cause way more problems?
j
Copy code
def meta = row
does not create a new map - it creates a new reference to the existing map in memory. If you have:
Copy code
def meta = row
def meta2 = row
Then if you modify meta2, meta will also be modified.
n
Hm, I see, so the issue is probably: 
meta.single_end = !meta.paired
? and i should just 
def single_end = !meta.paired
?
j
or just do 
def meta = row + [:]
. The addition operation with an empty map creates a brand-new map in memory, so you can change it without affecting other references to the original.
šŸ‘ 1
or alternatively :
Copy code
def meta = row + [paired: row.paired, single_end: !row.paired]
n
I see, I guess all of this will be better with records šŸ˜‰ Thanks for the explaination
j
Records won't fix this, I don't think...
šŸ˜° 1
p
n
Probably, thanks for the link
Yeah, that was very useful, it seems the reason why this does not happen all over the place in my pipeline is because i usually do some 
+
n
And on @Jim Downieā€™s point: on 26.09.1-edge (and I think starting in 26.04.0), records are immutable, so this exact pattern throws 
UnsupportedOperationException
on the first run instead of racing (lint doesn't flag it though; the fix is 
r + record(single_end: ...)
). Plain maps in typed mode are still mutable, so it only helps once metas are records.
šŸ˜® 1
j
Oh good!
n
Thats good to know, thanks
n
It looks like in the latest edge, 26.09.2-edge, there's even a linter error on trying to mutate a record:
Copy code
Linting: <http://records_are_immutable.nf|records_are_immutable.nf>
  Error <http://records_are_immutable.nf:24:5|records_are_immutable.nf:24:5>: Record fields cannot be assigned -- records are immutable
  ā”‚  24 |     meta.single_end = !meta.paired
  ā•°     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  Nextflow linting complete!
   āŒ 1 file had 1 error
šŸŽ‰ 1
j
Nice!
n
@Jim Downie but, caveat, the immutability is shallow, List or Maps in the record can change. I was curious and tested this, and here is an ai-generated test example gist with more detail
šŸ‘ 1
n
The shallow immutability seems a bit dangerous to me. Would I have to do something like:
Copy code
channel.of(record(id: 'S1', reads: ['r1.fq', 'r2.fq'], tags: [lane: '1']))
        .map { s ->
            def copy = s + record(id: 'S1-copy') + record(reads: s.reads + [])  + record(tags: s.tags + [:])
to avoid running into that?
šŸ«£ 1
n
Copy code
def copy = s + record(id: 'S1-copy', reads: s.reads + [], tags: s.tags + [:])
// ā†’ copy.reads and copy.tags are new objects, separate from s
But I'd think you'd probably want to build new collections, never writing into them
Copy code
def changed = s + record(reads: s.reads + ['r3.fq'], tags: s.tags + [lane: '2'])
  // ā†’ changed: [r1.fq, r2.fq, r3.fq] [lane:2];  s untouched: [r1.fq, r2.fq] [lane:1]
(tested on NF 26.04, still valid on latest edge). The key thing is adding 
+ []/+ [:]
to avoid a plain copy
n
Copy code
def copy = s + record(id: 'S1-copy', reads: s.reads + [], tags: s.tags + [:])
thats at least a bit less bad than what I had, but still seems like something that is easily overlooked
j
I think in general it's less likely that you'll be appending to lists in place, and if you have maps those will quite likely be replaced by records of their own
Alternatively, use tuples instead of lists. They should be immutable too.
n
Those are reasonable suggestions. I am mainly wondering if something that will be the de-facto type for channels should exhibit this behaviour. Of course, you can argue that if you write the language you should understand it, but I do not really get what the advantage is of having this shallow. Of course I also recently ran into the same issue with maps, so maybe its just me not getting something basic
Open in Slack
PreviousNext
Powered by