-
Notifications
You must be signed in to change notification settings - Fork 765
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
Adding 'disk' parameter to @resources decorator #1500
Adding 'disk' parameter to @resources decorator #1500
Conversation
"cpu": "1", | ||
"gpu": "0", | ||
"memory": "4096", | ||
"shared_memory": None, |
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.
#1504 opened for this, the shared_memory
is not yet supported for Kubernetes.
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.
https://outerbounds-community.slack.com/archives/C020U025QJK/p1691751137791999 earlier discussions with context on why disk
has been missing.
Added some context and an addition to the docstring. I'll give this a more thorough spin tomorrow morning :) |
Testing[479] @ c5a7817 |
Testing[479] @ c5a7817 had 1 FAILURE. |
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. |
An easy solution for the overriding behavior would be to instead have
as a default for the resources decorator. If this is acceptable, also update the docstring to mark the attrib as optional. |
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. |
Testing[479] @ 4d30041 |
Testing[479] @ 4d30041 had 2 FAILUREs. |
found one issue with more thorough testing
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
This seems to be due to the way we inject attributes for the batch deco via Same issue affects step-functions as well |
I found that the logic for checking decorator attributes is in the base class of the decorator Something like this:
|
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.
should be good to go with the merged fixes to batch/sfn resource decorator handling.
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.