Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

bugfix for type of some output files in cnv_somatic_pair_workflow.wdl #6735

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

slzhao
Copy link
Contributor

@slzhao slzhao commented Aug 1, 2020

Hello,

I made two changes in output files of cnv_somatic_pair_workflow.wdl to fix bugs:

  1. Change "File" into "String" for all "entity_id" outputs. The "File" type will make error when Output Copying was used in cromwell, as there is no such file in output;
  2. Change "File select_first([CNVOncotatorWorkflow.oncotated_called_file, "null"])" into "File? oncotated_called_file_tumor" for oncotate and funcotate outputs. If oncotate or funcotate was not performed, the original will output "null" and make error when Output Copying was used in cromwell, as there is "null" file in output;

@cmnbroad
Copy link
Collaborator

cmnbroad commented Aug 3, 2020

I've encountered and reported a similar problem for cromwell. I think the changes proposed here won't result in correct localization/delocalization, but I'm not sure what the correct solution is for optional outputs where no file name is provided. See this cromwell jira ticket.

@lbergelson
Copy link
Member

I'm not sure external people can see that jira at all.

@slzhao
Copy link
Contributor Author

slzhao commented Aug 3, 2020

Hello,

Just want to clarify that the Point 2 change I made is about "File?" in output and it works with cromwell v51. If the task generated that file was not run (such as skipped by if in this cnv_somatic_pair_workflow.wdl), the output will also skip it and will not report error.

I think the ticket you mentioned is talking about "String?", which is different that "File?" in output?

Thanks!

@droazen
Copy link
Contributor

droazen commented Oct 8, 2020

@mwalker174 Can you review this one when you get a chance?

Copy link
Contributor

@mwalker174 mwalker174 left a comment

Choose a reason for hiding this comment

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

Thank you @slzhao this looks good. Please see my one comment below.

Comment on lines +571 to +575
# File? oncotated_called_file_tumor = select_first([CNVOncotatorWorkflow.oncotated_called_file, "null"])
# File? oncotated_called_gene_list_file_tumor = select_first([CNVOncotatorWorkflow.oncotated_called_gene_list_file, "null"])
# File? funcotated_called_file_tumor = select_first([CNVFuncotateSegmentsWorkflow.funcotated_seg_simple_tsv, "null"])
# File? funcotated_called_gene_list_file_tumor = select_first([CNVFuncotateSegmentsWorkflow.funcotated_gene_list_tsv, "null"])

Copy link
Contributor

Choose a reason for hiding this comment

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

You can delete these commented lines

@samuelklee
Copy link
Contributor

Thanks for catching these, @slzhao!

@ldgauthier
Copy link
Contributor

@slzhao do you have time to make that one small edit? After that, this is good to merge.

@samuelklee
Copy link
Contributor

@slzhao item 1 in your original post randomly came up in the OpenWDL Slack, which ultimately caused me to realize that this PR is still open. Would you like to remove the commented lines and merge? If we don't hear back from you, I'll merge this and issue a separate PR to remove the commented lines.

@samuelklee samuelklee merged commit c5bccee into broadinstitute:master Dec 14, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants