Remove meta clone everywhere for improved resume stability #332
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…reserve metamaps across channels
|
|
I'll need to watch the video. On first recollection, I'm not convinced that |
|
Eek then I'm completely confused:
At least here it says that the original map is unaltered:
https://docs.groovy-lang.org/latest/html/groovy-jdk/java/util/Map.html#subMap(java.lang.Object[])
…---
Sent from my mobile
On Fri, 21 Jul 2023, 11:51 Moritz E. Beber, ***@***.***> wrote:
I'll need to watch the video. On first recollection, I'm not convinced
that subMap is anything else than a clone.
—
Reply to this email directly, view it on GitHub
<#332 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEI6MT4B5JRPE4WXWKNDAF3XRJGLDANCNFSM6AAAAAA2RL6RKE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@Midnighter A nice summary from @sateeshperi after the talk! |
|
Here is a quick counter example, which shows that a = [x: [1, 2, 3], y: 3, z: '4']
b = a.subMap(['x', 'z'])
println "a.x ${a.x} == b.x ${b.x}" // a.x [1, 2, 3] == b.x [1, 2, 3]
a.x.add(4)
println "a.x ${a.x} == b.x ${b.x}" // a.x [1, 2, 3, 4] == b.x [1, 2, 3, 4]So in my opinion this will not change the situation. However, normally, just cloning maps by whatever chosen method is enough to avoid resume issues. So either, there is something happening that we're not aware of, or the pipeline is modifying a mutable meta map element in place. Both are bad options for us. |
|
Gah then I'm even more confused then... I was having problems on mag with resume which was solved when I switched to the subMap method... |
|
Wait... I may have been overzealous with the subMaps here now I look at it... I'll check tomorrow |
|
Right sorry, to be clear: is the + or - that does the deep copy. I have been using the subMap stuff to get the keys/value pairs themselves. I think in most cases I've used everything in the context of modification but I'll double check tomorrow |
No, that is also equivalent to clone. Sorry, if I am adding to your confusion. a = [x: [1, 2, 3], y: '4']
b = a + [y: '5']
println "a.x ${a.x} == b.x ${b.x}" // a.x [1, 2, 3] == b.x [1, 2, 3]
a.x.add(4)
println "a.x ${a.x} == b.x ${b.x}" // a.x [1, 2, 3, 4] == b.x [1, 2, 3, 4]The |
jfy133
commented
Jul 24, 2023
|
@Midnighter please see the thread linked in the (updated )OP . My understanding on the technical aspects was wrong but the outcome is still 'enough' 😅 |
There was a problem hiding this comment.
So, we don't like clone() because it creates maps that point back to the original, while .subMap(), + and - all lead to properly new maps, though string values etc within them remain pointers to those in the original maps, which we're okay with. Have I got that right?
|
@pinin4fjords I think so... at least I definitely had to leave in one specific |
pinin4fjords
approved these changes
Jul 25, 2023
maxulysse
reviewed
Jul 25, 2023
maxulysse
reviewed
Jul 25, 2023
JoseEspinosa
approved these changes
Jul 25, 2023
There was a problem hiding this comment.
Also from what I understand in the comments and the nice summary in slack, LGTM 🚀
Of course after addressing @maxulysse comments 😉
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because of this: https://www.youtube.com/watch?v=A357C-ux6Dw&t=879s
Essentially allows modification of maps without breaking sister maps in other channels.
Further explanation here: https://nfcore.slack.com/archives/C01LNCGJBMH/p1690207154503529?thread_ts=1684849566.995129&cid=C01LNCGJBMH
I've opted for consistency throughput the pipeline as the use of
subMapis a bit more powerful as you can make different maps directly in addition to dropping parameters.(Note that subMap doesn't make a deep copy as it thought it did/posted originally, but using the
+-subMapcombination is good enough)I've tested all channels before and after with dumps to make sure they look OK 👍 (i.e., lane merging, fastp, malt, kraken/bracken)
PR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).