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
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 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1cce8f1
Correct memory units from MB to MiB
dpoznik Feb 28, 2025
4fc5107
Correct Kubernetes memory units from MB to MiB
dpoznik Feb 28, 2025
bfaa672
Update Kubernetes units from MB to MiB
dpoznik Mar 4, 2025
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
2 changes: 1 addition & 1 deletion 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
2 changes: 1 addition & 1 deletion 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/aws/batch/batch_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,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
14 changes: 7 additions & 7 deletions metaflow/plugins/kubernetes/kubernetes_decorator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,34 +11,34 @@
from metaflow.metadata_provider.util import sync_local_metadata_to_datastore
from metaflow.metaflow_config import (
DATASTORE_LOCAL_DIR,
KUBERNETES_ANNOTATIONS,
KUBERNETES_CONDA_ARCH,
KUBERNETES_CONTAINER_IMAGE,
KUBERNETES_CONTAINER_REGISTRY,
KUBERNETES_CPU,
KUBERNETES_DISK,
KUBERNETES_FETCH_EC2_METADATA,
KUBERNETES_GPU_VENDOR,
KUBERNETES_IMAGE_PULL_POLICY,
KUBERNETES_MEMORY,
KUBERNETES_LABELS,
KUBERNETES_ANNOTATIONS,
KUBERNETES_MEMORY,
KUBERNETES_NAMESPACE,
KUBERNETES_NODE_SELECTOR,
KUBERNETES_PERSISTENT_VOLUME_CLAIMS,
KUBERNETES_PORT,
KUBERNETES_QOS,
KUBERNETES_SERVICE_ACCOUNT,
KUBERNETES_SHARED_MEMORY,
KUBERNETES_TOLERATIONS,
KUBERNETES_QOS,
KUBERNETES_CONDA_ARCH,
)
from metaflow.plugins.resources_decorator import ResourcesDecorator
from metaflow.plugins.timeout_decorator import get_run_time_limit_for_task
from metaflow.sidecar import Sidecar
from metaflow.unbounded_foreach import UBF_CONTROL

from ..aws.aws_utils import get_docker_registry, get_ec2_instance_metadata
from .kube_utils import parse_kube_keyvalue_list, validate_kube_labels
from .kubernetes import KubernetesException
from .kube_utils import validate_kube_labels, parse_kube_keyvalue_list

try:
unicode
Expand All @@ -59,11 +59,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 +62 to +66
Copy link
Collaborator

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
Collaborator

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
Contributor Author

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
Contributor Author

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
Collaborator

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
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
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