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

Change default behavior of GatherSampleEvidence to skip running module metrics #633

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VJalili
Copy link
Member

@VJalili VJalili commented Jan 31, 2024

The GatherSampleEvidence workflow is set to run GatherSampleEvidenceMetrics by default. However, running that task requires setting values for the optional inputs primary_contigs_fai and sv_pipeline_base_docker. Therefore, the GatherSampleEvidence workflow currently fails if only the required input is provided.

Since WDL does not support making the optional inputs as required w.r.t to the value of another argument, and Terra UI lacks features to hint to a user about such a chain of requirements, in-line comments in code seem the only option, as it is implemented. However, in-code comments are less user-friendly and should be reserved for developers and power users only.

Skipping to run this task as a default behavior results in the successful execution of the workflow if only the required input is provided seems to be a better design and improved UX.

# This is due to the interface requirements, as when the default value is true, it requires few other optional arguments to be set. However, this is not clear to a user when running this workflow, and the workflow will fail if the arguments are not provided with less intuitive error messages.
Copy link
Collaborator

@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.

We have a few workflows like this and I agree it's not ideal.

In this case, however, I think we can change the other parameters. The sv_pipeline_base_docker doesn't exist anymore, so it can be replaced with sv_pipeline_docker, and we can just make primary_contigs_fai required.

@epiercehoffman
Copy link
Collaborator

The default input configuration in our template JSONs and Terra workspace includes all inputs required to run module metrics in the workflows where it is enabled by default. I honestly don't think it's a big issue, because we don't expect users to configure inputs from scratch - the inputs are complicated in other ways too. I'm definitely open to disabling module metrics collection by default, and to any changes that improve the clarity of inputs, but I think for now the real solution in terms of ease of use here is to pre-configure the inputs, which we have done.

@VJalili
Copy link
Member Author

VJalili commented Feb 1, 2024

It is a better UX to have the workflows run successfully if only the required input is provided. It seems we can get this covered following Mark's suggestion without changing the default behavior.

@mwalker174
Copy link
Collaborator

We should disable module metrics across the pipeline. We don't use them in production and have just been useful for development in a few rare cases.

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.

None yet

3 participants