use dynamic resource allocation; disable timeline for now; added some… #96

BioinfoTongLI · 2023-09-16T20:21:36Z

… default parameters

codecov-commenter · 2023-09-16T20:23:55Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (c34922d) 36.28% compared to head (0fcf094) 36.27%.
Report is 25 commits behind head on dev.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev      #96      +/-   ##
==========================================
- Coverage   36.28%   36.27%   -0.02%     
==========================================
  Files          14       14              
  Lines         722      725       +3     
==========================================
+ Hits          262      263       +1     
- Misses        460      462       +2

Files	Coverage Δ
bin/integrate_anndata.py	`0.00% <ø> (ø)`
bin/integrate_image.py	`0.00% <ø> (ø)`
bin/process_merscope.py	`23.68% <ø> (ø)`
bin/build_config.py	`0.00% <0.00%> (ø)`
bin/process_h5ad.py	`85.59% <0.00%> (ø)`
bin/process_spaceranger.py	`54.28% <60.00%> (-0.94%)`	⬇️
bin/process_xenium.py	`24.63% <0.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

prete · 2023-10-19T21:58:37Z

This is not how dynamic process allocation is supposed to be used:

    cpus = { 1 * task.attempt * task.attempt}
    memory = { 10.GB * task.attempt * task.attempt}

That would force process to effectively fail at least one to get the right number of resources for the 4/20G profiles. Moreover, it also hides the number of cpus/mem that's required by each process by using task.attempt**2

I highly advice having labeled processes (not named) to be able to provide generic groups of resources like this:

process {
    withLabel: small {
        cpus = 1
        memory = 4.GB
    }
    withLabel: large {
        cpus = 4
        memory = 20.GB
    }
}

...and then go back to the processes and add the right labels.

Should you want to make those 'dynamic' too you could do something like:

process {
    withLabel: small {
        cpus = 1
        memory = { 4.GB * task.attempt }
    }
    withLabel: large {
        cpus = 4
        memory = { 20.GB * task.attempt }
    }
}

… require more cpus for processing. Will change if causing issues.

prete · 2023-10-19T22:49:35Z

nextflow.config

+        memory = { 2.GB * task.attempt }
+    }
+    withLabel: large {
+        cpus = { 4 * task.attempt }


Is the cpu number supposed to increase on each attempt?

for the image-to-zarr. Yes. bf2raw can use as many cores as possible. For a large image. I would want it go faster with more core. But for the other process Generate_image. It's not really true.

actually 20Gb might be too much as a starting point. Most laptops have <16Gb. I may just start with 10Gb memory.

I don't think people using laptops will have an issue with that because they will run it using 'local executor' 🤔

Hmm... In the current version, I think it applies to all users since no profile-specific allocation is in-place.
Or you mean it will omit this label when it's local?

I'm pretty sure the local executor won't use those and will only honour the config for executor{ memory=..., cpu=...}
so they should be fine to keep as-is... I can double check latter with a tiny example if you want to.

would be great. Cause it is reading it with local on my VM. Here's my dummy script

#!/usr/bin/env/ nextflow // Copyright © 2023 Tong LI <[email protected]> nextflow.enable.dsl=2 process test { label 'huge' script: """ echo 'hello world' """ } workflow { test() } ❯ more nextflow.config process { withLabel: 'huge' { memory = 500.Gb } Process requirement exceeds available memory -- req: 500 GB; avail: 336.4 GB

I think the (dynamic) resource allocation has to be optional. The numbers are very dependant on the environment being used, and users might be frustrated that they need to iterate through cycles they know will fail when they already know what needs to be allocated.

davehorsfall

I will continue to bring this PR up to date with recent changes and start to address comments in #104.

davehorsfall

We need to document how resource allocation is used and how the dynamically resource allocation can be used. The sanger_lsf profile also needs to be documented if we are going to keep it in the config.

davehorsfall · 2023-11-07T11:15:31Z

main.nf

@@ -109,6 +109,8 @@ def mergeArgs (stem, data_type, args) {
 process image_to_zarr {
    tag "${image}"
    debug verbose_log
+    label 'big_mem'


I've split the label for memory and cpu to give more granular control over resource allocation.

davehorsfall · 2023-11-07T11:15:58Z

nextflow.config

+      out_dir = './output'
+      report_dir = './reports'
+      custom_config_version = 'master'
+      custom_config_base = "https://raw.githubusercontent.com/nf-core/configs/${params.custom_config_version}"


Are these lines (4-5) needed at the moment?

davehorsfall · 2023-11-07T11:19:44Z

nextflow.config

+
+      withLabel: big_cpu {
+            cpus = { 
+                  4 * task.attempt 


@dannda and I had a quick discussion about this. Staring at 4 cpus, and running up by 4 through each iteration feels a lot? It may cause a problem when the number being requested can't be allocated?

davehorsfall · 2023-11-07T11:23:50Z

nextflow.config

+
+      withLabel: big_mem {
+            memory = { 
+                  10.GB * task.attempt 


The documentation gives a compelling argument for dynamic resource allocation feature. However, environmentally, it would be better to not unnecessarily burning through resources to find a shape that works when it is likely the person executing the job has a good idea of what is required. Perhaps we can add to the documentation to describe how the resource allocation can be optionally used?

dannda · 2023-11-07T14:30:35Z

nextflow.config

+      conda { 
+            conda.enabled = true
+            process {
+                  conda = "$baseDir/envs/environment.yaml"


Just noting here that docs should match these settings.

use dynamic resource allocation; disable timeline for now; added some…

b19c082

… default parameters

BioinfoTongLI mentioned this pull request Oct 19, 2023

nextflow.config #104

Open

use label to control resource allocation; assuming larger images will…

f7d5aeb

… require more cpus for processing. Will change if causing issues.

prete reviewed Oct 19, 2023

View reviewed changes

BioinfoTongLI and others added 2 commits October 19, 2023 23:49

Merge branch 'dev' into nf-config-update

afe1809

20 Gb memory is too much for a laptop. start by 10

0fcf094

davehorsfall reviewed Nov 7, 2023

View reviewed changes

davehorsfall added 3 commits November 7, 2023 10:06

Merge branch 'dev' into nf-config-update

c582563

update config to work towards close #104

f601799

align sanger_lsf profile with current version

6405c69

davehorsfall requested changes Nov 7, 2023

View reviewed changes

dannda reviewed Nov 7, 2023

View reviewed changes

dannda added 2 commits November 21, 2023 14:10

add labels to multimodal

a51371d

set verbose logging to false

65215a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use dynamic resource allocation; disable timeline for now; added some… #96

use dynamic resource allocation; disable timeline for now; added some… #96

BioinfoTongLI commented Sep 16, 2023

codecov-commenter commented Sep 16, 2023 •

edited

Loading

prete commented Oct 19, 2023 •

edited

Loading

prete Oct 19, 2023

BioinfoTongLI Oct 19, 2023

BioinfoTongLI Oct 19, 2023

prete Oct 20, 2023

BioinfoTongLI Oct 20, 2023

prete Oct 20, 2023

BioinfoTongLI Oct 21, 2023

davehorsfall Nov 7, 2023

davehorsfall left a comment •

edited

Loading

davehorsfall left a comment

davehorsfall Nov 7, 2023

davehorsfall Nov 7, 2023

davehorsfall Nov 7, 2023

davehorsfall Nov 7, 2023

dannda Nov 7, 2023

use dynamic resource allocation; disable timeline for now; added some… #96

Are you sure you want to change the base?

use dynamic resource allocation; disable timeline for now; added some… #96

Conversation

BioinfoTongLI commented Sep 16, 2023

codecov-commenter commented Sep 16, 2023 • edited Loading

Codecov Report

prete commented Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davehorsfall left a comment • edited Loading

Choose a reason for hiding this comment

davehorsfall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 16, 2023 •

edited

Loading

prete commented Oct 19, 2023 •

edited

Loading

davehorsfall left a comment •

edited

Loading