-
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
support for multiple PET/VETs #6969
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.
Couple of small questions, but looks good 👍
preemptible_tries = preemptible_tries, | ||
docker = docker_final | ||
input: | ||
done = CreateImportTsvs.done[0], |
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.
Does Cromwell wait until the full scatter of CreateImportTsvs
is complete? Or will this run as soon as the first shard is done?
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 didn't change this -- so I assumed it was doing the right thing before (it does seem to wait) but I wonder if instead if "done" should be an array of all the inputs? @ahaessly how did you arrive at this solution?
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.
@kcibul i think that when i used done, it was just a true/false that was set at the end of a task. i don't think i used it after a scatter. but i think passing the array would be safest. i guess we would change the input param to be an Array of String? (if it's an array of file, we should set the meta param for localizationOptional to true so it doesn't try to localize them all)
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 I agree, we should pass in the whole Array.
@@ -178,10 +207,6 @@ task LoadData { | |||
|
|||
command <<< | |||
set -x | |||
set +e | |||
# make sure dataset exists | |||
bq ls --project_id ~{project_id} ~{dataset_name} > /dev/null |
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.
Does this still fail with a nice error message if there's no dataset? (Nice enough that I'll realize I forgot to make one)
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 removed this because later on when you go to make the table, it fails will a clear error message if the dataset doesn't exist
memory: "4 GB" | ||
disks: "local-disk 10 HDD" | ||
memory: "10 GB" | ||
disks: "local-disk 1000 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.
Why does loading the data need so much disk? Isn't it just importing what's in a separate google bucket?
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.
no idea -- I changed it to match Terra, but you're right. I'm going to change it back :D
* support for multiple PET/VETs * fixed to use latest GATK JAR, eliminate odd java NoSuchMethod error * PR feedback
* support for multiple PET/VETs * fixed to use latest GATK JAR, eliminate odd java NoSuchMethod error * PR feedback
* support for multiple PET/VETs * fixed to use latest GATK JAR, eliminate odd java NoSuchMethod error * PR feedback
* support for multiple PET/VETs * fixed to use latest GATK JAR, eliminate odd java NoSuchMethod error * PR feedback
* support for multiple PET/VETs * fixed to use latest GATK JAR, eliminate odd java NoSuchMethod error * PR feedback
No description provided.