https://nf-co.re logo
Join Slack
Powered by
We don't have a low-memory-but-lots-of-cores label...
# team-maintainers
j
We don't have a low-memory-but-lots-of-cores label. Could we have one? Motivation: https://nfcore.slack.com/archives/C05AHQPA9A7/p1777982753023139
🙌 2
p
A new 
process_low_memory
?
Could then combine with 
process_high
to get lots of cpus and low memory
j
Can you combine labels? Is precedence obvious?
j
Yeah I agree actually @Phil Ewels, I've never been particularly happy either Splitting up for more granularity would make it much easier to optimise resources too imo
p
@Jonathan Manning yes I think so? Bit of a weird place to have it, but a search turns it up in this diff: https://github.com/nf-core/scnanoseq/blob/54005c66344e1402c489108a3620a1ee234a7c09/modules/nf-core/nanocomp/nanocomp.diff#L9-L10
👍 1
j
Sounds fine to me! Here's something to argue about https://github.com/nf-core/tools/pull/4264
✅ 1
n
Let's open an RFC. I'm fan of more control over these labels
p
Does it need an RFC to add one more label to the template? Seems kinda overkill?
j
Depends how destructive we want to be 😬
I would rather have something like a set called 
process_mem_{high,medium,low}
and then another set 
process_cpus_{high,medium,low,single}
Which you would mix and match
And then we slowly deprecate the old ones. This would then effect both tools (the template), but then also basically all the pipelines, and possibly config repos
p
Yeah that's a bigger job 😅
We need time as well
And probably 
xhigh
Is this overkill though?
The current change is a small, non-destructive addition and I feel like it achieves basically the same thing..?
Even if your suggested set is probably nicer if we were setting it up from scratch today
j
Maybe. But I sort of feel at least we should come up with a consistent structure, I worry a little we will start seeing creep of lots of different niche labels as people start adding more and more (I've been tempted too so the same a couple of times)
m
Isn't the point of the labels to work “out of the box” and let people fix resources in their own config?
j
Yes, but I think they are making our out of the box much defaults much too inefficient
p
Yeah, when stuff is 700x over provisioned by default and there's no way for us to adjust this, something seems off 😅
100 rainbow 2
j
OK, James' proposal as alternate: https://github.com/nf-core/tools/pull/4265
❤️ 1
j
I sort of feel making it more granular would help with that
@Jonathan Manning maybe through in time as well?
j
done!
p
My suggestion would be to start with adding the simple new label to the template and using it today in the #CE8SSJV3N pipeline. Then kicking off an RFC for the bigger change.
j
Fine by me
p
Though in fairness I'm about to post something in #C06TPECB95H that might make it less urgent 😆
j
Are you about to complicate my life Phil?
p
That's what I'm here for! 😆
😱 1
j
My suggestion would be to start with adding the simple new label to the template and using it today in the #CE8SSJV3N pipeline. Then kicking off an RFC for the bigger change.
Would someone care to condone this approach so it's not Seqera going rogue? I'll make the RFC in recompense. I'm keen to get Felix's new Trimgalore out because it happens to solve a wider more serious problem apart from the new-and-shininess of it.
Opinions? I'll follow the multi-pronged approach suggested if nobody yells at me soon.
j
I likey the second, the granularity is nice
p
I think everyone likes the second, the choice isn't an either / or really. More about how we get there..
this 3
j
I'm fine with adding the first label as a workaround as long as we start the process on the bigger overhaul (just not me ATM as I'm MEANT to be holidaying)
this 1
👍🏻 1
j
@Phil Ewels this what you meant by the docs update? https://github.com/nf-core/website/pull/4195
p
Yes, thanks! Though needs tweaking
j
Garr, OK, silly assumption, fixed
FWIW my way would have been better, even if reality-adjacent
😆 1
s
Copy code
A short-term, non-destructive workaround is being added in nf-core/tools#4264 (a single new process_low_memory label) so the rnaseq pipeline can ship Trim Galore v2.0 immediately.
Why not just overwrite the label?
j
So we have a sensible default on the module and none of the other bins fitted
s
this doesn't seem like a reason to make a new label
there is no hard requirement to have a label at the moment in the linting (it currently warns but doesn't fail)
and you can patch rnaseq however you like anyway
so that is not remotely blocking to rnaseq
its quite surprising that you don't have any custom cpu/memory definitions in there at the moment
(although having said that, sarek doesn't seem to have any, which I thought it did)
j
Please don't make me rework it again. The label is there now and we have a plan for a better way 🙂
p
Why not just overwrite the label?
To play devil's advocate: why bother with labels at all?
We added them for a reason - in the past every pipeline just decided its own defaults, but people asked for labels to be able to shape pipelines to match their infra
If all pipelines start ignoring labels and just setting whatever values, that ceases to work
Granted this is a bit of a simplistic case, but still..
s
Sure, that is the point of the labels, and I think the new label structure makes a lot of sense, but the adding a whole new label just for this seemed a bit overkill given that we are saying we need to overhaul it now anyyway
p
I guess.. A simple tweak would be to keep the same approach but use the naming strategy that we will likely use in the future revamp. Then it's just a partial early implementation, everyone wins..
s/process_low_memory/process_mem_low
based on the above I think..?
3 Views
Open in Slack
PreviousNext
Powered by