Automatically convert spaces to dashes in workflows that accept strings

AndrewLangvt commented 4 weeks ago

This PR closes #326 .

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

:brain: Aim, Context and Functionality

Multiple workflows allow for string input, but when user inputs a string with a space, cromwell engine will fail. As such, this PR resolves this issue across all workflows.

:hammer_and_wrench: 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

:clipboard: Workflow/Task Step Changes

🔄 Data Processing

Docker/software or software versions changed: No

Databases or database versions changed: No

Data processing/commands changed: No

File processing changed: Yes

Compute resources changed: No

➡️ Inputs

Added the following inputs, all of which are provided the base input (strip _updated and that's the basename) and replaces space with underscore Freyja dashboard: freyja_dashboard_title_updated Freyja plot: freyja_plot_name_updated Augur_PHB: build_name_updated Core_Gene_Snp_PHB: cluster_name_updated Find_Shared_Variants_PHB: concatenated_file_name_updated kSNP3_PHB: cluster_name_updated Lyve_SET_PHB: dataset_name_updated MashTree_FASTA_PHB: cluster_name_updated Snippy_Streamline_PHB: tree_name_updated Snippy_Tree_PHB: tree_name_updated Usher_PHB: tree_name_updated Mercury_Prep_N_Batch_PHB: output_name_updated Concatenate_Column_Content_PHB: concatenated_file_name_updated Rename_FASTQ_PHB: new_filename_updated Zip_Column_Content_PHB: zipped_file_name_updated

⬅️ Outputs

None

:test_tube: Testing

Test Dataset

Tests were executed with the same parameters as PHB 2.0 validation for the preponderance of workflows. Those that deviated from this did so simply to cut down on compute costs and a reasonable number of samples were still run

Commandline Testing with MiniWDL or Cromwell (optional)

all workflows passed miniwdl check

Terra Testing

Suggested Scenarios for Reviewer to Test

For each of the workflows, provide a string with spaces to the input.

Theiagen Version Release Testing (optional)

:microscope: Final Developer Checklist

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

🎯 Reviewer Checklist

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

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

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

kapsakcj commented 3 weeks ago

It would be good to go through the documentation and remove the mentions of No whitespaces! or whatever message that listed in the workflow input parameter descriptions. I see it on Freyja_dashboard, and I'm assuming it might be mentioned in the input param tables for other workflows too.

This PR will not require updating workflow diagrams, so I checked that off

kapsakcj commented 3 weeks ago

Very nice work on your 2nd PR 🎉

I tested all workflows on independent datasets and intentionally put spaces in all the string inputs that were changed by this PR. Just a couple of spots where the updated variable names were missing, but those have been resolved. After those quick updates, everything ran great. Workflows tested:

✅ freyja_plot test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/c92372ab-b6d2-498d-9406-aacfec6ee97a
✅ freyja_dash test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/01819f19-bd9d-4343-bd3a-7e8a30acb7b3
✅ augur test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/3e40301c-d947-47e0-8c35-2ac2102036f8
✅ core_gene_snp test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/c2c4d3aa-6d7a-47cb-99c0-cd15e807226d
✅ find_shared_variants test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/9f92f5b8-3d78-427b-b5f3-0d91d5262f1d
✅ usher test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/b16ea312-78ea-4d64-9d89-a69bd94eed24
✅ kSNP3 test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/22faf22a-df45-4161-86a1-b36847d7aad0
✅ lyveset test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/2287d9fc-7b77-487c-a42b-abba4ce362aa
✅ mashtree test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/797cae85-79e8-4899-b740-4921e3dc508d
✅ snippy_streamline test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/7952905a-2e84-404b-85b8-85fafb63d179
✅ snippy_tree test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/9a3b5d2f-9300-4cfa-905b-c01af6b6ca3e
✅ mercury_prep_n_batch test: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/f3ab9a54-658f-43f2-b1d7-bd9c8e228f7d
✅ concatenate_column_content: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/c1c2f415-7640-4135-874d-cb3e6eb7ea23
✅ rename_fastq_files: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/806cc38b-239b-420e-a672-15f6aa77b688
✅ zip_column_content: https://app.terra.bio/#workspaces/theiagen-validations/curtis-sandbox-theiagen-validations/job_history/8bc3feac-fb76-4170-8832-379e52b94f2e

Please let me know when you are finished updating the documentation and I'll approve and merge. Thanks again for the changes, this will save everyone lots of time in preventing user error and troubleshooting

AndrewLangvt commented 3 weeks ago

@kapsakcj documentation is now updated for this PR

kapsakcj commented 3 weeks ago

@AndrewLangvt Unless you know someone is actively using the dev branch, I'd recommend deleting it

PR merged, thank you!

theiagen / public_health_bioinformatics