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

Release PR 1.5.0 #242

Merged
merged 300 commits into from
Aug 13, 2024
Merged

Release PR 1.5.0 #242

merged 300 commits into from
Aug 13, 2024

Conversation

apeltzer
Copy link
Member

Adding here "just" the points addressed in this release 👇🏻

1.5.0 - 2024-08-12

Added

  • #202 Added cellranger mkfastq subworkflow for demultiplexing 10x samples.
  • #206 Add test with uncompressed data.
  • #208 Added parameter for removing adapter information from samplesheets.
  • #210 Add checkqc module.
  • #212 Added resource setting arguments to mkfastq module to work with github CI.
  • #214 Added test_pe (paired end) profile.
  • #220 Added kraken2.
  • #221 Added checkqc_config to pipeline schema.
  • #225 Added test profile for multi-lane samples, updated handling of such samples and adapter trimming.
  • #234 Added module for samplesheet validation.
  • #236 Add samplesheet generation.

Changed

  • #204 Update to latest bcl_demultiplex sub workflow.
  • #210 Update bcl2fastq and bcl_demultiplex.
  • #214 Updated method for removing adapters from samplesheet, added custom AdapterRemover function.
  • #210 Update bcl2fastq and bcl_demultiplex.
  • #216 List fastq reports for R1 and R2 separately in multiqc report.
  • #219 Modified workflow to store samplesheet in results folder.
  • #217 Update all nf-core modules and tests.
  • #240 Updated samshee, stub script and error strategy

Fixed

  • #224 Fix input filename collision.
  • #231 Fix checkqc error message and .command.log emission.

nschcolnicov and others added 30 commits July 17, 2024 21:08
@nschcolnicov
Fixed editorconfig issues
06240de
@k1sauce
feat: update bcl_demultiplex subworkflow
6099e0e
test: update snaps for new versions
ab786a8
@atrigila
fix schema format path
0d91126
@atrigila
Merge pull request #205 from atrigila/fix_path_schema
52a1b14 
fix schema format path
Updated main.nf.test and snap
605f9a4
Merge branch 'add_demultiplexing_subworkflow_10x' of https://github.c…
dca93a2 
…om/nschcolnicov/demultiplex into add_demultiplexing_subworkflow_10x
@atrigila
update changelog
174bd3d
@atrigila
Merge pull request #206 from atrigila/test_uncompress
574a256 
Add test with uncompressed data
Added CI test for mkfastq:
412efdf
Pulled from dev
1ab96c0
@nschcolnicov
Installed mkfastq from modules for CI test
04bd37b
@nschcolnicov
Re-added removed test profile during merge, updated mkfastq test reso…
b76b664 
…urces for CI
@atrigila
add checkqc module
5d24e0b
Added adapter removal
a4db7ef
Updated conditional statement and nextflow_schema
89d8ced
Updated snaps and changelog
4813c51
@nschcolnicov
Ran pre-commun
e34c4c5
@atrigila
add config checkqc
42928d5
@atrigila
create temp dir for testing checkqc
ace3efa
@atrigila
add checkqc config
18c9671
@nschcolnicov
Updated CI test and mkfastq nf-test and snap
787744e
@nschcolnicov @grst
Apply suggestions from code review
885e867 
Co-authored-by: Gregor Sturm <mail@gregor-sturm.de>
@nschcolnicov
Merge pull request #208 from nschcolnicov/remove_trimming
4f6a841 
Add feature for adapter removal in illumina samplesheet, to avoid automatic trimming during demultiplexing by bcl conversion tools.
@atrigila
add checkqc module config
6026d78
@atrigila
add condition to run checkqc
59941e6
@atrigila
add checkqc config as param
e07ed2b
@atrigila
update checkqc test
b981ca0
@atrigila
update modules config
0c1d9f9
@atrigila
skip checkqc if required
6d16ffb
@apeltzer apeltzer requested a review from a team as a code owner August 12, 2024 21:00
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 12, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 7b37e90

+| ✅ 190 tests passed       |+
#| ❔   3 tests were ignored |#
!| ❗   7 tests had warnings |!

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: 1.5.0
  • pipeline_todos - TODO string in README.md: Describe the minimum required steps to execute the pipeline, e.g. how to prepare samplesheets.
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!
  • 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
  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-08-13 19:36:06

@nschcolnicov
Copy link
Contributor

We are looking into this issue with @atrigila.
For the utils_nfvalidation_plugin, the issue seems to be with where the output was expected to be located when writing the nf-tests.
The utils_nfvalidation_plugin/main.nf uses this line to print the help message:
image
This gets printed into the nextflow.log, but not into stdout:
image

image

One alternative to fix this would be to replace the log.info with a println, this way the output will be redirected to stdout insted of to the nextflow.log:
image

@apeltzer An alternative would be to re write the nf-tests so that they check the nextflow.log, or something similar. What do you think?

@grst
Copy link
Member

grst commented Aug 13, 2024

No strong opinion... I think it's fine (or even preferable) if help is written to STDOUT (most CLI tools do that).
Have you checked what other pipelines do?

@apeltzer
Copy link
Member Author

Variantbenchmarking / taxprofiler both use the unmodified utils_nfvalidation_plugin/main.nf

@nschcolnicov
Copy link
Contributor

@apeltzer @grst I think using log instead of println is better, the log message sticks a little longer in the terminal than println, making it easier to read, besides that, you get it printed in a different color which also helps visibility, but you can do that with println too, so not the biggest advantage.
I'm not sure why the nf-tests were passing before for this subworkflow, could it be that they weren't being properly executed? I'll create an issue in the modules repo

@atrigila
Copy link
Contributor

The nf-validation plugin does not have a snapshot, therefore, I don't know if we can say that these tests were working before.

@apeltzer
Merge pull request #245 from nf-core/fix-utils
f6c31b8 
Update some modules, upgrade to nf-schema
@apeltzer
Copy link
Member Author

OK - lets stick with nf-validation for now, the bug (or whatever it in the end turns out to be) was reported upstream and is probably tackled when nf-core moves towards wider usage of nf-schema in favor of nf-validation, thus also updating the required subworkflows / replace them entirely.

@nschcolnicov
Copy link
Contributor

@apeltzer @atrigila @grst Removed workflow testing from CI to avoid running the tests on the subworkflows, because UTILS_NEXTFLOW_PIPELINE and UTILS_NFVALIDATION_PLUGIN have nf-tests that are not working. The reason for them failing has been documented and an issue was opened: nf-core/modules#6166.
These failing nf-tests don't seem to have any impact on the disambiguate pipeline, so they have been disabled in this PR, to prepare the dev branch for release.
We will update the CI tests once the issue with the subworfklows is sorted out.

@apeltzer
Copy link
Member Author

So its just the downloading feature failing now - filed an issue for that one: nf-core/tools#3111

nschcolnicov
nschcolnicov approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@nschcolnicov nschcolnicov left a comment

Choose a reason for hiding this comment

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

Left some comments, minor things that don't affect execution or performance, we can address them before the release or after. Besides that PR looks good.

atrigila
atrigila approved these changes Aug 13, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@nschcolnicov
Copy link
Contributor

@apeltzer @atrigila I created a PR for the comments left in this review, not sure if we are going to merge them in this release, but to get the ball rolling in case we do:
https://github.com/nf-core/demultiplex/pull/248/files

@apeltzer
Merge pull request #248 from nf-core/release_pr_comments
7b37e90 
Removed customdumpsoftwareversions and updated README.md
@apeltzer apeltzer merged commit 512ea7e into master Aug 13, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nschcolnicov nschcolnicov nschcolnicov approved these changes

@atrigila atrigila atrigila approved these changes

@maxulysse maxulysse Awaiting requested review from maxulysse

@grst grst Awaiting requested review from grst

@edmundmiller edmundmiller Awaiting requested review from edmundmiller

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

Successfully merging this pull request may close these issues.
7 participants
@apeltzer @nschcolnicov @grst @atrigila @k1sauce @nvnieuwk @edmundmiller