-
Notifications
You must be signed in to change notification settings - Fork 590
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
Some changes to the ngs cohort extract files #7113
Conversation
ericsong
commented
Feb 27, 2021
- Add ngs_cohort_extract.py to the Dockerfile
- Don't swallow exceptions thrown in the python script; results in succeeding workflows even if the script fails.
create_position_table(fq_temp_table_dataset, min_variant_samples) | ||
make_new_pet_union_all(fq_pet_vet_dataset, fq_temp_table_dataset, cohort) | ||
populate_final_extract_table(fq_temp_table_dataset, fq_destination_dataset, destination_table, fq_sample_mapping_table) | ||
except Exception as err: |
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 will just eat the exception, but then continue on to print the "Final cohort extract written to..." error message (which isn't true?)
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.
the try/catch is being removed so it will no longer eat the exception. it seems counter intuitive to remove the try/catch but i'm not sure the best way to handle this. maybe add a comment that states exceptions are not being caught so the calling script will error out when an exception occurs. i feel like we could into a cycle otherwise where we try to add exception handling back in.
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.
ah -- duh, of course. My mistake.
I think the reason the exception handing was in there though was so that we would get the dump_job_stats() output even in the case of failures (so you could see how much $$/bytes were consumed before the failure).
In that case, I'd suggst we have a
try:
finally:
dump_job_stats
without explicitly catching the exception
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.
that works!
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.
pushed up that change
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.
👍
…tract python script (#7113) * add ngs to cohort extract docker * throw error if there's an exception * add back try block to print job stats * whitespace changes * remove unused var
…tract python script (#7113) * add ngs to cohort extract docker * throw error if there's an exception * add back try block to print job stats * whitespace changes * remove unused var