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

Create patches for umi-tools #983

Closed
wants to merge 1 commit into from

Conversation

MatthiasZepper
Copy link
Member

This PR is a hotfix for the 3.11 release candidate to add additional process labels to umi_tools extract (process long) and umi_tools dedup (process high_memory) as a temporary solution to fix errors during pipeline execution until a more appropriate process label has been added to the modules.

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/rnaseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile 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).

Copy link
Member

@drpatelh drpatelh left a comment

Choose a reason for hiding this comment

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

Thanks! Be awesome if we can just update these on nf-core/modules and re-install here?

@MatthiasZepper
Copy link
Member Author

Sure, but how?

If we change the module, we might want to revert it to process_medium or even add new, more atomic labels? I don't think that this is a good permanent solution, actually...

@drpatelh
Copy link
Member

drpatelh commented Mar 29, 2023

You would create a PR to nf-core/modules with exactly the same changes and then use nf-core modules install to install in the pipeline.

If we change the module, we might want to revert it to process_medium or even add new, more atomic labels? I don't think that this is a good permanent solution, actually...

Are the current label definitions breaking for you on dev? If so, maybe we do need to update. If not, then I think we can leave things as they are and update in a future release. We can suggest to override the defaults with a custom config until we have fixed.

@MatthiasZepper
Copy link
Member Author

MatthiasZepper commented Mar 29, 2023

Yes, the release candidate version of the pipeline is failing for me with the uppmax profile on a small human sample: umi_tools extract exceeds the runtime and umi_tools dedup the memory limits of process_single as we have defined it in the base.config. This was the reason for my proposal on Slack you have already commented on.

@MatthiasZepper
Copy link
Member Author

Opened a correction PR in the modules repo.

@MatthiasZepper
Copy link
Member Author

Obsolete, as the required changes were directly done to the modules instead of applying patches.

@MatthiasZepper MatthiasZepper deleted the patch_umitools branch March 30, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants