RHOAIENG-4528 - Customizable kfp-launcher with a config map #630

amadhusu · 2024-04-19T22:20:18Z

The issue resolved by this Pull Request:

Description of your changes:

kfp-launcher is a KFP component that is responsible for fulfilling the "Executor wrapper" and "Publisher" roles as described in the KFP v2 System Design document:

Executor: user container that runs user specified image with command and arguments.
Publisher: publish MLMD metadata read from the executor to MLMD. The publisher is built in a statically compiled go binary and injected as an entrypoint of the executor Pod.

kfp-launcher requires a configmap to exist in the namespace where it runs. This configmap contains pipeline root and object storage configuration. This configmap must be named "kfp-launcher".

We currently deploy a default copy of the kfp-launcher configmap via DSPO, but we want the user to be able to provide their own configmap configuration as well, so that they can specify multiple object storage sources and paths (as described in https://issues.redhat.com/browse/RHOAIENG-4528). This PR adds this functionality.

We don't want to assume anything about the structure of the kfp-launcher configmap (and we know that it will likely change in the future), so this implementation simply copies the data contents of the user-provided configmap into the kfp-launcher configmap.

Testing instructions

Deploy DSPO
Apply the config map mentioned below to the namespace where you will deploy the DSPA.
Deploy the DSPA mentioned below
After all the DSPA Pods are running, verify the kfp-launcher ConfigMap is the same as the config map we applied earlier.
Deploy the config/samples/v2/dspa-simple/ DSPA in another namespace
After all the DSPA Pods are running ,verify the kfp-launcher ConfigMap is created with autogenerated values as was always the case.
Run a Pipeline to verify that the simple-dspa runs as it should

Attachments:

config_map.yaml

kind: ConfigMap
apiVersion: v1
metadata:
  name: custom-config
  labels:
    app: ds-pipeline-test
    component: data-science-pipelines
data:
    s3: |
      defaultPipelineRoot: s3://rhods-dsp-dev endpoint: https://s3.amazonaws.com/ region: us-east-2 secretName: aws-artifact-secret accessKeyKey: accesskey secretKeyKey: secretkey

dspa.yaml

apiVersion: datasciencepipelinesapplications.opendatahub.io/v1alpha1
kind: DataSciencePipelinesApplication
metadata:
  name: sample
spec:
  apiServer:
    customKfpLauncherConfigMap: custom-config
  dspVersion: v2
  objectStorage:
    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

dsp-developers · 2024-04-19T22:22:29Z

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-630
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 git@github.com:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/630/head
git checkout -b pullrequest 3c04640cefd84fc0e47a6a024783670e2b59881f
oc new-project opendatahub
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-630"

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

dsp-developers · 2024-04-23T11:41:43Z

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-630

api/v1alpha1/dspipeline_types.go

dsp-developers · 2024-04-25T08:00:13Z

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-630

controllers/dspipeline_params.go

dsp-developers · 2024-04-26T12:32:27Z

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-630

hbelmiro

/lgtm

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

HumairAK · 2024-04-30T14:21:29Z

/hold

dsp-developers · 2024-05-08T11:57:57Z

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-630

dsp-developers · 2024-07-18T13:52:39Z

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-630

controllers/dspipeline_params.go

dsp-developers · 2024-07-24T11:32: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-630

dsp-developers · 2024-07-24T11:41:44Z

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-630

dsp-developers · 2024-07-24T11:44:10Z

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-630

dsp-developers · 2024-07-24T11:52: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-630

dsp-developers · 2024-07-24T13:37: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-630

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

dsp-developers · 2024-07-24T14:12: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-630

VaniHaripriya · 2024-07-26T17:48:26Z

Tested as per instructions, both the scenarios are working as expected. But the Persistence Agent pod keeps crashing, also not able to create pipeline runs. Attaching the log file.
ds-pipeline-persistenceagent-sample-846cd74f8f-h7kw2-ds-pipeline-persistenceagent.log

gregsheremeta

overall approach is correct!

gregsheremeta · 2024-07-26T19:32:29Z

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

@@ -1,5 +1,9 @@
 apiVersion: v1
 data:
+  {{ if .APIServer.CustomKfpLauncherConfig }}
+  providers: |


(Context: "pipeline root" is a terrible name, and what it actually means is "the base folder in my object storage where all my stuff will go.")

Hm, looks like the user can only set the fields under providers -- i.e., they can't change defaultPipelineRoot. That seems incorrect to me.

If I'm allowed to override this ConfigMap with my own for the purposes of making the object storage connection exactly what I want, I should absolutely be able to have my pipeline root (base object storage path) set right along side in that same override ConfigMap.

Given the (original -- I suggested a change) desription of the field above, as a user, I would be caught off guard if only a subset of the ConfigMap was replaced. I'm expecting to be able to control everything under data.

gregsheremeta · 2024-07-26T19:46:17Z

api/v1alpha1/dspipeline_types.go

+	// CustomKfpLauncherConfig is a custom config file that you can provide
+	// for the api server to use instead of the one provided with DSPO.


Suggested change

// CustomKfpLauncherConfig is a custom config file that you can provide

// for the api server to use instead of the one provided with DSPO.

// Allows the user to fully replace the contents of the kfp-launcher ConfigMap.

// kfp-launcher requires a ConfigMap to exist in the namespace where it runs.

// This ConfigMap contains pipeline root and object storage configuration.

// This ConfigMap must be named "kfp-launcher". We currently deploy a default copy

// of the kfp-launcher ConfigMap via DSPO, but a user may want to provide their own

// ConfigMap configuration, so that they can specify multiple object storage sources

// and paths. The "data" contents of the "kfp-launcher" ConfigMap will be fully replaced

// with the "data" contents of the ConfigMap specified here.

sorry about the spaces. use tabs :)

gregsheremeta · 2024-07-26T19:53:47Z

controllers/dspipeline_params.go

+				// If the custom kfp-launcher configmap is not available, that is OK
+				if !apierrs.IsNotFound(err) {
+					log.Error(err, fmt.Sprintf("Encountered error when attempting to fetch ConfigMap: [%s], Error: %v", cfg, err))
+					return err


if it's ok, I think we try to proceed with the defaults as a fallback, just like if the user didn't specify any ConfigMap file at all.

So, don't return an error here -- log it and then keep going. If you return, all the rest of the param setting below doesn't happen.

I'd log something like "User specified a CustomKfpLauncherConfig, but it's not found. Falling back to using defaults."

Yeah my suggestion was to throw the error, but I'm fine with the fallback approach as long as we are providing sufficient logs for users to be able to deduce why their configmap was not picked up.

gotcha. Yeah I like falling back here :) Thanks!

gregsheremeta · 2024-07-26T20:12:53Z

Also, this requires a test to ensure the generated kfp-launcher configmap contains the contents we expect when we override the default one

gregsheremeta · 2024-08-05T20:12:42Z

moving to #681

/close

gregsheremeta · 2024-08-05T20:13:12Z

/close

openshift-ci · 2024-08-05T20:13:15Z

@gregsheremeta: Closed this PR.

In response to this:

/close

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-sigs/prow repository.

gregsheremeta · 2024-08-05T20:13:24Z

/close

openshift-ci · 2024-08-05T20:13:28Z

@gregsheremeta: Closed this PR.

In response to this:

/close

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-sigs/prow repository.

openshift-ci bot added the do-not-merge/work-in-progress label Apr 19, 2024

openshift-ci bot requested review from gmfrasca and HumairAK April 19, 2024 22:20

amadhusu force-pushed the RHOAIENG-4528 branch from 3c04640 to 60db2af Compare April 23, 2024 11:39

amadhusu changed the title ~~WIP: RHOAIENG-4528 - Customizable kfp-launcher with a config map~~ RHOAIENG-4528 - Customizable kfp-launcher with a config map Apr 23, 2024

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

gmfrasca reviewed Apr 24, 2024

View reviewed changes

api/v1alpha1/dspipeline_types.go Outdated Show resolved Hide resolved

amadhusu force-pushed the RHOAIENG-4528 branch from 60db2af to 25c3e40 Compare April 25, 2024 07:57

amadhusu requested a review from gmfrasca April 25, 2024 14:17

hbelmiro reviewed Apr 25, 2024

View reviewed changes

controllers/dspipeline_params.go Outdated Show resolved Hide resolved

amadhusu force-pushed the RHOAIENG-4528 branch from 25c3e40 to 44ac6fc Compare April 26, 2024 12:30

amadhusu requested review from hbelmiro and DharmitD April 26, 2024 12:32

hbelmiro reviewed Apr 26, 2024

View reviewed changes

openshift-ci bot assigned hbelmiro Apr 26, 2024

openshift-ci bot added the lgtm label Apr 26, 2024

HumairAK reviewed Apr 30, 2024

View reviewed changes

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

openshift-ci bot added the do-not-merge/hold label Apr 30, 2024

amadhusu force-pushed the RHOAIENG-4528 branch from 44ac6fc to f1d9919 Compare May 8, 2024 11:55

openshift-ci bot removed the lgtm label May 8, 2024

openshift-merge-robot added the needs-rebase label May 11, 2024

amadhusu force-pushed the RHOAIENG-4528 branch from f1d9919 to b33521c Compare May 28, 2024 12:59

openshift-merge-robot removed the needs-rebase label May 28, 2024

amadhusu force-pushed the RHOAIENG-4528 branch from b33521c to 6b5e154 Compare May 28, 2024 13:03

HumairAK reviewed Jul 18, 2024

View reviewed changes

controllers/dspipeline_params.go Show resolved Hide resolved

HumairAK reviewed Jul 18, 2024

View reviewed changes

controllers/dspipeline_params.go Outdated Show resolved Hide resolved

amadhusu force-pushed the RHOAIENG-4528 branch from a78bb3d to 5fc7f78 Compare July 24, 2024 11:29

amadhusu force-pushed the RHOAIENG-4528 branch from 5fc7f78 to 4820cb0 Compare July 24, 2024 11:39

amadhusu force-pushed the RHOAIENG-4528 branch from 4820cb0 to d0a72b6 Compare July 24, 2024 11:42

amadhusu requested review from HumairAK, gregsheremeta and hbelmiro July 24, 2024 11:44

amadhusu force-pushed the RHOAIENG-4528 branch from d0a72b6 to 48533a7 Compare July 24, 2024 11:50

amadhusu force-pushed the RHOAIENG-4528 branch from 48533a7 to e050c7a Compare July 24, 2024 13:34

Customizable kfp-launcher with a config map

6d9dc49

Signed-off-by: Achyut Madhusudan <amadhusu@redhat.com>

amadhusu force-pushed the RHOAIENG-4528 branch from e050c7a to 6d9dc49 Compare July 24, 2024 14:10

gregsheremeta requested changes Jul 26, 2024

View reviewed changes

openshift-ci bot closed this Aug 5, 2024

gregsheremeta reopened this Aug 5, 2024

openshift-ci bot closed this Aug 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHOAIENG-4528 - Customizable kfp-launcher with a config map #630

RHOAIENG-4528 - Customizable kfp-launcher with a config map #630

amadhusu commented Apr 19, 2024 •

edited

Loading

dsp-developers commented Apr 19, 2024

dsp-developers commented Apr 23, 2024

dsp-developers commented Apr 25, 2024

dsp-developers commented Apr 26, 2024

hbelmiro left a comment

HumairAK commented Apr 30, 2024

dsp-developers commented May 8, 2024

dsp-developers commented Jul 18, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

VaniHaripriya commented Jul 26, 2024

gregsheremeta left a comment

gregsheremeta Jul 26, 2024

gregsheremeta Jul 26, 2024

gregsheremeta Jul 26, 2024

gregsheremeta Jul 26, 2024

HumairAK Jul 26, 2024

gregsheremeta Jul 26, 2024

gregsheremeta commented Jul 26, 2024 •

edited

Loading

gregsheremeta commented Aug 5, 2024

gregsheremeta commented Aug 5, 2024

openshift-ci bot commented Aug 5, 2024

gregsheremeta commented Aug 5, 2024

openshift-ci bot commented Aug 5, 2024

		// CustomKfpLauncherConfig is a custom config file that you can provide
		// for the api server to use instead of the one provided with DSPO.

-	// CustomKfpLauncherConfig is a custom config file that you can provide
-	// for the api server to use instead of the one provided with DSPO.
+    // Allows the user to fully replace the contents of the kfp-launcher ConfigMap.
+    // kfp-launcher requires a ConfigMap to exist in the namespace where it runs.
+    // This ConfigMap contains pipeline root and object storage configuration.
+    // This ConfigMap must be named "kfp-launcher". We currently deploy a default copy
+    // of the kfp-launcher ConfigMap via DSPO, but a user may want to provide their own
+    // ConfigMap configuration, so that they can specify multiple object storage sources
+    // and paths. The "data" contents of the "kfp-launcher" ConfigMap will be fully replaced
+    // with the "data" contents of the ConfigMap specified here.

RHOAIENG-4528 - Customizable kfp-launcher with a config map #630

RHOAIENG-4528 - Customizable kfp-launcher with a config map #630

Conversation

amadhusu commented Apr 19, 2024 • edited Loading

The issue resolved by this Pull Request:

Description of your changes:

Testing instructions

Checklist

dsp-developers commented Apr 19, 2024

dsp-developers commented Apr 23, 2024

dsp-developers commented Apr 25, 2024

dsp-developers commented Apr 26, 2024

hbelmiro left a comment

Choose a reason for hiding this comment

HumairAK commented Apr 30, 2024

dsp-developers commented May 8, 2024

dsp-developers commented Jul 18, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

dsp-developers commented Jul 24, 2024

VaniHaripriya commented Jul 26, 2024

gregsheremeta left a comment

Choose a reason for hiding this comment

gregsheremeta Jul 26, 2024

Choose a reason for hiding this comment

gregsheremeta Jul 26, 2024

Choose a reason for hiding this comment

gregsheremeta Jul 26, 2024

Choose a reason for hiding this comment

gregsheremeta Jul 26, 2024

Choose a reason for hiding this comment

HumairAK Jul 26, 2024

Choose a reason for hiding this comment

gregsheremeta Jul 26, 2024

Choose a reason for hiding this comment

gregsheremeta commented Jul 26, 2024 • edited Loading

gregsheremeta commented Aug 5, 2024

gregsheremeta commented Aug 5, 2024

openshift-ci bot commented Aug 5, 2024

gregsheremeta commented Aug 5, 2024

openshift-ci bot commented Aug 5, 2024

amadhusu commented Apr 19, 2024 •

edited

Loading

gregsheremeta commented Jul 26, 2024 •

edited

Loading