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

Update isoseq modules and migrate to nf-test #4753

Merged
merged 45 commits into from
May 15, 2024

Conversation

sguizard
Copy link
Contributor

Modules migrated to nf-test:

  • isoseq/cluster
  • isoseq/refine
  • pbccs
  • pbbam/pbmerge
  • lima

Software update:

  • isoseq/cluster: v.3.8.1 -> 4.0.0
  • isoseq/refine: v.3.8.1 -> 4.0.0
  • pbbam/pbmerge: 2.1.0 -> 2.4.0
  • lima: 2.7.1 -> 2.9.0

@sguizard sguizard requested a review from a team as a code owner January 17, 2024 15:31
@sguizard sguizard requested a review from leoisl January 17, 2024 15:31
@sguizard
Copy link
Contributor Author

The conda pbbam/pbmerge is failing for not matching snapshots, but singularity and docker ones succeed. Should it be ignored?

Copy link
Contributor

@famosab famosab 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 :) I left some comments. For the next time: I think its best practise to do one module per PR so it stays easier for the reviewers.

modules/nf-core/isoseq/cluster/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/isoseq/cluster/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/isoseq/refine/main.nf Show resolved Hide resolved
modules/nf-core/isoseq/refine/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/lima/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/pbbam/pbmerge/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/pbccs/tests/main.nf.test Outdated Show resolved Hide resolved
@famosab
Copy link
Contributor

famosab commented Mar 21, 2024

The conda pbbam/pbmerge is failing for not matching snapshots, but singularity and docker ones succeed. Should it be ignored?

I would go for comparing the file contents rather than the checksums in that case but not ignoring conda alltogether. So go for something like

# checking output vcf files
{ assert path(process.out.candidate_small_indels_vcf.get(0).get(1)).linesGzip.contains("##fileformat=VCFv4.1") }

# checking output files with same headers
{ assert snapshot(file(process.out.stats.get(0).get(1)).readLines()[0..5]).match() }

@SPPearce
Copy link
Contributor

SPPearce commented May 7, 2024

@sguizard , are you able to finish this PR off and get these changes merged in? Please ask if you need help.
Conda often gives md5sums for bam files that are different to docker/singularity, so you need to check for existence of the file rather than the md5sum.

@famosab
Copy link
Contributor

famosab commented May 14, 2024

From the anaconda registry it seems to me that there was a version jump from 2.1 to 2.4 so 2.3 and 2.5 do not exist there. And for pbcopper there is no version 2.4 on anaconda. Or am I missing something?

@famosab
Copy link
Contributor

famosab commented May 14, 2024

Second comment: It seems to me that pbbam was archived and the individual tools were transferred to pbtk. I think this should be reflected in the nf-core modules?

I would suggest to remove all changes made to pbbam/pbmerge from this PR and tackle the proper update on a separate branch with a separate PR. Because all the other modules have been ported properly and then it would be less crowded. What do you think @SPPearce @sguizard?

@SPPearce
Copy link
Contributor

Second comment: It seems to me that pbbam was archived and the individual tools were transferred to pbtk. I think this should be reflected in the nf-core modules?

I would suggest to remove all changes made to pbbam/pbmerge from this PR and tackle the proper update on a separate branch with a separate PR. Because all the other modules have been ported properly and then it would be less crowded. What do you think @SPPearce @sguizard?

That sounds sensible, let's do that then.

Copy link
Contributor

@famosab famosab left a comment

Choose a reason for hiding this comment

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

Remove the ppbam related files from this commit and follow up on #5601

modules/nf-core/pbbam/pbmerge/main.nf Outdated Show resolved Hide resolved
modules/nf-core/pbbam/pbmerge/meta.yml Outdated Show resolved Hide resolved
modules/nf-core/pbbam/pbmerge/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/pbbam/pbmerge/tests/main.nf.test.snap Outdated Show resolved Hide resolved
modules/nf-core/pbbam/pbmerge/tests/nextflow.config Outdated Show resolved Hide resolved
modules/nf-core/pbbam/pbmerge/tests/tags.yml Outdated Show resolved Hide resolved
modules/nf-core/pbbam/pbmerge/environment.yml Outdated Show resolved Hide resolved
PacBio have change their program organisation. Pbmerge is now included into pbtk.
@sguizard
Copy link
Contributor Author

I removed pbbam/pbmerge and all tests are green 🎉
Thanks for the support.

@famosab
Copy link
Contributor

famosab commented May 15, 2024

@SPPearce Do you think we should remove the module completely here (as indicated in the issue #5601)? Then I will retract my comments :D

Edit: I asked in the #modules slack channel

@SPPearce
Copy link
Contributor

As discussed on the slack channel, I would suggest that we leave the pbmerge module untouched in this PR, and open a separate PR to deprecate it (rather than delete it).

.vscode/settings.json Outdated Show resolved Hide resolved
sguizard and others added 2 commits May 15, 2024 09:49
Co-authored-by: Famke Bäuerle <45968370+famosab@users.noreply.github.com>
@SPPearce SPPearce requested a review from famosab May 15, 2024 10:04
Copy link
Contributor

@famosab famosab left a comment

Choose a reason for hiding this comment

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

LGTM

You just need to run prettier on the pytest yml I think

@SPPearce SPPearce enabled auto-merge May 15, 2024 10:39
@SPPearce SPPearce added this pull request to the merge queue May 15, 2024
Merged via the queue into nf-core:master with commit 8da4788 May 15, 2024
24 checks passed
@SPPearce
Copy link
Contributor

Good work @sguizard 🎉

@sguizard
Copy link
Contributor Author

Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants