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

implement GVS ID assignment #7355

Merged
merged 9 commits into from
Aug 11, 2021
Merged

implement GVS ID assignment #7355

merged 9 commits into from
Aug 11, 2021

Conversation

ahaessly
Copy link
Contributor

@ahaessly ahaessly commented Jul 19, 2021

Assign Ids to samples.
Update the sample info table.
Update import wdl to remove the generation of the sample_info.tsv files

@ahaessly ahaessly marked this pull request as ready for review July 22, 2021 15:03
@gatk-bot
Copy link

Travis reported job failures from build 35222
Failures in the following jobs:

Test Type JDK Job ID Logs
integration openjdk11 35222.12 logs

Copy link
Contributor

@kcibul kcibul left a comment

Choose a reason for hiding this comment

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

just a few comments, the id assignment part looks good!

workflow GvsAssignIds {

input {
Array[String] external_sample_names
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be more usable as a File that we then do a read_lines on? Or have both options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we currently get this from the data model. is there a reason someone would use a file instead?

echo "project_id = ~{project_id}" > ~/.bigqueryrc

# create the lock table
bq --project_id=~{project_id} mk ~{dataset_name}.metadata_lock
Copy link
Contributor

Choose a reason for hiding this comment

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

just checking, this script exits with an error of the metadata_lock table already exists?

Copy link
Contributor

Choose a reason for hiding this comment

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

also -- maybe we should name this differently now that this isn't going into the "metadata" table? sample_info_lock or sample_id_assignment_lock?

Copy link
Contributor

Choose a reason for hiding this comment

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

also also… I'm surprised that this doesn't have to specify the schema when the table is created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will explicitly check whether the table exists and exit with error if it does.
renaming to sample_id_assignment_lock
adding schema when creating

scripts/variantstore/wdl/GvsImportGenomes.wdl Outdated Show resolved Hide resolved
scripts/variantstore/wdl/GvsImportGenomes.wdl Outdated Show resolved Hide resolved
call GetMaxTableId {
input:
sample_map = sample_map
if (defined(sample_map)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do we need the legacy mode for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will leaving this in be helpful for anyone? I wasn't sure if people had tests that used a sample map file. Although, this won't update the sample_info table anymore, so maybe I should just remove it. If anyone else has an opinion, let me know.

@ahaessly ahaessly merged commit 15bbb08 into ah_var_store Aug 11, 2021
@ahaessly ahaessly deleted the ah_generate_id branch August 11, 2021 17:43
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