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

Batch module updates - Summer 2024 #5828

Open
2 of 18 tasks
edmundmiller opened this issue Jun 18, 2024 · 15 comments
Open
2 of 18 tasks

Batch module updates - Summer 2024 #5828

edmundmiller opened this issue Jun 18, 2024 · 15 comments

Comments

@edmundmiller
Copy link
Contributor

edmundmiller commented Jun 18, 2024

Warning

Please open PRs against the batch_update_staging branch. Then we can have small, easy to review, atomic PRs but also merge all batch updates to the modules repo in one go.

Note

[Edit - @ewels]: I've split into two groups, those that will require changes from pipeline developers (other than just pulling in updated modules as normal) and those that will not (those that will require changes to pipeline code).

Anything that doesn't need changes to pipeline code can be done iteratively if we wish, rather than batched.

⚡ Will require pipeline updates ⚡⚡⚡⚡⚡⚡⚡⚡

  1. 0 of 5
  2. 0 of 2
    container-issue

Just a heads up for bulk changes: there is a strict matrix limit of 256 tests in a test strategy on github, so that is the max number of module changes in a PR. @mashehu #2738

@edmundmiller edmundmiller pinned this issue Jun 18, 2024
@ewels ewels changed the title Batch Updates Batch module updates - Summer 2024 Jun 19, 2024
@SPPearce
Copy link
Contributor

Can we automatically swap the test datasets paths too?

@ewels
Copy link
Member

ewels commented Jun 19, 2024

@SPPearce can you elaborate / open an issue about this please? (Or point to an existing one)

@SPPearce
Copy link
Contributor

SPPearce commented Jun 19, 2024

@SPPearce can you elaborate / open an issue about this please? (Or point to an existing one)

We have swapped the file paths used in tests from:
file(params.test_data['sarscov2']['genome']['genome_fasta'], checkIfExists: true)
to direct paths like:
file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta', checkIfExists: true)

This is in the guidelines now, but could do with a bulk update.

Made an issue: #5848

@muffato
Copy link
Member

muffato commented Jun 19, 2024

My concern is that several of the changes require Nextflow 24.04. It means that from this point forward, nf-core will only support Nextflow 24.04+. Is there an analysis of 24.04 vs 22.04-23.10 and what developers have to do to make their pipelines compatible with 24.04 ? Any pipeline developer who is unable to move to 24.04 would remain stuck on pre-June 2024 modules and sub-workflows.

@ewels
Copy link
Member

ewels commented Jun 19, 2024

@muffato this has been policy for a long time now in nf-core. We are not adopting anything that requires an edge release of Nextflow here, everything is stable. There are other updates that are in 24.04 that we also want to update for pipelines, such as the use of resourceLimits. Some of those changes have been requested for ~6 years, and have been present in the Nextflow edge release for ~6 months.

We will soon bump the minimum required version of Nextflow in the pipeline template, and many / most of these changes should come through automatically via template syncs and module updates. We will do our best to write lint tests to cover most of them so that any required changes don't slip through the gaps. We've also recently started the practice of writing blog posts and doing bytesize talks where we talk over and demonstrate the updates that are required for new versions of the template.

I appreciate that it can be frustrating to keep updating pipelines with these kinds of changes. But that's also part of what makes nf-core great. We are a large pool of feature requests to Nextflow which makes it better. Stability is still possible through using previous versions of Nextflow and pipelines. I don't think that we should be held back from implementing new features once they are available in stable versions of Nextflow.

Any pipeline developer who is unable to move to 24.04

The hope is that no-one falls into this camp. If you can see any potential reasons for this, then please say and we can try to resolve them.

@muffato
Copy link
Member

muffato commented Jun 20, 2024

Oh yes @ewels , I don't deny the advantages of moving over. There will be a lot of benefits for people to move. Hopefully we can get away with it by just updating the Nextflow version number and can do pipeline template updates later.

@famosab
Copy link
Contributor

famosab commented Jun 24, 2024

Should this also include adding stubs? (That is not a breaking change but might still be worth to keep in mind?) Related to #4570.

@LilyAnderssonLee LilyAnderssonLee unpinned this issue Jun 25, 2024
@ewels ewels pinned this issue Jun 25, 2024
@ewels
Copy link
Member

ewels commented Jul 5, 2024

Should this also include adding stubs? (That is not a breaking change but might still be worth to keep in mind?) Related to #4570.

Added, thanks!

@Krannich479 Krannich479 unpinned this issue Jul 9, 2024
@mahesh-panchal
Copy link
Member

Can we add, including arity: to modules or is that out of scope? It'll require a more recent minimum version.

@mahesh-panchal mahesh-panchal pinned this issue Jul 10, 2024
@Jorisvansteenbrugge Jorisvansteenbrugge unpinned this issue Jul 17, 2024
@muffato muffato pinned this issue Jul 18, 2024
@bounlu bounlu unpinned this issue Jul 24, 2024
@ewels ewels pinned this issue Aug 29, 2024
@ewels
Copy link
Member

ewels commented Aug 29, 2024

@mahesh-panchal - we will be requiring a pretty recent minimum version of Nextflow for a lot of this (see comments above), so I think that should be fine 👍🏻

Do we have an issue for that? Could you write one if not? Then we can add it to the list above. Thanks!

@mahesh-panchal
Copy link
Member

There's not an issue, but a PR #5915. I'll make an issue.

@mahesh-panchal
Copy link
Member

Added #6454 to list

@GallVp
Copy link
Member

GallVp commented Aug 29, 2024

I also added pytest port #6226 . I have been experimenting with some automation. But there is still quite a bit of work remaining, round 170 single process pytests and around 200 multi-process pytests. @SPPearce has been very helpful in reviewing the PRs.

@chriswyatt1
Copy link
Contributor

Happy to try tackling some of the issues here. Is the plan to tackle this module by module?, or try to script some of the changes, before bulk checking that the modules still function.

@SPPearce
Copy link
Contributor

Happy to try tackling some of the issues here. Is the plan to tackle this module by module?, or try to script some of the changes, before bulk checking that the modules still function.

They need scripted changes, not manual updates per file. It is extremely painful to do anything manually across all ~1000 modules :|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

8 participants