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-1159 - Enhance GVSWithdrawSamples #8599

Merged
merged 20 commits into from
Dec 5, 2023

Conversation

gbggrant
Copy link
Collaborator

@gbggrant gbggrant commented Dec 4, 2023

This PR updates GvsWithdrawSamples to:

  1. Use a "true" temporary table (uniquely named, goes away after 24 hours)
  2. Check if there are any samples in the uploaded list of samples to withdraw that are NOT in the existing sample_info table. Fail and print out the list of samples if so.
  3. Added a boolean flag to allow the user to pass the workflow if condition 2 is true.

A) Example Run with 0 samples withdrawn and 0 new samples (samples in the withdrawn file that aren't in sample_info)
B) Example Run with 1 sample withdrawn and 0 new samples (samples in the withdrawn file that aren't in sample_info)
C) Example Run with 1 sample withdrawn and 1 new sample (sample in the withdrawn file that wasn't in sample_info). This run was run with the override flag allowing it to pass
D) Example Run with 1 sample withdrawn and 1 new sample (sample in the withdrawn file that wasn't in sample_info). This run failed (as intended)

@gbggrant gbggrant marked this pull request as ready for review December 4, 2023 20:05
@gbggrant gbggrant requested review from rsasch and mcovarr December 4, 2023 20:05
scripts/variantstore/wdl/GvsWithdrawSamples.wdl Outdated Show resolved Hide resolved
scripts/variantstore/wdl/GvsWithdrawSamples.wdl Outdated Show resolved Hide resolved
LEFT JOIN `~{dataset_name}.sample_info` sample_info ON sample_info.sample_name = callset.sample_name
WHERE sample_info.sample_name IS NULL' > new_samples.txt

echo "The following samples in the uploaded list have NOT been loaded into ~{dataset_name}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I worry this message might get missed...could this condition be checked first and generate an error if there are samples that need to be loaded?

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 this too, but the AC are to let it pass if there are samples that have not been loaded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But I kind of feel that it should fail if there are samples in the file that haven't been uploaded.

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.

My only request is that you update the docs (https://github.com/broadinstitute/gatk/blob/ah_var_store/scripts/variantstore/docs/aou/AOU_DELIVERABLES.md) to include what to do with the outputs (e.g. ingest the missing samples, make sure the number of samples that are withdrawn is as expected).

@gbggrant gbggrant requested a review from rsasch December 4, 2023 20:56
@gbggrant gbggrant requested a review from mcovarr December 4, 2023 22:29
@gbggrant gbggrant requested a review from RoriCremer December 5, 2023 20:27
Copy link

@koncheto-broad koncheto-broad left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding the flag to control the behavior

@gbggrant gbggrant merged commit b9d6afb into ah_var_store Dec 5, 2023
20 checks passed
@gbggrant gbggrant deleted the gg_VS-1159_EnhanceGvsWithdrawSamples branch December 5, 2023 22:32
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.

4 participants