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

Moved whitelist parameters as input files #962

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

Conversation

vsmalladi
Copy link
Contributor

Description

White list was hardcoded as gs paths. Made them defaults and moved to inputs for workflow so that they will be able to be inputed for different paths for azure.


Checklist

If you can answer "yes" to the following items, please add a checkmark next to the appropriate checklist item(s) and notify our WARP documentation team by tagging either @ekiernan or @kayleemathews in a comment on this PR.

  • Did you add inputs, outputs, or tasks to a workflow?
  • Did you modify, delete or move: file paths, file names, input names, output names, or task names?
  • If you made a changelog update, did you update the pipeline version number?

@dsde-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

1 similar comment
@dsde-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@kevinpalis kevinpalis left a comment

Choose a reason for hiding this comment

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

LGTM!

@jessicaway
Copy link
Member

Hi @vsmalladi,

I believe you are trying to make the pipeline flexible to accept whitelist files from sources other than GCP, is that correct?

The pipeline should already allow this.

The reason the defaults are listed outside the inputs section is to have the pipeline automatically select the whitelist based on the chemistry. This behavior and defaults can be overridden by explicitly providing a whitelist as input to the pipeline. Here is the input that allows this: https://github.com/broadinstitute/warp/blob/develop/pipelines/skylab/optimus/Optimus.wdl#L38

Does that address the concern you are trying to address with this PR or is there a further issue?

@vsmalladi
Copy link
Contributor Author

Hi @vsmalladi,

I believe you are trying to make the pipeline flexible to accept whitelist files from sources other than GCP, is that correct?

The pipeline should already allow this.

The reason the defaults are listed outside the inputs section is to have the pipeline automatically select the whitelist based on the chemistry. This behavior and defaults can be overridden by explicitly providing a whitelist as input to the pipeline. Here is the input that allows this: https://github.com/broadinstitute/warp/blob/develop/pipelines/skylab/optimus/Optimus.wdl#L38

Does that address the concern you are trying to address with this PR or is there a further issue?

@jessicaway I get that, but then this assumes that the person knows about the files before trying to run the pipeline. Moving it to Inputs allows the files to be known by the user and UI in terra to have a user put in Azure or other location for the reference data.

@vsmalladi vsmalladi marked this pull request as draft April 12, 2023 22:17
@jessicaway
Copy link
Member

@vsmalladi, I'm not sure I understand the distinction since the whitelist is already a pipeline input and Terra does not populate the default values in the input view. Regardless, we want to make this more flexible to be run in Azure as well as GCP, so an update to the current implementation will be needed.

Would you have a few min for a quick call sometime next week? It would be helpful to understand the Azure constraints so we can update this.

@vsmalladi
Copy link
Contributor Author

@jessicaway yes lets do a call next week will reach out on slack.

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.

4 participants