Skip to content
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

Add/Enable Customizable Sample Pipeline to APIServer #88

Merged
merged 3 commits into from
Apr 13, 2023

Conversation

gmfrasca
Copy link
Member

Allows us to override the default pipeline, in this case with a more data-science applicable example

Description

Introduce the sample-config and sample-pipeline ConfigMaps, which are mounted as volumes on the APIServer pod. these contain overrides for sampel_config.json and a compiled Tekton pipeline showing a simple Iris training run example. This shouldn't be merged before the matching (DSP PR)[https://github.com/opendatahub-io/data-science-pipelines/pull/80] which provides the py source code to the pipeline (else there will be a dead link in the sample pipeline description

How Has This Been Tested?

Built a dev image of the DSPO, which can be used to override the image defined in the controller-manager Deployment in odh-applications. From there, create a new DSPA using the dspa_simple.yaml definition and verify flip-coin example has been replaced with an iris-training example.

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

@gmfrasca gmfrasca changed the title WIP: Add Customizable Sample Pipeline to APIServer Add Customizable Sample Pipeline to APIServer Apr 12, 2023
@gmfrasca gmfrasca changed the title Add Customizable Sample Pipeline to APIServer Add/Enasble Customizable Sample Pipeline to APIServer Apr 12, 2023
@gmfrasca gmfrasca changed the title Add/Enasble Customizable Sample Pipeline to APIServer Add/Enable Customizable Sample Pipeline to APIServer Apr 12, 2023
@gmfrasca gmfrasca force-pushed the iris-sample branch 3 times, most recently from 979e6da to 77fbdff Compare April 12, 2023 22:18
@HumairAK
Copy link
Contributor

✘ FAILURE after 0.016s: operator-tests/data-science-pipelines-operator/basictests/dsp-operator.sh:23: executing 'echo 3' expecting success and text '1': the output content test failed

@gmfrasca can you rebase? that' why the e2e failed.

- Create new bool .APIServer.EnableSamplePipeline. Default: true
- Conditionnally include custom SamplePipeline if bool is set to true
@HumairAK
Copy link
Contributor

HumairAK commented Apr 13, 2023

Tested, sample configs show up when enableSamplePipeline: true, and run fine.

Some additional notes:

  1. Setting enableSamplePipeline: false results in configmap clean up, but still remains in dspa because it exists in the db
  2. you can delete it manually from the db via the UI or curl call
  3. If you do (2) then you can't the samples back even if you enableSamplePipeline: true, you will see the following log line in server pod: I0413 18:15:42.157084 7 main.go:182] Samples already loaded in the past. Skip loading.

This effectively means you can only "load" the samples by setting enableSamplePipeline: true for the liftetime of a DSPA I think.

What I would expect to happen is, when I set enableSamplePipeline: false then pipelines would disappear, then setting it to true would make it re-appear. But I think this would make the scope of this task more complicated, maybe we can have follow on tasks for that?

/lgtm

@gmfrasca
Copy link
Member Author

/retest

@HumairAK : I think i agree that this is outside the scope of this work. Reasoning being that I believe following the equivalent process with the base flipcoin example remains the same (stand up a DSPA, delete sample pipeline, delete apiserver pod and sample will remain missing in new apiserver).

Additionally, getting too far in the weeds would mean having DSPO manually handling DB entries, which will lead to a very messy Operator code with a lot of breaking points, and not a great deal of value add in return.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anishasthana

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants