-
Notifications
You must be signed in to change notification settings - Fork 597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
created variable for user to specify java heap size in picard #8406
created variable for user to specify java heap size in picard #8406
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this! A few small comments, let me know if you have any questions.
@@ -25,6 +25,8 @@ workflow MitochondriaPipeline { | |||
File? ref_fasta_index | |||
File? ref_dict | |||
|
|||
String java_heap_memory = "1000m" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be more convenient to declare only within the task itself. It will still show up as an optional input in Terra, but it means fewer overall inputs that you have to thread through the main wdl.
@@ -247,6 +250,7 @@ task RevertSam { | |||
input { | |||
File input_bam | |||
String basename = basename(input_bam, ".bam") | |||
String heap_mem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would tie this in to the memory declaration in the runtime arguments as well. That way if you want to increase the memory you'll increase the machine as well as the java argument. It's also good to leave some extra memory on the machine, so it would look like the following:
Int machine_mem = 2000
Then outside of the input block you'd have:
Int java_mem = machine_mem - 1000
Then you'd use it in the java command as:
java -Xmx~{java_mem}m -jar /usr/gitc/picard.jar \
And in the runtime block change the memory to the following:
memory: machine_mem + " MiB"
You could also change the unit here to GB if you want. Here's an example of what I'm trying to say here: https://github.com/broadinstitute/gatk/blob/master/scripts/cnn_variant_wdl/cnn_variant_common_tasks.wdl#L33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meganshand sounds good, thanks for the example! just pushed the new changes, let me know what you think. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for fixing this!
due to error
Exception in thread "main" java.lang.OutOfMemoryError: Java heap space: failed reallocation of scalar replaced objects
during RevertSam step in MitochondriaPipeline.wdl @meganshand