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

Meet template convention #21

Draft
wants to merge 23 commits into
base: dev
Choose a base branch
from
Draft

Meet template convention #21

wants to merge 23 commits into from

Conversation

monikrzyz
Copy link

@monikrzyz monikrzyz commented Nov 17, 2023

Overview

Purpose

To have curated pipeline in IHIS.

Acceptance Criteria/Jira Ticket

DEL-15019

Changes

  • meets convention defined in the ticket

Internal CloudOS Tests

Not necessary if run in Internal CloudOS CI

Client CloudOS Tests

PR Checklist

Author to check:

  • The PR title is informative and begins with appropriate [Fix:|Feat:|Docs:] based on PR type
  • This PR is raised to dev branch for DSL1 or DSL2-dev for DSL2 translations
  • The description contains the acceptance criteria or link to the associated Jira ticket
  • The description contains links to CloudOS tests covering all test cases in client environment

@monikrzyz monikrzyz marked this pull request as draft November 17, 2023 15:44
@monikrzyz monikrzyz marked this pull request as ready for review November 17, 2023 15:44
@monikrzyz monikrzyz requested a review from a team as a code owner November 20, 2023 10:06
@monikrzyz monikrzyz marked this pull request as draft November 20, 2023 11:33
@monikrzyz monikrzyz marked this pull request as ready for review November 20, 2023 11:33
@monikrzyz monikrzyz marked this pull request as draft November 20, 2023 11:35
@monikrzyz monikrzyz marked this pull request as ready for review November 20, 2023 11:35
@monikrzyz monikrzyz marked this pull request as draft November 20, 2023 11:40
@monikrzyz monikrzyz marked this pull request as ready for review November 20, 2023 11:40
@monikrzyz monikrzyz marked this pull request as draft November 20, 2023 12:36
@monikrzyz monikrzyz marked this pull request as ready for review November 20, 2023 12:36
@monikrzyz monikrzyz marked this pull request as draft November 20, 2023 12:42
@monikrzyz monikrzyz marked this pull request as ready for review November 20, 2023 12:43
@monikrzyz monikrzyz marked this pull request as draft November 20, 2023 12:49
@monikrzyz monikrzyz marked this pull request as ready for review November 20, 2023 12:49
@monikrzyz monikrzyz marked this pull request as draft November 20, 2023 13:03
@monikrzyz monikrzyz marked this pull request as ready for review November 20, 2023 13:04
@monikrzyz monikrzyz marked this pull request as draft November 23, 2023 13:37
@sk-sahu sk-sahu changed the base branch from adds-profiles to dev November 28, 2023 14:40
@monikrzyz monikrzyz marked this pull request as ready for review December 8, 2023 02:43
@monikrzyz monikrzyz marked this pull request as draft December 8, 2023 02:58
@monikrzyz monikrzyz marked this pull request as ready for review December 8, 2023 02:58
Copy link

@l-mansouri l-mansouri 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 Monika for all the work! However, there are some things that needs to be addressed before merging. Let me know if the comments are not very clear.

@@ -0,0 +1,4 @@
params {
igenomes_base = 's3://ngi-igenomes/igenomes'

Choose a reason for hiding this comment

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

I think we had agreed that all this was supposed to point to either the eu-west1 featured bucked or the original featured bucket, if I remember correctly. Do we still need to keep the igenome??

@@ -0,0 +1,4 @@
params {
igenomes_base = 's3://ngi-igenomes/igenomes'
reference_bucket = 's3://lifebit-featured-datasets/'

Choose a reason for hiding this comment

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

Suggested change
reference_bucket = 's3://lifebit-featured-datasets/'
reference_data_bucket = 's3://lifebit-featured-datasets/'

@@ -0,0 +1,3 @@
params {
igenomes_base = 's3://reference-data-cyn-pro/igenomes'

Choose a reason for hiding this comment

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

Suggested change
igenomes_base = 's3://reference-data-cyn-pro/igenomes'
reference_data_bucket = 's3://reference-data-cyn-pro/igenomes'

if we need to keep this for cynapxe, then maybe it would be best to have it as reference_data_bucket

@@ -1,5 +1,5 @@
docker.enabled = true

params {
input = "testdata/input_mix_vcf_origin.tsv"
input = "s3://lifebit-featured-datasets/pipelines/mutational-signature-nf/testdata/input_mix_vcf_origin.tsv"

Choose a reason for hiding this comment

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

Suggested change
input = "s3://lifebit-featured-datasets/pipelines/mutational-signature-nf/testdata/input_mix_vcf_origin.tsv"
input = "${params.reference_data_bucket}"/pipelines/mutational-signature-nf/testdata/input_mix_vcf_origin.tsv"

@@ -92,6 +103,9 @@ profiles {
cloudos_production_test_2 { includeConfig 'conf/cloudos_production_test_2.config' }
}

includeConfig 'conf/containers/quay.config'

Choose a reason for hiding this comment

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

this line has to go before the profile definition otherwise it will not work on ihis and it's already there, so I would remove it from here!

Choose a reason for hiding this comment

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

there are no reference files for this pipeline?

Choose a reason for hiding this comment

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

also here https://github.com/lifebit-ai/mutational-signature-nf/blob/74e1bebcf78d67bcc1755359a8085da01197d65b/nextflow.config#L37C5-L37C49 there's this container definition that is not parametrised. I am not sure that this is ever used but we might need to double check that!

signature_tool_lib_container = "518054667335.dkr.ecr.${params.container_region}.amazonaws.com/lifebitai/signature_tool_lib:1e58326"
utility_scripts_container = "518054667335.dkr.ecr.${params.container_region}.amazonaws.com/lifebitai/utility_scripts:b586275"
awscli_bcftools_container = "518054667335.dkr.ecr.${params.container_region}.amazonaws.com/lifebitaiorg/awscli_bcftools_comb:v0.1"
main_container = '518054667335.dkr.ecr.${params.container_region}.amazonaws.com/lifebitai/ubuntu:18.10'

Choose a reason for hiding this comment

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

These all need the double quotes, otherwise the ${params.container_region} will not be compiled and it will be used as a string.

@sk-sahu sk-sahu marked this pull request as draft January 18, 2024 15:18
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.

5 participants