Add ilab kfp pipeline to the DSPO repo #764

VaniHaripriya · 2024-12-13T17:13:49Z

The issue resolved by this Pull Request:

Description of your changes:

Added support for provisioning the iLab pipeline as part of a DSPA deployment.
Introduced a new parameter, enableIlabKFPPipeline(set to false by default), which can be set to true to enable the iLab KFP pipeline in the UI.

Testing instructions

Deploy DSPO.
Deploy DSPA using the provided yaml file:
Scenario 1:
Provision both the Iris and InstructLab pipelines, create pipeline runs, and ensure they complete successfully.

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1
kind: DataSciencePipelinesApplication
metadata:
  name: sample
spec:
  dspVersion: v2
  apiServer:
    enableSamplePipeline: true
    enableInstructlabPipeline: true
    image: quay.io/opendatahub/ds-pipelines-api-server:pr-120
    cABundle:
      configMapKey: ca.crt
      configMapName: kube-root-ca.crt
  podToPodTLS: false
  objectStorage:
    # Need to enable this for artifact download links to work
    # i.e. for when requesting /apis/v2beta1/artifacts/{id}?share_url=true
    enableExternalRoute: true
    minio:
      deploy: true
      image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'
  mlpipelineUI:
    image: quay.io/opendatahub/ds-pipelines-frontend:latest

Scenario 2:
Provision the Iris pipeline, create pipeline run, and ensure it completes successfully.

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1
kind: DataSciencePipelinesApplication
metadata:
  name: sample
spec:
  dspVersion: v2
  apiServer:
    enableSamplePipeline: true    
    image: quay.io/opendatahub/ds-pipelines-api-server:pr-120
    cABundle:
      configMapKey: ca.crt
      configMapName: kube-root-ca.crt
  podToPodTLS: false
  objectStorage:
    # Need to enable this for artifact download links to work
    # i.e. for when requesting /apis/v2beta1/artifacts/{id}?share_url=true
    enableExternalRoute: true
    minio:
      deploy: true
      image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'
  mlpipelineUI:
    image: quay.io/opendatahub/ds-pipelines-frontend:latest

Scenario 3:
Provision the Instructlab pipeline, create pipeline run, and ensure it completes successfully.

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1
kind: DataSciencePipelinesApplication
metadata:
  name: sample
spec:
  dspVersion: v2
  apiServer:
    enableInstructlabPipeline: true    
    image: quay.io/opendatahub/ds-pipelines-api-server:pr-120
    cABundle:
      configMapKey: ca.crt
      configMapName: kube-root-ca.crt
  podToPodTLS: false
  objectStorage:
    # Need to enable this for artifact download links to work
    # i.e. for when requesting /apis/v2beta1/artifacts/{id}?share_url=true
    enableExternalRoute: true
    minio:
      deploy: true
      image: 'quay.io/opendatahub/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance'
  mlpipelineUI:
    image: quay.io/opendatahub/ds-pipelines-frontend:latest

Checklist

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

openshift-ci · 2024-12-13T17:13:53Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dsp-developers · 2024-12-13T17:16:01Z

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764
An OCP cluster where you are logged in as cluster admin is required.

To use this image run the following:

cd $(mktemp -d)
git clone [email protected]:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/764/head
git checkout -b pullrequest f41faec8325da596cf8fe63d1f17333dc0e170de
oc new-project opendatahub
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-764"

More instructions here on how to deploy and test a Data Science Pipelines Application.

dsp-developers · 2024-12-13T17:20:19Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

dsp-developers · 2024-12-13T17:41:33Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

dsp-developers · 2024-12-13T17:47:49Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

dsp-developers · 2024-12-13T17:55:09Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

gmfrasca · 2024-12-13T19:31:22Z

api/v1/dspipeline_types.go

+	// Include ilab KFP pipeline with the deployment of this DSP API Server. Default: false
+	// +kubebuilder:default:=false
+	// +kubebuilder:validation:Optional
+	EnableIlabKFPPipeline bool   `json:"enableIlabKFPPipeline"`


let's rename this (and all similar variables) to EnableInstructlabPipeline. Its a has a little more clarity, and we should avoid using 'KFP' in DSP-specific functions like this one

gmfrasca · 2024-12-13T19:34:24Z

.../bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml

@@ -105,6 +105,11 @@ spec:
                      Server. Setting Deploy to false disables operator reconciliation.
                      Default: true'
                    type: boolean
+                  enableIlabKFPPipeline:
+                    default: false
+                    description: 'Include ilab KFP pipeline with the deployment of


similar to above, lets remove 'KFP' and expand ilab to Instructlab. Perhaps include a little more context, like

Include instructlab multi-phase training and LLM generation pipeline with the deployment of this DSP API Server. Default: false

gmfrasca · 2024-12-13T19:39:07Z

config/internal/apiserver/sample-pipeline/sample-config.yaml.tmpl

+      {{- end }}
+      {{- if .EnableIlabKFPPipeline }}
+      {
+        "name": "ilab-kfp-pipeline",


This is a human-readable name so lets give it a friendlier title, like [Instructlab] Multi-Phase Training Pipeline

gmfrasca · 2024-12-13T19:41:09Z

config/internal/apiserver/sample-pipeline/sample-config.yaml.tmpl

+      {
+        "name": "ilab-kfp-pipeline",
+        "description": "[source code](https://github.com/opendatahub-io/ilab-on-ocp) Ilab KFP pipeline",
+        "file": "/pipelines/pipeline.yaml"


we should name the pipeline something a little more clear than 'pipeline.yaml', ie instructlab.yaml

dsp-developers · 2024-12-13T21:23:17Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

dsp-developers · 2024-12-13T21:43:28Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

mprahl · 2024-12-17T19:50:02Z

@VaniHaripriya this looks good to me (though I'm new 😅 )!

Could you please add an automated test?

It looks like there's already test utility functions to see if a pipeline exists. For example:

		pipelineID, err := TestUtil.RetrievePipelineId(t, suite.Clientmgr.httpClient, APIServerURL, pipelineDisplayName)
		require.NoError(t, err)

VaniHaripriya · 2024-12-17T20:50:37Z

@VaniHaripriya this looks good to me (though I'm new 😅 )!

Could you please add an automated test?

It looks like there's already test utility functions to see if a pipeline exists. For example:
		pipelineID, err := TestUtil.RetrievePipelineId(t, suite.Clientmgr.httpClient, APIServerURL, pipelineDisplayName)
		require.NoError(t, err)

@mprahl Thank you for reviewing, I am adding the tests as part of this RHOAIENG-16508

hbelmiro · 2024-12-23T12:13:52Z

/lgtm

rimolive · 2024-12-26T17:39:16Z

/lgtm

HumairAK · 2025-01-06T17:20:01Z

@VaniHaripriya this looks good to me (though I'm new 😅 )!
Could you please add an automated test?

It looks like there's already test utility functions to see if a pipeline exists. For example:

Agree with this, @VaniHaripriya for:

I am adding the tests as part of this RHOAIENG-16508

This is for e2e tests that will go here

For this pr a simple test that verifies pipeline exists when dspa flag is enabled would be good to have. Since we are already deploying dspa twice (one with external data connections and one without) we can have just one configured to enable the provisioning, and test both enable/disable

dsp-developers · 2025-01-07T17:22:46Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

dsp-developers · 2025-01-07T17:32:11Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

dsp-developers · 2025-01-07T17:41:53Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

mprahl · 2025-01-07T17:46:22Z

api/v1/dspipeline_types.go

-	ArgoLauncherImage    string `json:"argoLauncherImage,omitempty"`
-	ArgoDriverImage      string `json:"argoDriverImage,omitempty"`
+	EnableSamplePipeline bool `json:"enableSamplePipeline"`
+	// Include instructlab multi-phase training and LLM generation pipeline with the deployment of this DSP API Server. Default: false


Since this is user facing documentation in the CRD, we should fix the casing. Sorry for missing this before.

Suggested change

// Include instructlab multi-phase training and LLM generation pipeline with the deployment of this DSP API Server. Default: false

// Enable the Instructlab Multi-Phase Training pipeline with the deployment of this DSP API server. Default is false.

dsp-developers · 2025-01-07T18:14:29Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

dsp-developers · 2025-01-07T18:49:14Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

mprahl · 2025-01-08T14:53:30Z

api/v1/dspipeline_types.go

-	ArgoLauncherImage    string `json:"argoLauncherImage,omitempty"`
-	ArgoDriverImage      string `json:"argoDriverImage,omitempty"`
+	EnableSamplePipeline bool `json:"enableSamplePipeline"`
+	// Enable the Instructlab Multi-Phase Training pipeline with the deployment of this DSP API server. Default is false


Sorry, I didn't realize the booleans above had it formatted this way.

Suggested change

// Enable the Instructlab Multi-Phase Training pipeline with the deployment of this DSP API server. Default is false

// Enable the Instructlab Multi-Phase Training pipeline with the deployment of this DSP API server. Default: false

mprahl · 2025-01-08T14:53:52Z

api/v1alpha1/dspipeline_types.go

-	ArgoLauncherImage    string `json:"argoLauncherImage,omitempty"`
-	ArgoDriverImage      string `json:"argoDriverImage,omitempty"`
+	EnableSamplePipeline bool `json:"enableSamplePipeline"`
+	// Enable the Instructlab Multi-Phase Training pipeline with the deployment of this DSP API server. Default is false


Suggested change

// Enable the Instructlab Multi-Phase Training pipeline with the deployment of this DSP API server. Default is false

// Enable the Instructlab Multi-Phase Training pipeline with the deployment of this DSP API server. Default: false

mprahl · 2025-01-08T14:54:45Z

config/internal/apiserver/default/deployment.yaml.tmpl

@@ -206,8 +206,8 @@ spec:
            - mountPath: /etc/tls/private
              name: proxy-tls
          {{ end }}
-          {{ if or .APIServer.EnableSamplePipeline .CustomCABundle }}
-            {{ if .APIServer.EnableSamplePipeline }}
+          {{ if or .APIServer.EnableSamplePipeline .CustomCABundle .APIServer.EnableInstructLabPipeline}}


I think this if statement can be removed.

mprahl

This looks good! Just a couple minor comments and then I'll approve it.

dsp-developers · 2025-01-08T16:07:20Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

dsp-developers · 2025-01-08T16:09:59Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

config/internal/apiserver/default/deployment.yaml.tmpl

dsp-developers · 2025-01-08T16:39:06Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

mprahl · 2025-01-08T16:48:52Z

@VaniHaripriya could you please squash the commits? After that, I'll go ahead and approve it!

Signed-off-by: VaniHaripriya <[email protected]> Co-authored-by: Matt Prahl <[email protected]>

VaniHaripriya · 2025-01-08T17:12:31Z

@VaniHaripriya could you please squash the commits? After that, I'll go ahead and approve it!

Thank you Matt! @mprahl

dsp-developers · 2025-01-08T17:14:02Z

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-764

openshift-ci · 2025-01-08T17:44:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mprahl

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~~ [mprahl]

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 added the do-not-merge/work-in-progress label Dec 13, 2024

gmfrasca reviewed Dec 13, 2024

View reviewed changes

VaniHaripriya force-pushed the RHOAIENG-16507 branch from 93e0d57 to 6977289 Compare December 13, 2024 21:14

VaniHaripriya marked this pull request as ready for review December 13, 2024 21:33

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 13, 2024

openshift-ci bot requested review from DharmitD and gmfrasca December 13, 2024 21:33

VaniHaripriya force-pushed the RHOAIENG-16507 branch 2 times, most recently from 042a446 to 6771c34 Compare December 13, 2024 21:41

mprahl approved these changes Dec 17, 2024

View reviewed changes

openshift-ci bot assigned mprahl Dec 17, 2024

openshift-ci bot added the lgtm label Dec 17, 2024

openshift-ci bot assigned hbelmiro Dec 23, 2024

openshift-ci bot assigned rimolive Dec 26, 2024

openshift-ci bot added the approved label Jan 7, 2025

mprahl reviewed Jan 7, 2025

View reviewed changes

VaniHaripriya force-pushed the RHOAIENG-16507 branch from 2e7642e to fac0ff7 Compare January 7, 2025 18:11

VaniHaripriya force-pushed the RHOAIENG-16507 branch from d1edd08 to 7e06c07 Compare January 7, 2025 18:46

mprahl self-requested a review January 8, 2025 14:52

mprahl reviewed Jan 8, 2025

View reviewed changes

VaniHaripriya force-pushed the RHOAIENG-16507 branch from e7ec310 to 5d0682e Compare January 8, 2025 16:07

mprahl reviewed Jan 8, 2025

View reviewed changes

config/internal/apiserver/default/deployment.yaml.tmpl Outdated Show resolved Hide resolved

mprahl self-requested a review January 8, 2025 16:48

Add ilab kfp pipeline to the DSPO repo

d439d03

Signed-off-by: VaniHaripriya <[email protected]> Co-authored-by: Matt Prahl <[email protected]>

VaniHaripriya force-pushed the RHOAIENG-16507 branch from 9ddf65d to d439d03 Compare January 8, 2025 17:11

mprahl approved these changes Jan 8, 2025

View reviewed changes

openshift-ci bot added the lgtm label Jan 8, 2025

openshift-merge-bot bot merged commit 22529bb into opendatahub-io:main Jan 8, 2025
7 checks passed

gmfrasca mentioned this pull request Jan 10, 2025

Revert "Add ilab kfp pipeline to the DSPO repo" #767

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ilab kfp pipeline to the DSPO repo #764

Add ilab kfp pipeline to the DSPO repo #764

VaniHaripriya commented Dec 13, 2024 •

edited

Loading

openshift-ci bot commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

gmfrasca Dec 13, 2024

gmfrasca Dec 13, 2024 •

edited

Loading

gmfrasca Dec 13, 2024

gmfrasca Dec 13, 2024

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

mprahl commented Dec 17, 2024

VaniHaripriya commented Dec 17, 2024

hbelmiro commented Dec 23, 2024

rimolive commented Dec 26, 2024

HumairAK commented Jan 6, 2025

dsp-developers commented Jan 7, 2025

dsp-developers commented Jan 7, 2025

dsp-developers commented Jan 7, 2025

mprahl Jan 7, 2025

dsp-developers commented Jan 7, 2025

dsp-developers commented Jan 7, 2025

mprahl Jan 8, 2025

mprahl Jan 8, 2025

mprahl Jan 8, 2025

mprahl left a comment

dsp-developers commented Jan 8, 2025

dsp-developers commented Jan 8, 2025

dsp-developers commented Jan 8, 2025

mprahl commented Jan 8, 2025

VaniHaripriya commented Jan 8, 2025

dsp-developers commented Jan 8, 2025

openshift-ci bot commented Jan 8, 2025

	// Include instructlab multi-phase training and LLM generation pipeline with the deployment of this DSP API Server. Default: false
	// Enable the Instructlab Multi-Phase Training pipeline with the deployment of this DSP API server. Default is false.

Add ilab kfp pipeline to the DSPO repo #764

Add ilab kfp pipeline to the DSPO repo #764

Conversation

VaniHaripriya commented Dec 13, 2024 • edited Loading

The issue resolved by this Pull Request:

Description of your changes:

Testing instructions

Checklist

openshift-ci bot commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

gmfrasca Dec 13, 2024

Choose a reason for hiding this comment

gmfrasca Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

gmfrasca Dec 13, 2024

Choose a reason for hiding this comment

gmfrasca Dec 13, 2024

Choose a reason for hiding this comment

dsp-developers commented Dec 13, 2024

dsp-developers commented Dec 13, 2024

mprahl commented Dec 17, 2024

VaniHaripriya commented Dec 17, 2024

hbelmiro commented Dec 23, 2024

rimolive commented Dec 26, 2024

HumairAK commented Jan 6, 2025

dsp-developers commented Jan 7, 2025

dsp-developers commented Jan 7, 2025

dsp-developers commented Jan 7, 2025

mprahl Jan 7, 2025

Choose a reason for hiding this comment

dsp-developers commented Jan 7, 2025

dsp-developers commented Jan 7, 2025

mprahl Jan 8, 2025

Choose a reason for hiding this comment

mprahl Jan 8, 2025

Choose a reason for hiding this comment

mprahl Jan 8, 2025

Choose a reason for hiding this comment

mprahl left a comment

Choose a reason for hiding this comment

dsp-developers commented Jan 8, 2025

dsp-developers commented Jan 8, 2025

dsp-developers commented Jan 8, 2025

mprahl commented Jan 8, 2025

VaniHaripriya commented Jan 8, 2025

dsp-developers commented Jan 8, 2025

openshift-ci bot commented Jan 8, 2025

VaniHaripriya commented Dec 13, 2024 •

edited

Loading

gmfrasca Dec 13, 2024 •

edited

Loading