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

[Read_Screen] skip_screen actually skips the screen #699

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sage-wright
Copy link
Member

@sage-wright sage-wright commented Dec 24, 2024

This PR closes #677

🗑️ This dev branch should be deleted after merging to main.

🧠 Summary

For years and years, the read_screen task was advertised as skippable, and while it theoretically did not perform any screening, the task and VM was still spun up so the task itself was not skipped. This was due to my unexperienced self not really knowing what she was doing and also it being my very first addition to the Theiagen code base all those years ago.

Because of that, I am finally fixing this terribly inadequate solution and am making an improvement wherein if the skip_screen variable is set to true, the read_screen task is never even run or touched, which is much improved.

⚡ Impacted Workflows/Tasks

Workflows:

  • TheiaCoV_Illumina_PE
  • TheiaCoV_Illumina_SE
  • TheiaCoV_ONT
  • TheiaEuk_Illumina_PE
  • TheiaProk_Illumina_PE
  • TheiaProk_Illumina_SE
  • TheiaProk_ONT

Tasks:

  • task_read_screen.wdl

This PR may lead to different results in pre-existing outputs: Yes
Instead of the read_screen output variable saying PASS it will be blank, since the screen didn't run.

This PR uses an element that could cause duplicate runs to have different results: No

🛠️ Changes

⚙️ Algorithm

The read screen is actually skipped instead of started and all internal processes skipped. No more needless VM creation!

!!!!! ⚠️ if you're skipping the screen in theiaeuk, PLEASE provide an expected genome size or RASUSA will try to downsample to a genome size of 0 and that will make it very unhappy so... not sure how else to get around that lol

➡️ Inputs

None

⬅️ Outputs

None

🧪 Testing

Please admire the LACK of read_screen tasks in the following workflows where skip_screen was set to true:

AND THE PRESENCE OF when skip_screen is blank/default false:

Please note that though several of these tasks are failing, those failures are completely unrelated to this PR and are actually just due to me being a silly goose and not giving the workflow the right primer bed files or references or using bad data so all of these workflows reveal that what I did worked as intended.

Suggested Scenarios for Reviewer to Test

If you are afraid of the previous statement I made and are rightfully judgmental of my admittedly shoddy testing, you may want to rerun some of those tests that have failure marks to make them go green though I am confident they will once you give the workflow what it needs: the right primers/appropriate references/actually good data.

🔬 Final Developer Checklist

  • The workflow/task has been tested and results, including file contents, are as anticipated
  • The CI/CD has been adjusted and tests are passing (Theiagen developers)
  • Code changes follow the style guide
  • Documentation and/or workflow diagrams have been updated if applicable
    • You have updated the latest version for any affected worklows in the respective workflow documentation page and for every entry in the three workflows_overview tables.

🎯 Reviewer Checklist

  • All changed results have been confirmed
  • You have tested the PR appropriately (see the testing guide for more information)
  • All code adheres to the style guide
  • MD5 sums have been updated
  • The PR author has addressed all comments
  • The documentation has been updated

@sage-wright sage-wright marked this pull request as ready for review December 24, 2024 16:19
@sage-wright sage-wright requested a review from a team as a code owner December 24, 2024 16:19
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.

make skip screen actually skip the task
1 participant