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.

Sign up for GitHub

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

Jump to bottom

Remove Sentieon license details from -profile test #1237

Closed
wants to merge 14 commits into from
Closed

Remove Sentieon license details from -profile test #1237

wants to merge 14 commits into from

Conversation

adamrtalbot
Copy link
Contributor

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/sarek branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions
Copy link

github-actions bot commented Sep 15, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 9c11604

+| ✅ 141 tests passed       |+
#| ❔  10 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-09-15 14:39:05

maxulysse
maxulysse reviewed Sep 15, 2023
View reviewed changes
conf/base.config Outdated
Comment on lines 57 to 60
withLabel: 'sentieon' {
ext.sentieon_auth_mech_base64 = secrets.SENTIEON_AUTH_MECH_BASE64
ext.sentieon_auth_data_base64 = secrets.SENTIEON_AUTH_DATA_BASE64
}
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't put that in base.config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK so I'll move it to conf/sentieon.config.

maxulysse
maxulysse requested changes Sep 15, 2023
View reviewed changes
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

I'd do that in a separate config dedicated to sentieon licence

@adamrtalbot
Copy link
Contributor Author

I'd do that in a separate config dedicated to sentieon licence

Moved to separate file conf/sentieon.config

asp8200
asp8200 reviewed Sep 15, 2023
View reviewed changes
nextflow.config Outdated
@@ -137,6 +137,9 @@ params {
// Load base.config by default for all pipelines
includeConfig 'conf/base.config'

// Load Sentieon config
includeConfig 'conf/sentieon.config'
Copy link
Contributor

@asp8200 asp8200 Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It seems to me that with that, you wouldn't be able to run anything without having SENTIEON_AUTH_MECH_BASE64 and SENTIEON_AUTH_DATA_BASE64 set as nf-secrets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How so? if these aren't set, surely it just ignores them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this work:

process {
    withLabel: 'sentieon' {
        ext.sentieon_auth_mech_base64 = secrets.SENTIEON_AUTH_MECH_BASE64 ?: ''
        ext.sentieon_auth_data_base64 = secrets.SENTIEON_AUTH_DATA_BASE64 ?: ''
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

No, that is actually the problem. By referring to those two secrets in cache.config without them being set, Ram got

Unknown config secret 'SENTIEON_AUTH_MECH_BASE64'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do they need to be secret? Can we just make them params?

Copy link
Contributor

Choose a reason for hiding this comment

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

You write too fast 😆 "No, that is actually the problem." was my answer to "How so? if these aren't set, surely it just ignores them?".

This

ext.sentieon_auth_mech_base64 = secrets.SENTIEON_AUTH_MECH_BASE64 ?: ''

seems to work. In fact, it could probably just have been introduced in the orginial cache.config and then problem solved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what if you need SENTIEON_AUTH_MECH_BASE64 but you are running for real? It's not just a test thing, it's for extra security: https://support.sentieon.com/appnotes/license_server/

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm - I didn't know about that "License Server Extension". It is not something we use at my workplace. Perhaps make an issue for making Sarek handle that as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that what this is for? What's the point in these variables?

Copy link
Contributor

Choose a reason for hiding this comment

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

This just in from Ram: He still gets

Unknown config secret 'SENTIEON_AUTH_MECH_BASE64' -- Check script

when using

        ext.sentieon_auth_mech_base64 = secrets.SENTIEON_AUTH_MECH_BASE64 ?: ''
        ext.sentieon_auth_data_base64 = secrets.SENTIEON_AUTH_DATA_BASE64 ?: ''

in cache.config and commercial-license (ie. only SENTIEON_LICENSE_BASE64 as nf-secret)

@ramprasadn
Copy link

Tested this branch locally by running
nextflow run main.nf -profile test_cache,targeted,singularity --input ./tests/csv/3.0/mapped_single_bam.csv --tools sentieon_haplotyper --step variant_calling --outdir results

and I got this error back

ERROR ~ Error executing process > 'NFCORE_SAREK:SAREK:BAM_VARIANT_CALLING_GERMLINE_ALL:BAM_VARIANT_CALLING_SENTIEON_HAPLOTYPER:SENTIEON_HAPLOTYPER (test)'

Caused by:
  Unknown config secret 'SENTIEON_AUTH_MECH_BASE64' -- Check script './workflows/../subworkflows/local/bam_variant_calling_germline_all/../bam_variant_calling_sentieon_haplotyper/../../../modules/nf-core/sentieon/haplotyper/main.nf' at line: 40

Source block:
  if (workflow.profile.tokenize(',').intersect(['conda', 'mamba']).size() >= 1) {
          error "Sentieon modules do not support Conda. Please use Docker / Singularity / Podman instead."
      }
  def args = task.ext.args ?: ''
  def args2 = task.ext.args2 ?: ''
  def args3 = task.ext.args3 ?: ''
  def prefix = task.ext.prefix ?: "${meta.id}"
  def dbsnp_command = dbsnp ? "-d $dbsnp " : ""
  def interval_command = intervals ? "--interval $intervals" : ""
  def sentieon_auth_mech_base64 = task.ext.sentieon_auth_mech_base64 ?: ''
  def sentieon_auth_data_base64 = task.ext.sentieon_auth_data_base64 ?: ''
  def vcf_cmd = ""
  def gvcf_cmd = ""
  def base_cmd = '--algo Haplotyper ' + dbsnp_command
  if (emit_vcf) {  // emit_vcf can be the empty string, 'variant', 'confident' or 'all' but NOT 'gvcf'
          vcf_cmd = base_cmd + args2 + ' --emit_mode ' + emit_vcf + ' ' + prefix + '.unfiltered.vcf.gz'
      }
  if (emit_gvcf) { // emit_gvcf can be either true or false
          gvcf_cmd = base_cmd + args3 + ' --emit_mode gvcf ' + prefix + '.g.vcf.gz'
      }
  """
      if [ "\${#SENTIEON_LICENSE_BASE64}" -lt "1500" ]; then  # If the string SENTIEON_LICENSE_BASE64 is short, then it is an encrypted url.
          export SENTIEON_LICENSE=\$(echo -e "\$SENTIEON_LICENSE_BASE64" | base64 -d)
      else  # Localhost license file
          # The license file is stored as a nextflow variable like, for instance, this:
          # nextflow secrets set SENTIEON_LICENSE_BASE64 \$(cat <sentieon_license_file.lic> | base64 -w 0)
          export SENTIEON_LICENSE=\$(mktemp)
          echo -e "\$SENTIEON_LICENSE_BASE64" | base64 -d > \$SENTIEON_LICENSE
      fi

      if  [ ${sentieon_auth_mech_base64} ] && [ ${sentieon_auth_data_base64} ]; then
          # If sentieon_auth_mech_base64 and sentieon_auth_data_base64 are non-empty strings, then Sentieon is mostly likely being run with some test-license.
          export SENTIEON_AUTH_MECH=\$(echo -n "${sentieon_auth_mech_base64}" | base64 -d)
          export SENTIEON_AUTH_DATA=\$(echo -n "${sentieon_auth_data_base64}" | base64 -d)
          echo "Decoded and exported Sentieon test-license system environment variables"
      fi

      sentieon driver $args -r $fasta -t $task.cpus -i $input $interval_command $vcf_cmd $gvcf_cmd

      cat <<-END_VERSIONS > versions.yml
      "${task.process}":
          sentieon: \$(echo \$(sentieon driver --version 2>&1) | sed -e "s/sentieon-genomics-//g")
      END_VERSIONS
      """

Tip: when you have fixed the problem you can continue the execution adding the option `-resume` to the run command line

 -- Check '.nextflow.log' file for details

asp8200
asp8200 reviewed Sep 15, 2023
View reviewed changes
tests/config/tags.yml
@@ -433,6 +433,30 @@ mutect2:
- tests/csv/3.0/recalibrated_tumoronly.csv
- tests/test_mutect2.yml

## Sentieon
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this not cause the pytests for sentieon to be run twice?

Copy link
Member

Choose a reason for hiding this comment

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

agree, we don't need that to run twice.
Maybe it was added just to trigger it

@adamrtalbot
Copy link
Contributor Author

Tested this branch locally by running nextflow run main.nf -profile test_cache,targeted,singularity --input ./tests/csv/3.0/mapped_single_bam.csv --tools sentieon_haplotyper --step variant_calling --outdir results

and I got this error back
...

Darn, the ternary doesn't work. I've wrapped it in a parameter now --sentieon_license_server_extension. Try now.

@adamrtalbot
Copy link
Contributor Author

Tested this branch locally by running nextflow run main.nf -profile test_cache,targeted,singularity --input ./tests/csv/3.0/mapped_single_bam.csv --tools sentieon_haplotyper --step variant_calling --outdir results
and I got this error back
...

Darn, the ternary doesn't work. I've wrapped it in a parameter now --sentieon_license_server_extension. Try now.

Oh hold on, I messed up by leaving the old config in there 🤦

ramprasadn
ramprasadn reviewed Sep 15, 2023
View reviewed changes
nextflow.config Outdated Show resolved Hide resolved
@ramprasadn
Copy link

This is painful. I guess now they are failing since sentieon.config is loaded before test_cache profile 🙄

Perhaps there is an environmental variable or something we can use to detect if we are on CI? 🤔

@maxulysse maxulysse mentioned this pull request Sep 15, 2023
Closed
10 tasks
@maxulysse
Copy link
Member

yeah, so I'd rather we deal with profile tests with a specific profile.
@ramprasadn you're probably going to add CI for sentieon on raredisease too at some point, no?

@maxulysse maxulysse mentioned this pull request Sep 18, 2023
Merged
@maxulysse maxulysse closed this Sep 18, 2023
@maxulysse maxulysse deleted the sentieon-license-fixing branch September 18, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asp8200 asp8200 asp8200 left review comments

@ramprasadn ramprasadn ramprasadn left review comments

@maxulysse maxulysse maxulysse requested changes

Assignees
No one assigned
Labels
sentieon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adamrtalbot @ramprasadn @maxulysse @asp8200 @edmundmiller