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

fix: conda environments #90

Merged
merged 14 commits into from
Nov 15, 2024
Merged

fix: conda environments #90

merged 14 commits into from
Nov 15, 2024

Conversation

znorgaard
Copy link
Collaborator

@znorgaard znorgaard commented Nov 8, 2024 •
edited
Loading

Also includes #93

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/fastquorum branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nf-test test main.nf.test -profile test,docker).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,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 GitHub Actions
Copy link

github-actions bot commented Nov 8, 2024 •
edited
Loading

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 9e604d8

+| ✅ 193 tests passed       |+
#| ❔   8 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 3.0.2
  • Run at 2024-11-15 17:35:07

@znorgaard znorgaard marked this pull request as ready for review November 8, 2024 20:48
@znorgaard znorgaard requested a review from SPPearce November 8, 2024 23:16
@znorgaard znorgaard mentioned this pull request Nov 8, 2024
Merged
11 tasks
SPPearce
SPPearce reviewed Nov 11, 2024
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
@SPPearce
Copy link
Contributor

At some point we should move to the nf-core modules repo versions of the fgbio modules that are in there (i.e. the ones that aren't a combination of two subtools.
Currently we have one nf-core module from fgbio, fgbio sortbam. If we want to update the version of that like you have here, it is good practice to then use a patch diff (nf-core modules diff) to ensure that things stay in sync as we update the nf-core modules. It'd be good generally to update all the nf-core modules to 2.4.0 on the modules repo, then that wouldn't be needed.

Not sure why the defaults channels (and the name: field) have turned back up in the nf-core modules, what did you do to update those? There is a technical reason to remove the defaults (something to do with anaconda requiring a license, I can dig out the information if you want).

@znorgaard znorgaard requested a review from SPPearce November 15, 2024 16:15
SPPearce
SPPearce approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@SPPearce SPPearce left a comment

Choose a reason for hiding this comment

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

I've just added patch diff files by running nf-core modules patch on each module - these will ensure that the changes to the environment will remain if we update the modules from nf-core.

CHANGELOG.md
Comment on lines +17 to +25
| Module | Previous SHA | New SHA |
| -------------- | ------------ | ------- |
| bwa/index | e0ff65e | 6666521 |
| fastqc | b49b899 | 6666521 |
| fgbio/sortbam | 2fc7438 | bc6d86f |
| multiqc | fe9614c | cf17ca4 |
| samtools/dict | 3c8fd07 | b13f07b |
| samtools/faidx | 04fbbc7 | b13f07b |
| samtools/merge | 04fbbc7 | b13f07b |
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these SHAs coming from?
I'd be happy with the table above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nf-core modules list local

@znorgaard
Copy link
Collaborator Author

znorgaard commented Nov 15, 2024 •
edited
Loading

Something about the stacked PR merge is making linting break. Going to revert that merge to see if it will fix the problem. Shouldn't impact anything else here.

Looks like nf-core pipelines lint doesn't like the diff files. Going to see if it's a specific module or any diff file.

It's specifically the "----" line in the samtools diffs.

nf-core/tools#3281

@znorgaard znorgaard merged commit 9aa88d0 into dev Nov 15, 2024
8 checks passed
@znorgaard znorgaard deleted the zn_conda branch December 3, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SPPearce SPPearce SPPearce approved these changes

Assignees

@SPPearce SPPearce

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@znorgaard @SPPearce