feat: Ray Cluster and Operator Deployment #638

MichaelClifford · 2022-08-10T19:12:13Z

Description

This PR is a follow up to #573. It Includes the manifests needed to deploy Ray on Openshift, but has removed any dependencies on JupyterHub from the previous PR.

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

It can be tested by following the instructions in the README included in the PR.

openshift-ci · 2022-08-10T19:12:24Z

Hi @MichaelClifford. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

LaVLaS · 2022-08-11T03:22:20Z

@MichaelClifford This is going to require support CI tests that will run within our test framework. See the test README

anishasthana

Under what SIG/WG are we proposing this component? Or is it going to be something like a tier 2 component for the time being?

MichaelClifford · 2022-08-26T13:26:48Z

@anishasthana based on the conversation at the ML Ops SIG meeting earlier this week, this will exist initially as a tier 2 component.

MichaelClifford · 2022-08-26T20:20:22Z

@LaVLaS added to the PR to add a couple of tests. Let me know if it needs anything else 😄

MichaelClifford · 2022-08-29T13:19:20Z

/retest

LaVLaS · 2022-08-30T00:03:09Z

@LaVLaS added to the PR to add a couple of tests. Let me know if it needs anything else smile

@MichaelClifford We are in the process of re-organizing components into tiers. I know there have been discussions about incubating Ray.io as a Tier 1 component which means it will need a full testsuite to verify:

It deploys successfully
Test of functionality to ensure that a successful deployment actually works.

You already have some tests to verify that it was deployed successfully, you'll need to add some tests to verify functionality so that any reviewer can determine if any future changes actually work.

MichaelClifford · 2022-09-09T19:26:12Z

@LaVLaS I've added an additional test that checks for functionality. It connects to the example ray cluster, runs a small job, then confirms the job ran on both the head and worker node.

Thanks for your guidance on this 😃 Let me know if we need more tests here.

anishasthana

Couple of nits + things to follow up on eventually. I'll try testing this PR by the end of the week

anishasthana · 2022-09-20T13:16:34Z

ray/cluster/base/kustomization.yaml

+kind: Kustomization
+apiVersion: kustomize.config.k8s.io/v1beta1
+resources:
+  - ray-cluster.yaml


Missing new line

anishasthana · 2022-09-20T13:17:16Z

ray/cluster/base/ray-cluster.yaml

+              # cause problems for other pods.
+              memory: 2G
+  - name: worker-nodes
+    # we can parameterize this when we fix the JH launcher json/jinja bug


Given ODH focus on NBC is this still valid?

anishasthana · 2022-09-20T13:17:47Z

tests/basictests/ray.sh

+check_cluster
+check_functionality
+
+os::test::junit::declare_suite_end


Missing new line

anishasthana · 2022-09-20T13:18:41Z

tests/resources/ray/ray-simple-test.yaml

+    spec:
+      containers:
+      - name: ray-odh-tests
+        image: quay.io/michaelclifford/ray-odh-tests:latest


Ideally we'd have this image in the ODH org. Where's the dockerfile for this image defined?

Dockerfile is currently here: https://github.com/redhat-et/ray-on-the-edge-demo/tree/main/tests

Should these files be included in this repo under something like ray/tests/?

What it the process to get images into the ODH org?

anishasthana · 2022-09-20T13:19:16Z

ray/operator/base/ray-operator-deployment.yaml

+      containers:
+        - name: ray-operator
+          imagePullPolicy: Always
+          image: 'quay.io/thoth-station/ray-operator:v0.1.3'


Ideally we'd have this image in the ODH org. Where's the dockerfile for this image defined?

anishasthana · 2022-09-20T13:19:25Z

ray/cluster/base/ray-cluster.yaml

+        containers:
+        - name: ray-node
+          imagePullPolicy: Always
+          image: quay.io/thoth-station/ray-ml-worker:v0.2.1


Ideally we'd have this image in the ODH org. Where's the dockerfile for this image defined?

anishasthana · 2022-09-20T13:19:32Z

ray/cluster/base/ray-cluster.yaml

+        containers:
+        - name: ray-node
+          imagePullPolicy: Always
+          image: quay.io/thoth-station/ray-ml-worker:v0.2.1


Ideally we'd have this image in the ODH org. Where's the dockerfile for this image defined?

anishasthana

Actually.... we need metrics and monitoring defined (if available).
I'd take a look at the ModelMesh component for a good example.

https://github.com/opendatahub-io/odh-manifests/tree/master/model-mesh/prometheus
https://github.com/opendatahub-io/odh-manifests/blob/master/tests/basictests/modelmesh.sh#L96

LaVLaS · 2022-09-22T13:03:40Z

@MichaelClifford Adding the comment based on the offline conversation with @opendatahub-io/sig-mlops that we will merge this here but include it in the migration to the opendatahub-io-contrib (Tier 2) org since the @opendatahub-io/sig-mlops does not have ray.io on the immediate roadmap for MLOps integration

anishasthana · 2022-10-05T18:05:21Z

ray/cluster/prometheus/monitor.yaml

+  labels:
+    k8s-app: ray-monitor
+  name: ray-monitor
+  namespace: open-data-hub


I don't think this namespace is valid or needed.

anishasthana · 2022-10-05T18:05:35Z

ray/cluster/prometheus/monitor.yaml

+      scheme: http
+  selector:
+    matchLabels:
+      app: ray-monitor


Missing newline :-)

HumairAK · 2022-10-05T18:06:11Z

ray/cluster/prometheus/monitor.yaml

+      scheme: http
+  selector:
+    matchLabels:
+      app: ray-monitor


remove newline

sorry -- it's the opposite, newline missing

HumairAK · 2022-10-05T18:06:19Z

ray/cluster/base/kustomization.yaml

+resources:
+  - ray-cluster.yaml
+  - ../prometheus
+


remove newline

sorry -- it's the opposite, newline missing

HumairAK · 2022-10-05T18:06:45Z

ray/OWNERS

+reviewers:
+- HumairAK
+- michaelclifford
+- rimolive


there's an additional space before rimolive

HumairAK · 2022-10-05T18:15:11Z

ray/README.md

+1. [Ray operator](./operator/ray-operator-deployment.yaml): The operator will process RayCluster resources and schedule ray head and worker pods based on requirements.
+2. [Ray CR](./operator/ray-custom-resources.yaml):  RayCluster Custom Resource (CR) describes the desired state of ray cluster.
+3. [Ray Cluster](./cluster/ray-cluster.yaml): Defines an instance of an example Ray Cluster 


I don't think you can create relative links to .yamls can you? it doesn't work when I click it in your branch, I think you'll have to link the resulting url path

I think you can. I just need to update the links, these are based on an older file structure.

HumairAK · 2022-10-05T18:19:50Z

ray/README.md

@@ -0,0 +1,70 @@
+# Deploying Ray with Open Data Hub
+
+_WIP Docs: _


is this supposed to be here? seems unnecessary

HumairAK · 2022-10-05T18:26:15Z

ray/README.md

+First use the `oc kustomize` command to generate a yaml containing all the requirements for the operator and the "raycluster" custom resource, then `oc apply` that yaml to deploy the operator to your cluster. 
+
+```bash
+$ oc kustomize deploy/odh-ray-nbc/operator > operator_deployment.yaml


deploy/odh-ray-nbc/operator
Is this an outdated path? the current path to the operator seems to be ray/operator/base ?

HumairAK · 2022-10-05T18:27:37Z

ray/README.md

+$ oc kustomize deploy/odh-ray-nbc/operator > operator_deployment.yaml
+```
+```bash
+$ oc apply -f operator_deployment.yaml


as this is a namespace scoped deployment I'd specify -n <your-namespace>

I'd reflect the -n <your-namespace> in the following commands as well

HumairAK · 2022-10-05T18:29:09Z

ray/README.md

+
+
+```bash
+$ oc kustomize deploy/odh-ray-nbc/cluster > cluster_deployment.yaml


same as before, the path deploy/odh-ray-nbc/cluster seems like a typo, looks like the cluster is at ray/cluster/base

openshift-ci · 2022-10-13T05:09:33Z

The following users are mentioned in OWNERS file(s) but are untrusted for the following reasons. One way to make the user trusted is to add them as members of the opendatahub-io org. You can then trigger verification by writing /verify-owners in a comment.

HumairAK
- User is not a member of the org. User is not a collaborator. Satisfy at least one of these conditions to make the user trusted.
- ray/OWNERS
michaelclifford
- User is not a member of the org. User is not a collaborator. Satisfy at least one of these conditions to make the user trusted.
- ray/OWNERS

LaVLaS · 2022-10-14T03:57:17Z

/retest

/approve

/lgtm

openshift-ci · 2022-10-14T12:19:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LaVLaS

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [LaVLaS]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from LaVLaS and VaishnaviHire August 10, 2022 19:12

openshift-ci bot added the needs-ok-to-test label Aug 10, 2022

anishasthana reviewed Aug 12, 2022

View reviewed changes

crobby added the ok-to-test label Aug 22, 2022

MichaelClifford force-pushed the ray branch from 6b5e3cc to 9a20c9c Compare August 26, 2022 20:09

openshift-merge-robot added the needs-rebase label Aug 26, 2022

openshift-ci bot removed the needs-ok-to-test label Aug 26, 2022

MichaelClifford force-pushed the ray branch from 9a20c9c to 1646b00 Compare August 26, 2022 20:17

openshift-merge-robot removed the needs-rebase label Aug 26, 2022

MichaelClifford force-pushed the ray branch from 1646b00 to fdd1a60 Compare August 26, 2022 20:27

MichaelClifford force-pushed the ray branch 2 times, most recently from 8fdb338 to 6286da0 Compare August 29, 2022 19:13

MichaelClifford force-pushed the ray branch 2 times, most recently from 58f79ef to f455888 Compare August 30, 2022 15:02

MichaelClifford force-pushed the ray branch 3 times, most recently from 5b689d2 to c53a6fc Compare September 9, 2022 17:53

anishasthana reviewed Sep 20, 2022

View reviewed changes

anishasthana suggested changes Sep 20, 2022

View reviewed changes

openshift-ci bot assigned anishasthana Sep 20, 2022

MichaelClifford force-pushed the ray branch from c53a6fc to 47c89e1 Compare September 21, 2022 19:47

openshift-ci bot added the do-not-merge/invalid-owners-file label Sep 21, 2022

MichaelClifford force-pushed the ray branch from 47c89e1 to 5698b17 Compare September 27, 2022 14:45

anishasthana suggested changes Oct 5, 2022

View reviewed changes

HumairAK reviewed Oct 5, 2022

View reviewed changes

ray/OWNERS

reviewers:

- HumairAK

- michaelclifford

- rimolive

Copy link

Contributor

HumairAK Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's an additional space before rimolive

HumairAK reviewed Oct 5, 2022

View reviewed changes

feat: Ray Cluster and Operator Deployment

c06a581

MichaelClifford force-pushed the ray branch from 5698b17 to c06a581 Compare October 6, 2022 15:01

openshift-merge-robot added the needs-rebase label Oct 10, 2022

Merge branch 'master' into ray

445350b

openshift-merge-robot removed the needs-rebase label Oct 13, 2022

openshift-ci bot assigned LaVLaS Oct 14, 2022

openshift-ci bot added lgtm approved labels Oct 14, 2022

LaVLaS removed the do-not-merge/invalid-owners-file label Oct 14, 2022

LaVLaS approved these changes Oct 14, 2022

View reviewed changes

openshift-merge-robot merged commit 438b933 into opendatahub-io:master Oct 14, 2022

		@@ -0,0 +1,70 @@
		# Deploying Ray with Open Data Hub

		_WIP Docs: _



		```bash
		$ oc kustomize deploy/odh-ray-nbc/cluster > cluster_deployment.yaml

feat: Ray Cluster and Operator Deployment #638

feat: Ray Cluster and Operator Deployment #638

Conversation

MichaelClifford commented Aug 10, 2022

Description

Merge criteria:

openshift-ci bot commented Aug 10, 2022

LaVLaS commented Aug 11, 2022

anishasthana left a comment

Choose a reason for hiding this comment

MichaelClifford commented Aug 26, 2022

MichaelClifford commented Aug 26, 2022

MichaelClifford commented Aug 29, 2022

LaVLaS commented Aug 30, 2022

MichaelClifford commented Sep 9, 2022

anishasthana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anishasthana left a comment

Choose a reason for hiding this comment

LaVLaS commented Sep 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HumairAK Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 13, 2022

LaVLaS commented Oct 14, 2022

openshift-ci bot commented Oct 14, 2022

HumairAK Oct 5, 2022 •

edited

Loading