search

naobservatory / mgs-workflow

MIT License
4 stars 2 forks source link

Respect given memory parameter in BBTools processes #36

Open mikemc opened 4 months ago

mikemc commented 4 months ago

See https://github.com/naobservatory/mgs-workflow/issues/29 for a description and illustration of the issue. The fix involves generating the correct bbtools/java memory parameter string from the workflow's task.memory parameter.

Applies to 'RUN:DEDUP:CLUMPIFY_PAIRED' and other processes that use BBTools scripts

willbradshaw commented 4 months ago

Similar issue also applies to other BBTools processes (grep for BBTools label or Xmx memory string

On Mon, 1 Jul 2024, 14:53 Michael McLaren, @.***> wrote:

See #29 https://github.com/naobservatory/mgs-workflow/issues/29 for a description and illustration of the issue. The fix involves generating the correct bbtools/java memory parameter string from the workflow's task.memory parameter.

— Reply to this email directly, view it on GitHub https://github.com/naobservatory/mgs-workflow/issues/36, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADWVV6UKV6Y2WBELKAPCZQ3ZKGQTNAVCNFSM6AAAAABKGDRMVWVHI2DSMVQWIX3LMV43ASLTON2WKOZSGM4DINJSG42DQMQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

mikemc commented 4 months ago

@willbradshaw a few clarification questions that will help me figure this out:

First, I notice sometimes we have params.mem appearing in the process definition, e.g. 

process KRAKEN {
    label "Kraken2"
    cpus 16
    memory "${params.mem}"
    input:

can you explain what that is doing and if it is related to task.memory?

Second, to help me determine how to translate the resources.config memory parameters to those needed by Java (e.g. 30g, 1600m), could you clarify where the format used in the resources.config for memory (e.g. "32.GB") comes from or what strings are allowed? (e.g. is "32.5GB" allowed?)

Third, is there an example you can point to where the resources.config memory parameters are being used (if the KRAKEN instance above is not one)?

If they aren't being used at all yet, then perhaps it would make sense to require that they are specified in the java format to avoid needing to translate from "32.5GB" to "32500m" or "32g" ("-XmX32.5g" is not valid)

willbradshaw commented 4 months ago

can you explain what that is doing and if it is related to task.memory?

params.mem here is an arbitrary value that's being passed to the process when the module is imported (using the addParams statement). I've called it mem in this case but I could call it anything and it would still work.

task.memory is a specific value that's declared via the memory process directive and specifies the amount of memory allocated by Nextflow to the process. In this case, params.mem is being used to allow the value of task.memory to be configured when the module is imported.

Second, to help me determine how to translate the resources.config memory parameters to those needed by Java (e.g. 30g, 1600m), could you clarify where the format used in the resources.config for memory (e.g. "32.GB") comes from or what strings are allowed? (e.g. is "32.5GB" allowed?)

Good question, I had to look this up. I think this is the relevant part of the Nextflow documentation, which states that "You can create a memory unit by adding a unit suffix to an integer, e.g. 1.GB". So I think we can safely assume it will be an integer value, with a 1:1 mapping between unit suffixes in nextflow and those in Java.

Third, is there an example you can point to where the resources.config memory parameters are being used (if the KRAKEN instance above is not one)?

As I understand it, it's being used in every process, including KRAKEN; it specifies the amount of memory that process is allowed to use. The problem in the BBTools processes isn't that the memory parameters aren't being used. Rather, it's that the process is only allocated the amount of memory specified by the relevant memory directive, but is trying to claim more than that and so failing.

mikemc commented 4 months ago

Thanks @willbradshaw , this is very helpful!

mikemc commented 4 months ago

I looked to see how https://github.com/nf-core/mag handles this issue and found this example,

    bbnorm.sh \\
        $input \\
        $output \\
        $args \\
        threads=$task.cpus \\
        -Xmx${task.memory.toGiga()}g \\
        &> ${prefix}.bbnorm.log

link

Apparently the Nextflow DSL defines a MemoryUnit class that has methods (https://www.nextflow.io/docs/latest/script.html#memoryunit) to help with the conversion, including toGiga() which is used above.

So my plan is to simply swap in -Xmx${task.memory.toGiga()}g everywhere. I'll do this once I get the full pipeline working for testing purposes.

notes for future reference

jtjvanlunenburg commented 2 months ago

Memory config can be unlimited, e.g. mem = 0, in which case task.memory is a null object and there is no toGiga method (nor a .giga prop)? How would you work around this scenario?

willbradshaw commented 1 month ago

@mikemc did you end up getting any further with this? Would you like me to include it in the list of issues for Harmon to work on in Q4?

mikemc commented 1 month ago

I consider the fix I describe above, implemented in https://github.com/naobservatory/mgs-workflow/commit/89b2c75c4dd503f4ffbde0388bc68b16c0d9d4c4, as fixing the issue and is what @evanfields has been using. I think it's an improvement over hard-setting the memory and would suggest making this fix. (This is also the behavior you'd get if you moved to using nf-core BBTools modules). I think I didn't make a pull request because you were in the midst of refactoring.

Regarding @jtjvanlunenburg 's question, I'm not really sure if unlimited memory is something you need to handle, but either way I'm guessing making the workflow support setting unlimited memory should be its own new issue and not block fixing the immediate issue.

willbradshaw commented 1 month ago

Cool, I'll add it to the list. I agree handling unlimited memory isn't a major priority.