Decouple ray submitter, worker, and head resources #2924
Conversation
Sovietaced
force-pushed
the
issue-5666
branch
from
5e8306a
to
c621359
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2924 +/- ##
===========================================
- Coverage 94.25% 79.31% -14.94%
===========================================
Files 18 199 +181
Lines 1358 20853 +19495
Branches 0 2684 +2684
===========================================
+ Hits 1280 16540 +15260
- Misses 78 3564 +3486
- Partials 0 749 +749 ☔ View full report in Codecov by Sentry. |
Sovietaced
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
| self, | ||
| metadata: K8sObjectMetadata = None, | ||
| pod_spec: typing.Dict[str, typing.Any] = None, | ||
| data_config: typing.Optional[DataLoadingConfig] = None, |
There was a problem hiding this comment.
drop this. Also why add this K8s pod. We dont need that, as Ray config will simply float like a json. So just use K8s pod object?
There was a problem hiding this comment.
we can keep the k8s pod too, i see that you are actually setting the pod properties.
This way we could also use pod template?
There was a problem hiding this comment.
Yeah we had a discussion about this on the backend change here: flyteorg/flyte#5933 (comment)
It started as just adding support for resources but we realized it would be more flexible if we added support for something similar to a pod template since that is ultimately what the kuberay contract is.
| @@ -89,7 +92,9 @@ def get_custom(self, settings: SerializationSettings) -> Optional[Dict[str, Any] | |||
| ray_job = RayJob( | |||
| ray_cluster=RayCluster( | |||
| head_group_spec=( | |||
| HeadGroupSpec(cfg.head_node_config.ray_start_params) if cfg.head_node_config else None | |||
| HeadGroupSpec(cfg.head_node_config.ray_start_params, cfg.head_node_config.k8s_pod) | |||
There was a problem hiding this comment.
but i donot want to block. I see that we have backend ray plugin proto already.
There was a problem hiding this comment.
I appreciate it thank you.
Sovietaced
force-pushed
the
issue-5666
branch
from
2db498c
to
aefe56f
Compare
| @@ -1018,7 +1018,7 @@ def data_config(self) -> typing.Optional[DataLoadingConfig]: | |||
|
|
|||
| def to_flyte_idl(self) -> _core_task.K8sPod: | |||
| return _core_task.K8sPod( | |||
| metadata=self._metadata.to_flyte_idl(), | |||
| metadata=self._metadata.to_flyte_idl() if self.metadata else None, | |||
There was a problem hiding this comment.
Handles cases where metadata is None
Signed-off-by: Jason Parraga <[email protected]>
Sovietaced
force-pushed
the
issue-5666
branch
from
aefe56f
to
185ba39
Compare
|
Congrats on merging your first pull request! 🎉 |
Tracking issue
Related to flyteorg/flyte#5666
Why are the changes needed?
These changes update the flytekit-ray package such that users can configure pod specs for worker and head workloads.
What changes were proposed in this pull request?
Updating the version of flyteidl and plumb k8s pods through worker and head config.
How was this patch tested?
See unit tests.
Check all the applicable boxes
Related PRs
flyteorg/flyte#5933