Correct memory units from MB to MiB

[dpoznik]

The docs currently suggest that the @batch and @resources memory units are MB, but I suspect they may be MiB.

For AWS Batch computes, I believe these parameters are passed directly to ResourceRequirement, like so:

      "ResourceRequirements": [
        {
          "Value": "1",
          "Type": "VCPU"
        },
        {
          "Value": "4096",
          "Type": "MEMORY"
        }
      ],

and the ResourceRequirement docs indicate memory values are in MiB.

Furthermore, the default of 4096 makes sense to me if the units are MiB, since 4096 MiB = 4 GiB. If MB, it's hard to interpret.

This PR originally proposed changes to four of seven instances of "(in MB)". Upon further reflection, it now includes the remaining three, Kubernetes-specific, instances.

[savingoyal]

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?

[dpoznik]

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.

[dpoznik]

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.

[saikonen]

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.

[saikonen]

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.

[dpoznik]

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!

[dpoznik]

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

[saikonen]

As a heads up I'll be looking into some other inconsistencies of @resources as well next week that have been on the backlog for a while now, and will hold off on merging this PR for a bit in case that prompts more changes.

For now this PR seems good to go as is so no further action required 👌

[dpoznik]

As a heads up I'll be looking into some other inconsistencies of @resources as well next week that have been on the backlog for a while now, and will hold off on merging this PR for a bit in case that prompts more changes.

For now this PR seems good to go as is so no further action required 👌

OK, great. Thanks for the update!