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

TSPS-222 Updates for warp setup #203

Merged
merged 9 commits into from
Feb 26, 2025
Merged

Conversation

mmorgantaylor
Copy link
Collaborator

@mmorgantaylor mmorgantaylor commented Feb 19, 2025

Description

Note none of this is used in the warp tests directly.

  • Update CompareVcfs test wdl to match warp test
  • Add LiftoverVcfs wdl here (moved over from warp branch)

Jira Ticket

In support of work in https://broadworkbench.atlassian.net/browse/TSPS-222

Checklist (provide links to changes)

  • Updated external documentation (if applicable)
  • Updated internal documentation (if applicable)
  • Planned non patch version bump (if applicable)
  • Updated CLI PR (if applicable)

@mmorgantaylor mmorgantaylor changed the title TSPS-222 Update CompareVcfs test wdl to match warp test TSPS-222 Updates for warp setup Feb 24, 2025
@mmorgantaylor mmorgantaylor marked this pull request as ready for review February 25, 2025 18:59
Copy link
Collaborator

@jsotobroad jsotobroad left a comment

Choose a reason for hiding this comment

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

couple comments about preemptibles/max retries

disks: "local-disk ${disk_size_gb} SSD"
memory: "64 GiB"
memory: "${memory_gb} GiB"
preemptible: 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

i would say we prob dont want preemptibles here. this is something we prob just want to run asap would be my guess unless it runs super fast (which i dont expect)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, though remember this is effectively a copy of the actual test warp does, so this one will only be run manually in which case i'd think we care less about ASAP and wouldn't mind the $ savings

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 can make it an input to the wdl so that the user can decide

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea if its comparing large vcfs most likely being preemptible wouldnt be a cost saving. being parameterized sounds good

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good and i'll set the default to 0 (here and in the other one)

--MAX_RECORDS_IN_RAM 100000

# compress vcf - this creates a file with .gz suffix
bgzip ~{output_basename}.liftedover.vcf
Copy link
Collaborator

Choose a reason for hiding this comment

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

huh can gatk (picard) really not output a block gzipped vcf? crazy

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 real dumb

memory: "~{mem_gb} GiB"
cpu: "1"
disks: "local-disk ~{disk_size} HDD"
maxRetries: max_retries
Copy link
Collaborator

Choose a reason for hiding this comment

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

same thing here, do we want max_retries and preemptibles here?

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'm fine with it since they're inputs to the wdl that can easily be overridden by the person running it

version 1.0

# Liftover VCFs from hg19 to hg38
workflow LiftoverVcfs {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be in the scientific validation folder instead? I dont think it belongs in the testing folder at least.

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 it sort of doesn't belong anywhere, but happy to move to scientific validation - agree that makes more sense than testing

@jsotobroad
Copy link
Collaborator

if you wanna be extra awesome you can add a readme to the testing folder like the other two folders in this subdir have 😄

@mmorgantaylor mmorgantaylor merged commit 34d158b into main Feb 26, 2025
15 checks passed
@mmorgantaylor mmorgantaylor deleted the TSPS-222_update_compare_vcfs branch February 26, 2025 19:44
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.

2 participants