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

Update docs for Nirvana reference disk [VS-796] [VS-531] #8170

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

mcovarr
Copy link
Collaborator

@mcovarr mcovarr commented Jan 24, 2023

No description provided.

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

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

❗ Current head 5d5908f differs from pull request most recent head e137c8c. Consider uploading reports for the commit e137c8c to get more accurate results

Additional details and impacted files
@@               Coverage Diff                @@
##             ah_var_store     #8170   +/-   ##
================================================
  Coverage                ?   86.218%           
  Complexity              ?     35514           
================================================
  Files                   ?      2191           
  Lines                   ?    166339           
  Branches                ?     17901           
================================================
  Hits                    ?    143414           
  Misses                  ?     16544           
  Partials                ?      6381           

@mcovarr mcovarr changed the base branch from ah_var_store to rc-vs-531-vds-documentation January 24, 2023 20:53
@mcovarr mcovarr marked this pull request as ready for review January 24, 2023 21:44
@mcovarr mcovarr changed the title Sketch of using Nirvana reference disk [VS-796] Update docs for Nirvana reference disk [VS-796] Jan 24, 2023
@mcovarr mcovarr changed the title Update docs for Nirvana reference disk [VS-796] Update docs for Nirvana reference disk [VS-796] [VS-531] Jan 24, 2023
Copy link
Collaborator

@gbggrant gbggrant left a comment

Choose a reason for hiding this comment

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

Have you tested this on a quickstart?

![Terra Use reference disks](Reference%20Disk%20Terra%20Opt%20In.png)

Note: this Nirvana reference image was not available in time for it to be used to create the Delta VAT. The
`GvsCreateVATfromVDS` WDL has been speculatively updated to take advantage of references loaded from an attached
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be a teeny bit more aggressive ("hey, make sure you run this on a test set before Echo, dummy!") haha

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah this dummy will be testing his changes with Quickstart data as soon as the WX-910 fixes hit production, hopefully early next week with the monolith release.

@mcovarr
Copy link
Collaborator Author

mcovarr commented Jan 24, 2023

Have you tested this on a quickstart?

I have not, I'd certainly be interested in some pairing / mobbing to do that.

Also this requires the Cromwell shell escape fixes to be available in production. I hope to merge that soon but I'm not sure these days how long after merge to Cromwell develop before code goes to production.

@gbggrant
Copy link
Collaborator

Here's an example run on a quickstart: https://app.terra.bio/#workspaces/gvs-dev/GVS%20Quickstart%20v3%20ggrant/job_history/4b83a498-5bd8-4f42-b312-1d0efcf55cfc - The ancestry file really should be installed in the quickstart storage if it is not there already, and the sites-only-vcf I generated using the hail code (a while ago).

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.

small nits, otherwise 👍🏻

scripts/variantstore/variant_annotations_table/README.md Outdated Show resolved Hide resolved
scripts/variantstore/variant_annotations_table/README.md Outdated Show resolved Hide resolved
![Terra Use reference disks](Reference%20Disk%20Terra%20Opt%20In.png)

Note: this Nirvana reference image was not available in time for it to be used to create the Delta VAT. The
`GvsCreateVATfromVDS` WDL has been speculatively updated to take advantage of references loaded from an attached
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be a teeny bit more aggressive ("hey, make sure you run this on a test set before Echo, dummy!") haha

mkdir datasources_dir
tar zxvf ~{nirvana_data_tar} -C datasources_dir ## --strip-components 2
DATA_SOURCES_FOLDER="$PWD/datasources_dir/references"
DATA_SOURCES_FOLDER="/cromwell_root/broad-public-datasets/gvs/vat-annotations/Nirvana/3.18.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the only line I'm at all worried about

Base automatically changed from rc-vs-531-vds-documentation to ah_var_store February 2, 2023 16:26
REFERENCE_DISK_MOUNT_POINT=$(lsblk | sed -E -n 's!.*(/mnt/[a-f0-9]+).*!\1!p')

# Find this one particular reference under the mount path:
GRCH38_PATH=$(find ${REFERENCE_DISK_MOUNT_POINT} -name Homo_sapiens.GRCh38.Nirvana.dat)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see if this fails if the file is not found

# version of the Nirvana reference image is deployed into Terra.

# Find where the reference disk has been mounted on this VM:
REFERENCE_DISK_MOUNT_POINT=$(lsblk | sed -E -n 's!.*(/mnt/[a-f0-9]+).*!\1!p')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see that this fails clearly if reference volume is not found (like 'Use reference disks' was not checked)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mcovarr mcovarr merged commit 5b34ade into ah_var_store Feb 15, 2023
@mcovarr mcovarr deleted the mc_nirvana_reference_disk branch February 15, 2023 12:55
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