-
Notifications
You must be signed in to change notification settings - Fork 300
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
Introduce pod_spec_from_resources()
ray helper function
#2943
Conversation
2778db2
to
b79cea4
Compare
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
f2b811c
to
da7d6ae
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2943 +/- ##
===========================================
- Coverage 79.32% 46.65% -32.67%
===========================================
Files 199 200 +1
Lines 20870 20962 +92
Branches 2684 2709 +25
===========================================
- Hits 16555 9780 -6775
- Misses 3566 10694 +7128
+ Partials 749 488 -261 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
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.
Just a few comments, nothing major.
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
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.
Sorry for flip-flopping on this, I only realized that we were removing the pod spec template. We should instead build helper functions around that, but still produce pod specs that get passed to the ray idl objects.
flytekit/core/resources.py
Outdated
requests: Optional[Resources], | ||
limits: Optional[Resources], | ||
) -> dict[str, Any]: | ||
def _construct_k8s_pods_resources(resources: Optional[Resources], k8s_gpu_resource_key: str = "nvidia.com/gpu"): |
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.
Using other gpus is going to be hard, even if we push this parameter to the outer function (i.e. construct_k8s_pod_spec_from_resources
).
): | ||
self._group_name = group_name | ||
self._replicas = replicas | ||
self._max_replicas = max(replicas, max_replicas) if max_replicas is not None else replicas | ||
self._min_replicas = min(replicas, min_replicas) if min_replicas is not None else replicas | ||
self._ray_start_params = ray_start_params | ||
self._k8s_pod = k8s_pod |
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.
We should keep this as part of the interface and build helper functions that construct valid pod specs instead (as mentioned in the original flyte PR). This is going to help in the other problem we're having with passing the gpu resource name around (in other words, gpu can be an argument of one of the helper function that builds pod specs).
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.
I get what you are saying. So we want users to construct the pod specs themself like calling construct_k8s_pod_spec_from_resources()
or specifying pod templates in user code?
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.
I would make the method name simple, maybe pod from resources
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
pod_spec_from_resources()
ray helper function
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.
Thank you!
Tracking issue
Related to flyteorg/flyte#5666
Why are the changes needed?
These changes update the flytekit ray plugin to let the user construct Ray pod definition with an introduced helper function called
pod_spec_from_resources()
. This function allows setting setting Resources for Ray Head & Worker nodesWhat changes were proposed in this pull request?
pod_spec_from_resources()
How was this patch tested?
Ray Head node Resources
Ray controller Resources
Ray worker Resources
Check all the applicable boxes
Related PRs
Docs link