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

Change falco process into medium, solve memory issue #334

Merged
merged 4 commits into from
Jul 24, 2023
Merged

Change falco process into medium, solve memory issue #334

merged 4 commits into from
Jul 24, 2023

Conversation

LilyAnderssonLee
Copy link
Contributor

PR checklist

Close #291

  • 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/taxprofiler 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).

@LilyAnderssonLee LilyAnderssonLee changed the title Change falco process into medium conf/base.config Change falco process into medium, solve memory issue Jul 21, 2023
@github-actions
Copy link

github-actions bot commented Jul 21, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit ae543dc

+| ✅ 157 tests passed       |+
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-07-21 13:46:36

sofstam
sofstam approved these changes Jul 21, 2023
View reviewed changes
Copy link
Collaborator

@sofstam sofstam left a comment

Choose a reason for hiding this comment

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

Can be merged once tests pass and if this is why @jfy133 also had in mind about bumping into taxprofiler :)

@sofstam
Copy link
Collaborator

sofstam commented Jul 21, 2023

Also, can you update the changelog as well? :)

@LilyAnderssonLee
Copy link
Contributor Author

Updated CHANGELOG.md

jfy133
jfy133 requested changes Jul 21, 2023
View reviewed changes
conf/base.config Outdated
}
withName: FALCO {
cpus = { check_max( 6 , 'cpus' ) }
memory = { check_max( 36.GB * task.attempt, 'memory' ) }
Copy link
Member

Choose a reason for hiding this comment

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

I think this is overkill! 1GB to 36GB is too much as it's actually small job (just needs a bit more than fastqC) , maybe increase it to 4.GB

Copy link
Contributor Author

@LilyAnderssonLee LilyAnderssonLee Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

FASTQC is a medium process.
Maybe it would be safer to change it to a low process with 12GB memory?

Copy link
Member

Choose a reason for hiding this comment

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

That still seems high... and FastQC being a medium process is a bit mof a misnomer, increasing threads for FastQC is only to increase the memory (1 CPU == 500 MB RAM... don't ask me why...).

Falco is still multithreaded so keep the CPUs :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will change the memory to 4GB and keep the cpu 6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfy133 According to this paper, Falco is known for its memory efficiency, and allocating 4GB of memory would indeed be a reasonable number. Additionally, Falco has been reported to utilize approximately 14 times less memory than FastQC when processing long-reads.
So the chosen memory setting of 4GB appears to be well-suited for running Falco efficiently and effectively for your analysis.

https://www.ncbi.nlm.nih.gov/pmc/articles/PMC7845152/

If you think it's fine, can you approve it?

@jfy133 jfy133 merged commit 4627208 into nf-core:dev Jul 24, 2023
@LilyAnderssonLee LilyAnderssonLee self-assigned this Aug 10, 2023
@LilyAnderssonLee LilyAnderssonLee added the enhancement Improvement for existing functionality label Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sofstam sofstam sofstam approved these changes

@jfy133 jfy133 jfy133 approved these changes

Assignees

@LilyAnderssonLee LilyAnderssonLee

Labels
enhancement Improvement for existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LilyAnderssonLee @sofstam @jfy133