-
Notifications
You must be signed in to change notification settings - Fork 596
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
VS-361 Add GvsWithdrawSamples wdl #7765
Conversation
cat log_message.txt | sed -e 's/Number of affected rows: //' > rows_updated.txt | ||
typeset -i rows_updated=$(cat rows_updated.txt) | ||
|
||
echo "did not update $num_samples rows - only updated $rows_updated" |
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.
seems like this should only be executed if the counts are mismatched (like the other copy of this on line 82)
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.
Yeah, that's some leftover debugging code.
|
||
echo "did not update $num_samples rows - only updated $rows_updated" | ||
|
||
if [ $num_samples -ne $rows_updated ]; then |
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 really like having this check...I wish there was a cleaner way of doing this with the bq
command line tool but if there is I haven't found it... 😞
String sample_info_table | ||
Array[String] sample_names | ||
|
||
# runtime |
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.
Looking at the other WDLs in this repo this comment appears to be marking inputs that are used in expressions in the runtime
section of tasks. Assuming I have that right (which I might not), # runtime
would not apply to these two variables.
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.
Yeah, that's true. I'll remove the comment.
exit 0 | ||
fi | ||
|
||
export GATK_LOCAL_JAR=~{default="/root/gatk.jar" gatk_override} |
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.
It seems gatk_override
isn't actually needed in this WDL and could be removed (there are no GATK invocations here).
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.
Thanks.
String? service_account_json_path | ||
} | ||
|
||
String sample_info_table = "sample_info" |
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.
Is there a use case that benefits from sample_info_table
being a declaration here (and an input in WithdrawSamples
) or could its usage just be hardcoded sample_info
in WithdrawSamples
?
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 wondered about that too - I followed the pattern in another wdl, but I agree that it seems unlikely. I'll remove it as an input.
runtime { | ||
docker: "us.gcr.io/broad-gatk/gatk:4.1.7.0" | ||
memory: "3.75 GB" | ||
disks: "local-disk " + 10 + " HDD" |
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.
can this be just one string rather than a concatenation of literals?
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.
Yeah - would have been a useful pattern if there was a variable for 10
Update to latest gatk docker
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.
LGTM 👍🏻
No description provided.