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

Replace check_max function with resourceLimits directive #3037

Merged
merged 22 commits into from
Sep 20, 2024

Conversation

jfy133
Copy link
Member

@jfy133 jfy133 commented Jun 25, 2024

Closes #2923

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

Copy link
Contributor

This PR is against the master branch ❌

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @jfy133,

It looks like this pull-request is has been made against the jfy133/nf-core-tools master branch.
The master branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master are only allowed if they come from the jfy133/nf-core-tools dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

@jfy133 jfy133 changed the base branch from master to dev June 25, 2024 12:54
CHANGELOG.md Outdated Show resolved Hide resolved
@jfy133
Copy link
Member Author

jfy133 commented Jun 25, 2024

I tested locally by running the nf-core/test-pipeline example in the CI.

By adjusting --max_memory to 16, the FASTQC process that would normally get 32GB (label_medium) was indeed capped at 16 (normally it would be allocated 32)

image

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Let's just remove params.max_* entirely 🔥

@maxulysse
Copy link
Member

Let's just remove params.max_* entirely 🔥

Just don't remove me please

@jfy133
Copy link
Member Author

jfy133 commented Sep 4, 2024

OK the tasks from the slack thread here

  • Remove all references to --max_* parameters
  • Add lint tests (so people remove the parameters)
  • Set new defaults in tests configs
  • Update the docs on the website
    • Add warning about legacy useage
    • New docs to describe how to use new resourceDirectives
  • mass update all configs on nf-core/configs
    • Keep old --max_* parameters (buit leave comment)
    • Copy vbalues to new resourcesLimits (with

@jfy133
Copy link
Member Author

jfy133 commented Sep 4, 2024

@nf-core-bot fix linting

@mirpedrol
Copy link
Member

This is ready to merge, ok if we merge it @jfy133 ?
We have to change the branch protection for the two pending tests, but I want to do it right before merging, otherwise it will block other PRs.

@jfy133
Copy link
Member Author

jfy133 commented Sep 19, 2024

Yes I think we can do but we Just have to be careful that people don't try to use dev to generate pipelines because configs won't work, I'll try and work out a mass configs PR ASAP.

Docs can come later I guess

@mirpedrol
Copy link
Member

ok I will merge it now! It won't affect old pipelines so it should be good 👍

@mirpedrol mirpedrol merged commit b2e6397 into nf-core:dev Sep 20, 2024
83 checks passed
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.

Replace check_max with resourceLimits
5 participants