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-567. Removing usage of ServiceAccount from CreateVat related WDLs #7974

Merged
merged 10 commits into from
Aug 10, 2022

Conversation

gbggrant
Copy link
Collaborator

@gbggrant gbggrant commented Aug 3, 2022

Removing usage of ServiceAccount from CreateVat related WDLs.

@gbggrant gbggrant marked this pull request as ready for review August 3, 2022 15:36
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

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

@@               Coverage Diff                @@
##             ah_var_store     #7974   +/-   ##
================================================
  Coverage                ?   79.218%           
  Complexity              ?     33309           
================================================
  Files                   ?      2173           
  Lines                   ?    165016           
  Branches                ?     17793           
================================================
  Hits                    ?    130723           
  Misses                  ?     28119           
  Partials                ?      6174           

# {
# localization_optional: true
# }
# }
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to just fully rip this out?

gsutil cp ~{inputFileofFileNames} ~{updated_input_files}
fi

# nothing to do - it is just reading the lines from the input - TODO - is there a better way to do this?
Copy link
Contributor

Choose a reason for hiding this comment

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

This was definitely written for the singular use case that is AoU annotation files are in GCP and I want to run them through the python script to transform them into the VAT schema

Copy link
Contributor

@RoriCremer RoriCremer Aug 3, 2022

Choose a reason for hiding this comment

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

I dont see any reason to have this task at all --and I dont think you need to worry about testing this WDL (maybe we even want to retire it?) since it will be used so rarely and if it is used again it will be in another emergency state that will probably require similar customization

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I've removed this file (as Bec mentioned, it's still in the github history).

@gbggrant
Copy link
Collaborator Author

gbggrant commented Aug 9, 2022

Passing runs of GvsCreateVat on Normal (non-AoU) here and AoU data here

Copy link
Collaborator

@mcovarr mcovarr 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, minor request for simplification.

Comment on lines 118 to 120
cp ~{vat_schema_json_filepath} ~{vat_schema_json_filename}
cp ~{variant_transcript_schema_json_filepath} ~{variant_transcript_schema_json_filename}
cp ~{genes_schema_json_filepath} ~{genes_schema_json_filename}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this just cping all these to .? Could all of this be simplified to something like

ln /data/variant_annotation_table/schema/* .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to go back to using a cp - even ln -s wouldn't work (cromwell couldn't resolve/delocalize the outputs)

scripts/variantstore/wdl/GvsCreateVAT.wdl Outdated Show resolved Hide resolved
String schema_filepath = "/data/variant_annotation_table/schema/"
String vat_schema_json_filename = "vat_schema.json"
String variant_transcript_schema_json_filename = "vt_schema.json"
String genes_schema_json_filename = "genes_schema.json"
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 awesome--thank you!

@gbggrant gbggrant merged commit 361ce37 into ah_var_store Aug 10, 2022
@gbggrant gbggrant deleted the gg_VS-567_RemoveSAFromGvsCreateVAT branch August 10, 2022 00:15
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