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

Adding plasflow module #3555

Closed
wants to merge 84 commits into from
Closed

Adding plasflow module #3555

wants to merge 84 commits into from

Conversation

limrp
Copy link
Contributor

@limrp limrp commented Jun 24, 2023

A new module for the cram-size command of the samtools tool has been added.

@limrp limrp marked this pull request as draft June 24, 2023 21:38
@limrp limrp marked this pull request as ready for review June 24, 2023 21:54
@limrp limrp changed the title Adding module for cram-size command of the samtools tool Adding module for cram-size command of samtools Jun 24, 2023
modules/nf-core/samtools/cramsize/main.nf Outdated Show resolved Hide resolved
modules/nf-core/samtools/cramsize/main.nf Outdated Show resolved Hide resolved
modules/nf-core/samtools/cramsize/main.nf Outdated Show resolved Hide resolved
modules/nf-core/samtools/cramsize/main.nf Outdated Show resolved Hide resolved
tests/modules/nf-core/samtools/cramsize/test.yml Outdated Show resolved Hide resolved
@nvnieuwk
Copy link
Contributor

Very good job already! Here are some comments to make this module adhere to the guidelines set up by nf-core. Feel free to ping me if you have any questions

@priyanka-surana priyanka-surana self-requested a review June 30, 2023 15:12
limrp and others added 10 commits July 6, 2023 12:31
Adding name of the tool

Co-authored-by: Nicolas Vannieuwkerke <[email protected]>
Adding prefix to make the module set up more flexible.

Co-authored-by: Nicolas Vannieuwkerke <[email protected]>
Changing `.baseName` by `prefix` in output names.

Co-authored-by: Nicolas Vannieuwkerke <[email protected]>
- Stub section added to main.nf script.
- test.yml file added to module samtools/cramsize
@nvnieuwk
Copy link
Contributor

Hi some tests are still failing, can you fix these before I do a final review? Also it's easier to get reviews when you have one module per PR (don't worry about it for now, just some advice for the future 😉)

@limrp
Copy link
Contributor Author

limrp commented Jul 28, 2023

Hi some tests are still failing, can you fix these before I do a final review? Also it's easier to get reviews when you have one module per PR (don't worry about it for now, just some advice for the future wink)

Thank you for checking! I'll review the test that are failing 😃
Sorry for the 2 modules in one PR. I'll be more careful in the future 🥲 😺

maxulysse and others added 9 commits July 28, 2023 01:33
* Update manta somatic

* Update manta tumoronly
* add call subcommand

* at calls runs now [skip ci]

* add test files

* linting

* fix tests

* add contains check

* Update tests/modules/nf-core/varlociraptor/callvariants/main.nf

Co-authored-by: Maxime U Garcia <[email protected]>

---------

Co-authored-by: Maxime U Garcia <[email protected]>
* add module template

* first version

* update testing and linting

* update file naming

* update test

* fix test
* ngmerge first commit

* main module code

* complete module

* remove trailing whitespace

* fix versions.yml

* fix version yml

* versions yml

* vyml

* try vyml again

* touch versions yml

* vyml
* initial commit

* module version 1

* full testing

* linting

* fix version yaml

* fix version yaml command

* fix version yml maybe this time please

* fix version yml for real this time

* try again versions yaml

* fix version yml

* vyml

* version yaml

* vyml

* update ref to have meta
* new module picard/scatterintervalsbyns

* update annotsv to 3.3.6

* update installannotations

* update vardictjava
@limrp
Copy link
Contributor Author

limrp commented Sep 27, 2023

Tests for plasflow are still failing if you fixed those I'll give you a ✔️ :p

Hi @nvnieuwk !
The only tests that are failing are the ones related to Plasflow from Conda. There is no issue with Plasflow from Docker and Singularity. There is also no problem with Samtools' cramsize.

I attempted to fix Plasflow's recipe in the Bioconda repository. I needed to specify the required Python version (3.5) for Plasflow to function.

Here is the link: bioconda/bioconda-recipes#42697

However, I was given the following message:

DEPRECATION: Python 3.5 reached the end of its life on September 13th, 2020. Please upgrade your Python as Python 3.5 is no longer maintained. pip 21.0 will drop support for Python 3.5 in January 2021. pip 21.0 will remove support for this functionality.
ERROR: tensorflow-0.10.0rc0-cp35-cp35m-linux_x86_64.whl is not a supported wheel on this platform.

It seems that it will not be possible to make Plasflow work with conda because conda will not support Python 3.5 soon. But everything else works fine.
Do you think that in a case like this is it possible to accept the PR?

@nvnieuwk
Copy link
Contributor

Can you add the tool to be excluded from the conda tests in .github/workflows/test.yml? You'll see it in the pytest clause. This way conda won't be tested (this should only be used for tools that absolutely don't work with conda though). After that all tests should pass and you'll be ready to go. Thanks for all the effort in trying to fix it

@limrp
Copy link
Contributor Author

limrp commented Sep 28, 2023

Can you add the tool to be excluded from the conda tests in .github/workflows/test.yml? You'll see it in the pytest clause. This way conda won't be tested (this should only be used for tools that absolutely don't work with conda though). After that all tests should pass and you'll be ready to go. Thanks for all the effort in trying to fix it

Hi @nvnieuwk ! Thanks for your reply!

Are you refering to this section of the test.yml file?

image

Should I add Plasflow to that list?

Thanks a lot for all you advice!

@nvnieuwk
Copy link
Contributor

Yes, that's it!

@edmundmiller edmundmiller requested a review from a team as a code owner October 19, 2023 16:26
@SPPearce SPPearce mentioned this pull request Jun 3, 2024
17 tasks
@SPPearce SPPearce changed the title Adding module for cram-size command of samtools Adding plasflow module Jun 4, 2024
@SPPearce SPPearce marked this pull request as draft June 12, 2024 10:37
@SPPearce SPPearce added the stale Stale label Aug 13, 2024
@SPPearce
Copy link
Contributor

I'm going to close this for now, as there seems to be no response from @limrp .
I moved the samtools cramsize module out into a separate PR and merged that, but I can't immediately see a way to get plasflow to work properly

@SPPearce SPPearce closed this Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.