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
Add support for CPU millis for cpus directive #2516
Conversation
This commit adds the ability to CPU millis as unit for the `cpus` directive. Target execution platform should be able to take advantage of this feature otherwise fallback to integer units representing the number of cores. More in details: - when providing an integer value for the `cpus` directive nothing change - when providing a decimal number eg `1.5` platform supporting CPUs millis takes the 1500m value; platforms not supporting it, the millis value is rounded to the nearest upper value ie `2` - when providing an integer number ending with `m` the value is intepreted as CPUs millis, therefore platform supporting it takes the value `1500m`; if the legacy platform takes the nearest upper value ie `2`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want me to run the tests myself as part of the review? I assume they work on your end.
Some future items for the record:
- demonstrate this feature in the Nextflow docs
- add a millis unit so that you can do
cpus = 1000.m
modules/nextflow/src/main/groovy/nextflow/processor/TaskConfig.groovy
Outdated
Show resolved
Hide resolved
modules/nextflow/src/main/groovy/nextflow/processor/TaskConfig.groovy
Outdated
Show resolved
Hide resolved
modules/nextflow/src/main/groovy/nextflow/processor/TaskConfig.groovy
Outdated
Show resolved
Hide resolved
Since we are moving to a world with fractional CPUs request, I've modelled this adding a |
Yep, do need to test it |
Tests pass for me. Ready to merge. I will hold off on documentation until we add request / limit for cpus. Side note for docs: you must use |
Going over the related issues #773, @pditommaso is the situation you described here still covered?
In other words, what will |
From the same thread, this is what I recommend:
We can explain this in the docs. Basically the Edit: To clarify, I don't think you would ever need to have the exact milli-cpus available in the |
Um no, in this PR |
Not sure to understand the rationale of the last comment. Why if it was specified |
Honestly... I don't think it matters whether |
Disagree, I've seen a lot of use cases in which a task declaring, let's say 4, in reality, it only has a peak using 4 cpus and on average less. I think in these cases declaring a fraction can help to better tune the real usage. |
So in that case we still want to round up the fractional cpu for |
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@pditommaso This PR is ready to be merged. We agreed that |
This commit adds the ability to CPU millis as unit for the `cpus` directive. Target execution platform should be able to take advantage of this feature otherwise fallback to integer units representing the number of cores. More in details: - when providing an integer value for the `cpus` directive nothing change - when providing a decimal number eg `1.5` platform supporting CPUs millis takes the 1500m value; platforms not supporting it, the millis value is rounded to the nearest upper value ie `2` - when providing an integer number ending with `m` the value is intepreted as CPUs millis, therefore platform supporting it takes the value `1500m`; if the legacy platform takes the nearest upper value ie `2`.
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@pditommaso This PR is updated with the latest changes. I also added fractional CPU support to AWS Batch. Ready for your to review. |
Just confirmed that decimal values don't work for AWS Batch:
So it looks like decimal values must be less than 1, which is only supported by Fargate. I will revert these changes. |
This reverts commit 5d576b5.
2c461f8
to
23432f3
Compare
e2b4a93
to
f32ea0b
Compare
cefb067
to
e523afd
Compare
0d59b4c
to
b93634e
Compare
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@pditommaso I updated and cleaned up this PR. To review, it adds fractional CPU support to the Local, K8s, and Google Batch executors. Otherwise fractional requests are rounded to the next integer. The I think it is good to go. Do you have any remaining concerns? |
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
81f7cb7
to
8a43489
Compare
@pditommaso I think we can close this PR. I think the original use cases can be covered in other ways:
// submit 100 tasks with 0.01 cpus, lots of overhead
cpus = '10m'
// submit one task batch with 1 cpu, much more efficient
batch = 100
cpus = 1 |
Agree |
This commit adds the ability to CPU millis as unit for the
cpus
directive.Target execution platforms should be able to take advantage of this feature, otherwise, it falls back to integer units representing the number of cores.
More in details:
cpus
directive nothing change1.5
platform supporting CPUs millistakes the 1500m value; platforms not supporting it, the millis value is
rounded to the nearest upper value ie
2
m
the value is intepretedas CPUs millis, therefore platform supporting it takes the value
1500m
;if the legacy platform takes the nearest upper value ie
2
.