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

[Merlin_magic; Sonneityping] harden workflow against failures due to sonnei typing disagreement #747

Merged
merged 4 commits into from
Feb 7, 2025

Conversation

awh082834
Copy link
Contributor

@awh082834 awh082834 commented Feb 4, 2025

This PR closes #600

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

🧠 Summary

Samples that would return percent_coverage < 90 would not produce indices that were required from the parser parse_mykrobe_predict.py housed within the mykrobe container. This would lead to mislabeled or dubiously characterized samples failing at task_sonneityping. Commit 7d18a7c of sonneitypingcommitted by Sage solves this issue along with a typo correction. This PR implements this commit into a new docker container, us-docker.pkg.dev/general-theiagen/staphb/mykrobe:0.12.1-parser-7d18a7c along with error handling to still deal with the possibility of missing output from mykrobe. I do not foresee this occurring with the new fix to the parser in place as it will report at "not sonnei" but I have kept the error handling present.

⚡ Impacted Workflows/Tasks

task_sonneityping.wdl
wf_merlin_magic.wdl
TheiaProk_PE
TheiaProk_SE
TheiaProk_ONT

This PR may lead to different results in pre-existing outputs: No
Samples already with a classification of Sonnei from task_sonneityping will not be changed, only samples that would fail will now have a classification of "not sonnei" returned.

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

🛠️ Changes

Docker image changed to updated parser version.

Error Handling:
A check added to check for sonneityping_predictResults.tsv with debug messages hinting at if the file was found or not.
If file is not found the necessary output text files are returned as empty, per the slack discussion, and the python code block is skipped.
File outputs have been made optional to also handle the event of a missing output.

⚙️ Algorithm

Docker image changed to pull newest commit of parse_mykrobe_predict.py

➡️ Inputs

us-docker.pkg.dev/general-theiagen/staphb/mykrobe:0.12.1 -> us-docker.pkg.dev/general-theiagen/staphb/mykrobe:0.12.1-parser-7d18a7c

⬅️ Outputs

File outputs are now optional outputs.

File sonneityping_mykrobe_report_csv = "~{samplename}.mykrobe.csv" -> File? sonneityping_mykrobe_report_csv = "~{samplename}.mykrobe.csv"
File sonneityping_mykrobe_report_json = "~{samplename}.mykrobe.json" -> File? sonneityping_mykrobe_report_json = "~{samplename}.mykrobe.json"
File sonneityping_final_report_tsv = "~{samplename}.sonneityping.tsv" -> File? sonneityping_final_report_tsv = "~{samplename}.sonneityping.tsv"

🧪 Testing

Each of the affected workflows was tested using non sonnei and sonnei sequences.

TheiaProk_PE
TheiaProk_SE
TheiaProk_ONT

TheiaProk_PE; Group of samples cited as issues in #600
Instead of failing, samples display "not sonnei" in sonneityper output.

Suggested Scenarios for Reviewer to Test

🔬 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 "Last Known Changes" field for any affected workflows in the respective workflow documentation page and for every entry in the three workflows_overview tables to be the tag for the next upcoming release. If you do not know the tag, please put "vX.X.X"

🎯 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

…utput is not produced but mykrobe runs to completion; debug messages added
@awh082834 awh082834 marked this pull request as ready for review February 4, 2025 20:34
@awh082834 awh082834 requested a review from a team as a code owner February 4, 2025 20:34
Copy link
Member

@sage-wright sage-wright left a comment

Choose a reason for hiding this comment

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

Code changes look good. I'm going to hold off on merging this until the docker image is merged to staphb for continuity.

Testing here and here and even when forcing to run sonneityping on non-sonnei it works as desired.

Well done 🎉 ⭐

@sage-wright sage-wright self-assigned this Feb 5, 2025
@sage-wright
Copy link
Member

one last test to confirm new docker didn't break everything accidentally here

Copy link
Member

@sage-wright sage-wright left a comment

Choose a reason for hiding this comment

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

Andrew's test here showed it worked! 🎉
⭐ well done

@sage-wright sage-wright merged commit 0f41d6b into main Feb 7, 2025
7 checks passed
@sage-wright sage-wright deleted the awh-merlinmagic-dev branch February 7, 2025 21:20
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.

[merlin_magic] harden workflow against failures due to sonnei typing disagreement
2 participants