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

Resolved bug when prefetching large studies #238

Merged

Conversation

alexblaessle
Copy link

@alexblaessle alexblaessle commented Nov 7, 2023

Prefetching large studies resulted in error stated in issue

#236

Resolved this by handing over text file with filepaths instead of list of filepaths. Rewrote sra_merge_samplesheet accordingly.

Tested with study with ~25k samples.

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/fetchngs 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).

Prefetching large studies resulted in error stated in issue

nf-core#236

Resolved this by handing over text file with filepaths instead
of list of filepaths. Rewrote sra_merge_samplesheet accordingly.

Tested with study with ~25k samples.
Copy link

github-actions bot commented Nov 7, 2023

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 87c94c0

+| ✅ 133 tests passed       |+
#| ❔  18 tests were ignored |#
!| ❗   6 tests had warnings |!

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: 1.11.0
  • pipeline_todos - TODO string in meta.yml: #Add a description of the subworkflow and list keywords
  • pipeline_todos - TODO string in meta.yml: #Add a list of the modules and/or subworkflows used in the subworkflow
  • pipeline_todos - TODO string in meta.yml: #List all of the channels used as input with a description and their structure
  • pipeline_todos - TODO string in meta.yml: #List all of the channels used as output with a descriptions and their structure
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline

❔ Tests ignored:

  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: assets/multiqc_config.yml
  • files_exist - File is ignored: conf/igenomes.config
  • files_exist - File is ignored: conf/modules.config
  • files_exist - File is ignored: lib/NfcoreTemplate.groovy
  • files_exist - File is ignored: lib/Utils.groovy
  • files_exist - File is ignored: lib/WorkflowFetchngs.groovy
  • files_exist - File is ignored: lib/WorkflowMain.groovy
  • files_exist - File is ignored: lib/nfcore_external_java_deps.jar
  • files_unchanged - File ignored due to lint config: .gitattributes
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • files_unchanged - File does not exist: lib/nfcore_external_java_deps.jar
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/fetchngs/fetchngs/.github/workflows/awstest.yml
  • multiqc_config - 'assets/multiqc_config.yml' not found

✅ Tests passed:

Run details

  • nf-core/tools version 2.10
  • Run at 2023-11-13 12:04:16

@maxulysse
Copy link
Member

Can you fix the EC lint?
You seem to have left some old code as comments, I think you can delete them as you're replacing them.
Can you try to update the snapshots?
It looks like all tests are failing for now

@Midnighter
Copy link
Contributor

It looks to me like this could maybe be done entirely in Groovy without the need for a process at all?

@alexblaessle
Copy link
Author

@Midnighter Probably could be done all in groovy. Since the script before was in bash, I kept it as is. Not a groovy expert myself. Feel free to groovynice it.

@alexblaessle
Copy link
Author

Can you fix the EC lint? You seem to have left some old code as comments, I think you can delete them as you're replacing them. Can you try to update the snapshots? It looks like all tests are failing for now

Already pushed. Old comments linting should be done

@alexblaessle
Copy link
Author

It looks to me like this could maybe be done entirely in Groovy without the need for a process at all?

Resolved in latest commit. Please LMK if this works for you. @Midnighter

Copy link
Contributor

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@maxulysse
Copy link
Member

Looking good.
Do you want to include the updated sratools modules in this PR as well?

@maxulysse
Copy link
Member

You do have some failures in the tests, but that's normal given what you're changing.
Do you think you can manage to fix them by yourself, or would you need help for it?

@drpatelh drpatelh added this to the 1.12.0 milestone Jan 3, 2024
Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Thanks @alexblaessle ! Will fix the tests in follow up PRs.

@drpatelh drpatelh merged commit f794ea3 into nf-core:dev Jan 4, 2024
7 of 15 checks passed
drpatelh added a commit to drpatelh/nf-core-fetchngs that referenced this pull request Jan 4, 2024
@drpatelh drpatelh mentioned this pull request Jan 4, 2024
drpatelh added a commit that referenced this pull request Jan 8, 2024
@adamrtalbot adamrtalbot mentioned this pull request Jan 29, 2024
10 tasks
@edmundmiller edmundmiller mentioned this pull request Feb 6, 2024
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