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

pangolin_update wf bug fix and hiding one input param related to Flu and organism_parameters subwf #415

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

kapsakcj
Copy link
Contributor

@kapsakcj kapsakcj commented Apr 16, 2024

This PR closes #.

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

🧠 Aim, Context and Functionality

  1. Pangolin_Update workflow: We missed deleting the nextclade_dataset_reference_input from org param call block when updating to nextclade V3 in Upgrade to nextclade v3 & update default dataset tags #375 . This caused an error in Terra since the workflow was non-functional and did not pass miniwdl check or womtools validate

  2. Additionally, for theiacov_ONT and TheiaCoV_Illumina_PE workflows: added gene_locations_bed_file to org param call blocks to hide optional input from user. This input should be hidden from the user in Terra because it will not be used even if a File is provided by the user.

🛠️ Impacted Workflows/Tasks & Changes Being Made

This will affect the behavior of the workflow(s) even if users don’t change any workflow inputs relative to the last version : No

Running this workflow on different occasions could result in different results, e.g. due to use of a live database, "latest" docker image, or stochastic data processing : No

📋 Workflow/Task Step Changes

🔄 Data Processing

Nothing has changed, these are just workflow level Input changes

Docker/software or software versions changed: No

Databases or database versions changed: No

Data processing/commands changed: No

File processing changed: No

Compute resources changed: No

➡️ Inputs

  1. for theiacov_ONT and TheiaCoV_Illumina_PE workflows: added gene_locations_bed_file to org param call blocks to hide optional input from user
  2. nextclade_dataset_reference_input from org param call block for Pangolin_Update workflow

⬅️ Outputs

N/A

🧪 Testing

Test Dataset

Pangolin_Update test: 4 sars-cov-2 samples

TheiaCoV_Illumina_PE and ONT: Not sure? Perhaps test with Flu samples through both?

Commandline Testing with MiniWDL or Cromwell (optional)

N/A

Terra Testing

  1. Pangolin_Update test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/8ef17805-b8d8-4094-9018-401cf7b53e7e

workflow ran successfully:
image

  1. TheiaCoV_Illumina_PE: https://app.terra.bio/#workspaces/theiagen-validations/PHB_Validation_nextcladeV3testing/job_history/521e50b0-db79-4396-b401-6a350def0a0b ⚠️ Just need to check that the workflows ran successfully Workflows ran successfully ✅
  2. TheiaCoV_ONT: https://app.terra.bio/#workspaces/theiagen-validations/PHB_Validation_nextcladeV3testing/job_history/723f8545-f8df-4eb8-9166-c829bfcc19fd ⚠️ Just need to check that the workflows ran successfully Workflows ran successfully ✅

For both of these workflows ⬆️ , the gene_locations_bed_file input has been hidden from the user, as expected:
image

Suggested Scenarios for Reviewer to Test

For pangolin_update, any sars-cov-2 samples that have been previously run through Pangolin (via TheiaCoV workflows)

Likely test Flu samples through ILMN PE and ONT workflows

Theiagen Version Release Testing (optional)

  • Will changes require functional or validation testing (checking outputs etc) during the release? functional
  • Do new samples need to be added to validation datasets? If so, upload these to the appropriate validation workspace Google bucket (). Please describe the new samples here and why these have been chosen. No
  • Are there any output files that should be checked after running the version release testing? No

🔬 Final Developer Checklist

  • The workflow/task has been tested locally and results, including file contents, are as anticipated
  • The workflow/task has been tested on Terra and results, including file contents, are as anticipated
  • The CI/CD has been adjusted and tests are passing (to be completed by Theiagen developer)
  • Code changes follow the style guide

🎯 Reviewer Checklist

  • All impacted workflows/tasks have been tested on Terra with a different dataset than used for development
  • All reviewer-suggested scenarios have been tested and any additional
  • All changed results have been confirmed to be accurate
  • All workflows/tasks impacted by change/s have been tested using a standard validation dataset to ensure no unintended change of functionality
  • All code adheres to the style guide
  • MD5 sums have been updated
  • The PR author has addressed all comments

🗂️ Associated Documentation (to be completed by Theiagen developer)

  • Relevant documentation on the Public Health Resources "PHB Main" has been updated
  • Workflow diagrams have been updated to reflect changes

…k and removed non-existent nextclade_dataset_reference_input from org param call block. theiacov_ONT and ILMN PE: added gene_locations_bed_file to org param call blocks to hide optional input from user
@kapsakcj kapsakcj marked this pull request as ready for review April 16, 2024 18:53
@cimendes cimendes self-requested a review April 16, 2024 19:02
@cimendes
Copy link
Member

cimendes commented Apr 17, 2024

@cimendes
Copy link
Member

Managed to import this branch without issue on Terra (failed with version in main) for Pangolin_Update_PHB

@cimendes
Copy link
Member

cimendes commented Apr 17, 2024

@cimendes cimendes merged commit f21ea74 into main Apr 17, 2024
9 checks passed
@cimendes cimendes deleted the cjk-pangolin-update-fix branch April 17, 2024 11:03
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