Skip to content
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

Merged
merged 10 commits into from
Apr 7, 2022

Conversation

gbggrant
Copy link
Collaborator

@gbggrant gbggrant commented Apr 7, 2022

No description provided.

@gbggrant gbggrant requested a review from rsasch April 7, 2022 00:43
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"
Copy link
Collaborator

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)

Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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}
Copy link
Collaborator

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@gbggrant gbggrant requested a review from mcovarr April 7, 2022 13:20
String? service_account_json_path
}

String sample_info_table = "sample_info"
Copy link
Collaborator

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?

Copy link
Collaborator Author

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"
Copy link
Collaborator

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?

Copy link
Collaborator Author

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
Copy link

@rsasch rsasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏻

@gbggrant gbggrant merged commit e99e842 into ah_var_store Apr 7, 2022
@gbggrant gbggrant deleted the gg_VS-361_AddGvsWithdrawSamples branch April 7, 2022 19:05
This was referenced Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants