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

Rc vs 63 vat sop documentation #7879

Merged
merged 6 commits into from
Jun 10, 2022
Merged

Conversation

RoriCremer
Copy link
Contributor

No description provided.

@RoriCremer RoriCremer force-pushed the rc-vs-63-vat-sop-documentation branch from fe992aa to b99e8be Compare June 1, 2022 16:18
@RoriCremer RoriCremer marked this pull request as ready for review June 1, 2022 16:18
@RoriCremer RoriCremer force-pushed the rc-vs-63-vat-sop-documentation branch from b99e8be to d01bf5c Compare June 1, 2022 16:19
@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

❗ No coverage uploaded for pull request base (ah_var_store@91c33df). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             ah_var_store     #7879   +/-   ##
================================================
  Coverage                ?   86.290%           
  Complexity              ?     35190           
================================================
  Files                   ?      2170           
  Lines                   ?    164888           
  Branches                ?     17786           
================================================
  Hits                    ?    142282           
  Misses                  ?     16281           
  Partials                ?      6325           

@RoriCremer RoriCremer force-pushed the rc-vs-63-vat-sop-documentation branch from 1899d7b to 06e9b73 Compare June 6, 2022 14:00
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.

Unfortunately have to leave these notes here, since the text in question isn't part of the PR diff; apologies if my meaning isn't clear.

They are cleaned up after 24 hours, but this code needs to be tweaked so that you can’t get into a state where duplicates are created here. The real question here is going to be, is there a use case that we might want to run where adding to a VAT that was created say weeks ago is beneficial, but given that calculations occur on a sample summing level, this seems unlikely.

Do we still need this? Seems like it belongs in a ticket (or two) and not documentation.

To check that all of the shards have successfully made it past the first of the sub workflow steps (the most complicated and likely to fail) they will need to be annotated / transformed into json files and put here: gsutil ls [output_path]/annotations/ | wc -l

This (mostly "they will need to be annotated / transformed into json files and put here") makes it sound like the person running the WDL needs to do something else, instead of this being a way to check on the success of the WDL.

Lastly, is the TSV file the only deliverable? Is there anything else that needs to be communicated...etc.? Either way, can this be explicitly added to this doc?

scripts/variantstore/variant_annotations_table/README.md Outdated Show resolved Hide resolved
scripts/variantstore/wdl/GvsCreateVATAnnotations.wdl Outdated Show resolved Hide resolved
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.

Sorry to harp on this. I think you should change

The pipeline takes in a jointVCF and outputs a table in BigQuery.

to something like

The pipeline takes in jointVCFs (and index files), creates a table in BigQuery, and outputs a bgzipped TSV file containing the contents of that table, which is the only deliverable.

to make it clear what the final "thing" is that is created and delivered to the client/user.

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.

looks good 👍🏻

@RoriCremer RoriCremer merged commit 0be4453 into ah_var_store Jun 10, 2022
@RoriCremer RoriCremer deleted the rc-vs-63-vat-sop-documentation branch June 10, 2022 18:09
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.

4 participants