Adding 'disk' parameter to @resources decorator

[jaypond]

We would like to use the @resources decorator instead of @kubernetes to specify resource requirements for our job so we can run flows locally or on Kubernetes by using --with kubernetes instead of modifying the code. The @resources decorator only supports cpu, gpu, memory and shared_memory at the moment.

This PR just adds the disk parameter. Default value (10240mb) is the same as the one in @kubernetes.

[saikonen]

#1504 opened for this, the shared_memory is not yet supported for Kubernetes.

[saikonen]

https://outerbounds-community.slack.com/archives/C020U025QJK/p1691751137791999 earlier discussions with context on why disk has been missing.

[saikonen]

Added some context and an addition to the docstring. I'll give this a more thorough spin tomorrow morning :)

[nflx-mf-bot]

Testing[479] @ c5a7817

[nflx-mf-bot]

Testing[479] @ c5a7817 had 1 FAILURE.

[saikonen]

As one last discussion point, we do need to consider the impact of this change as it is a breaking one in the following scenario due to how attribute overriding works between the resources and kubernetes decorators. Consider someone who has an existing flow as such:

from metaflow import step, FlowSpec, resources, kubernetes


class HelloFlow(FlowSpec):
    @resources
    @kubernetes(disk=4096)
    @step
    def start(self):
        print("Starting 👋")
        self.next(self.end)

    @step
    def end(self):
        print("Done! 🏁")


if __name__ == "__main__":
    HelloFlow()

prior to this change they would allocate only 4096 of disk, after this PR is introduced the default 10240 from resources will override this value due to being larger.

[saikonen]

An easy solution for the overriding behavior would be to instead have

"disk": None,

as a default for the resources decorator. If this is acceptable, also update the docstring to mark the attrib as optional.
Was there a specific reason to use the kubernetes default also for the resources decorator?

[jaypond]

Good catch. Just tested and pushed the update you recommended.

No particular reason for using the same default disk value for the resources decorator. I have no issues having disk as none by default.

[nflx-mf-bot]

Testing[479] @ 4d30041

[nflx-mf-bot]

Testing[479] @ 4d30041 had 2 FAILUREs.

found one issue with more thorough testing

[saikonen]

Encountered one last issue with this feature while testing on other platforms. Specifically with running on batch while using the resource deco, it now fails with an error of

Decorator 'batch' does not support the attribute 'disk'. These attributes are supported: cpu, gpu, memory, image, queue, iam_role, execution_role, shared_memory, max_swap, swappiness, inferentia, host_volumes, use_tmpfs, tmpfs_tempdir, tmpfs_size, tmpfs_path.

This seems to be due to the way we inject attributes for the batch deco via aws_utils.py#compute_resource_attributes() which does not have a distinction between supported or unsupported attributes.

Same issue affects step-functions as well

[jaypond]

I found that the logic for checking decorator attributes is in the base class of the decorator decorator.py. Would it be feasible if we just remove attributes which aren't supported by a decorator instead of throwing an error? Maybe throw a warning instead?

Something like this:

class Decorator(object):
    """
    Base class for all decorators.
    """

    name = "NONAME"
    defaults = {}
    # `allow_multiple` allows setting many decorators of the same type to a step/flow.
    allow_multiple = False

    def __init__(self, attributes=None, statically_defined=False):
        self.attributes = self.defaults.copy()
        self.statically_defined = statically_defined

        if attributes:
            for k, v in attributes.items():
                if k in self.defaults:
                    self.attributes[k] = v
                else:
                    #raise InvalidDecoratorAttribute(self.name, k, self.defaults)
                    del self.attributes[k]

[saikonen]

should be good to go with the merged fixes to batch/sfn resource decorator handling.