Skip to content

Correct memory units from MB to MiB #2309

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

Open
dpoznik wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Correct memory units from MB to MiB #2309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
01b2936
Update memory units from MB to MiB
dpoznik Jul 23, 2025
0e002cc
Update memory units in comments
dpoznik Mar 5, 2026
3b9c78c
Update memory units to "Mi" in Argo Workflows
dpoznik Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions R/R/decorators-aws.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
#'
#' @param cpu Integer number of CPUs required for this step. Defaults to `1`.
#' @param gpu Integer number of GPUs required for this step. Defaults to `0`.
#' @param memory Integer memory size (in MB) required for this step. Defaults to
#' @param memory Integer memory size (in MiB) required for this step. Defaults to
#' `4096`.
#' @param image Character. Specifies the image to use when launching on AWS
#' Batch. If not specified, an appropriate
Expand Down Expand Up @@ -49,8 +49,8 @@
#'
#' @examples \dontrun{
#' # This example will generate a large random matrix which takes up roughly
#' # 48GB of memory, and sums the entries. The `batch` decorator forces this
#' # step to run in an environment with 60000MB of memory.
#' # 48 GiB of memory, and sums the entries. The `batch` decorator forces this
#' # step to run in an environment with 60,000 MiB of memory.
#'
#' start <- function(self) {
#' big_matrix <- matrix(rexp(80000*80000), 80000)
Expand Down
6 changes: 3 additions & 3 deletions R/man/batch.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion metaflow/plugins/argo/argo_workflows.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3003,7 +3003,7 @@ def _container(cmds):
resources=kubernetes_sdk.V1ResourceRequirements(
requests={
"cpu": str(kube_defaults["cpu"]),
"memory": "%sM" % str(kube_defaults["memory"]),
"memory": "%sMi" % str(kube_defaults["memory"]),
}
),
).to_dict()
Expand Down
2 changes: 1 addition & 1 deletion metaflow/plugins/aws/batch/batch_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class BatchDecorator(StepDecorator):
Number of GPUs required for this step. If `@resources` is
also present, the maximum value from all decorators is used.
memory : int, default 4096
Memory size (in MB) required for this step. If
Memory size (in MiB) required for this step. If
`@resources` is also present, the maximum value from all decorators is
used.
image : str, optional, default None
Expand Down
8 changes: 4 additions & 4 deletions metaflow/plugins/kubernetes/kube_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ def qos_requests_and_limits(qos: str, cpu: int, memory: int, storage: int):
# Guaranteed - has both cpu/memory limits. requests not required, as these will be inferred.
qos_limits = {
"cpu": str(cpu),
"memory": "%sM" % str(memory),
"ephemeral-storage": "%sM" % str(storage),
"memory": "%sMi" % str(memory),
"ephemeral-storage": "%sMi" % str(storage),
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 5, 2026

Choose a reason for hiding this comment

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

ephemeral-storage behavior change not called out in the PR description

The PR description explicitly calls out the memory unit change (M → Mi) as a behavior change, but the parallel change to ephemeral-storage (the disk parameter) on this line is equally a behavioral change and isn't mentioned. Like the memory change, it means Kubernetes will now allocate ~4.86% more ephemeral storage per pod than before. Users with tight cluster storage quotas or namespace ResourceQuota limits on requests.ephemeral-storage may be affected.

Consider adding a note to the PR description / release notes to cover this, e.g.:

This also applies to the disk/ephemeral-storage parameter: values are now interpreted as MiB (Mi) instead of megabytes (M) in Kubernetes.

Copy link
Copy Markdown
Contributor Author

@dpoznik dpoznik Mar 5, 2026

Choose a reason for hiding this comment

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

Updated PR description accordingly

}
# NOTE: Even though Kubernetes will produce matching requests for the specified limits, this happens late in the lifecycle.
# We specify them explicitly here to make some K8S tooling happy, in case they rely on .resources.requests being present at time of submitting the job.
Expand All @@ -53,8 +53,8 @@ def qos_requests_and_limits(qos: str, cpu: int, memory: int, storage: int):
# Burstable - not Guaranteed, and has a memory/cpu limit or request
qos_requests = {
"cpu": str(cpu),
"memory": "%sM" % str(memory),
"ephemeral-storage": "%sM" % str(storage),
"memory": "%sMi" % str(memory),
"ephemeral-storage": "%sMi" % str(storage),
}
# TODO: Add support for BestEffort once there is a use case for it.
# BestEffort - no limit or requests for cpu/memory
Expand Down
4 changes: 2 additions & 2 deletions metaflow/plugins/kubernetes/kubernetes_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ class KubernetesDecorator(StepDecorator):
Number of CPUs required for this step. If `@resources` is
also present, the maximum value from all decorators is used.
memory : int, default 4096
Memory size (in MB) required for this step. If
Memory size (in MiB) required for this step. If
`@resources` is also present, the maximum value from all decorators is
used.
disk : int, default 10240
Disk size (in MB) required for this step. If
Disk size (in MiB) required for this step. If
Comment on lines +64 to +68
Copy link
Copy Markdown
Collaborator

@saikonen saikonen Mar 4, 2025

Choose a reason for hiding this comment

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

I had a peek at the units, and it seems that memory/disk values for kubernetes are treated as MB so the earlier docstring for these was correct.

we could hold off on changing the kubernetes units in the docs for now, as this would require also changing the implementation to match.

Copy link
Copy Markdown
Collaborator

@saikonen saikonen Mar 4, 2025

Choose a reason for hiding this comment

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

alternatively changing the units from M to Mi in kube_utils.py#qos_requests_and_limits should be the only additional change required, as this is used both by the kubernetes and argo workflows implementations.

Copy link
Copy Markdown
Contributor Author

@dpoznik dpoznik Mar 4, 2025

Choose a reason for hiding this comment

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

alternatively changing the units from M to Mi in kube_utils.py#qos_requests_and_limits should be the only additional change required, as this is used both by the kubernetes and argo workflows implementations.

Cool, thanks for taking a look at the Kubernetes units. Since the defaults are multiples of 1024, MiB seems to be the original intent. So I think it makes sense to update the implementation, especially if doing so is as easy as you've described. Accordingly, I've pushed up a new commit: bfaa672. But, if you prefer, I could revert bfaa672 and 4fc5107 and update the @resources docstring to indicate that the memory units are MiB for AWS Batch and MB for Kubernetes. Just let me know your preference. Thanks!

Copy link
Copy Markdown
Contributor Author

@dpoznik dpoznik Mar 4, 2025

Choose a reason for hiding this comment

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

Fwiw, the Kubernetes "Assign Memory Resources to Containers and Pods" example uses MiB, potentially indicating that MiB is more standard / expected there. Maybe.

`@resources` is also present, the maximum value from all decorators is
used.
image : str, optional, default None
Expand Down
4 changes: 2 additions & 2 deletions metaflow/plugins/resources_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ class ResourcesDecorator(StepDecorator):
gpu : int, optional, default None
Number of GPUs required for this step.
disk : int, optional, default None
Disk size (in MB) required for this step. Only applies on Kubernetes.
Disk size (in MiB) required for this step. Only applies on Kubernetes.
memory : int, default 4096
Memory size (in MB) required for this step.
Memory size (in MiB) required for this step.
Copy link
Copy Markdown
Collaborator

@savingoyal savingoyal Feb 28, 2025

Choose a reason for hiding this comment

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

given that these values are used in @kubernetes as well - would be good to ensure it's the case there too. also what about disk?

Copy link
Copy Markdown
Contributor Author

@dpoznik dpoznik Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've not used Kubernetes before, so I wasn't sure about the three instances I left unchanged:

  • @kubernetes memory
  • @kubernetes disk
  • @resources disk (used only for Kubernetes)

I am pretty sure this one (@resources memory) should be MiB for AWS Batch. That may also be the case for Kubernetes, but I don't know.

Copy link
Copy Markdown
Contributor Author

@dpoznik dpoznik Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The @kubernetes defaults, 4096 for memory (4 × 1024) and 10240 (10 × 1024) for disk, very strongly suggest that MiB was intended there as well, so I've updated the remaining three instances of "(in MB)" in 4fc5107.

shared_memory : int, optional, default None
The value for the size (in MiB) of the /dev/shm volume for this step.
This parameter maps to the `--shm-size` option in Docker.
Expand Down
Loading