Local model upload to PVC #115

jackdelahunt · 2023-10-03T12:26:27Z

Description

Upload a model to a PVC which can then be used in our pipelines

How Has This Been Tested?

Upload model to PVC:

make MODEL_PATH="PATH_TO_A_FILE" NAME=my-model create

You should get a final output showing details of the upload

PVC name: model-upload-pvc
Size: 1G
Model path in pod: /workspace/model-upload-pvc/model_dir/model.model

You can set the SIZE and PVC values aswell

make MODEL_PATH="PATH_TO_A_FILE" NAME=my-model SIZE=1G PVC=my-new-PVC create

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

piotrpdev

Good job so far 👍

pipelines/tekton/azureml-container-pipeline/model-upload/basic-pod.yaml

pipelines/tekton/azureml-container-pipeline/model-upload/Makefile

pipelines/tekton/azureml-container-pipeline/model-upload/basic-pod.yaml

piotrpdev · 2023-10-03T21:47:39Z

Also in the A/C in the issue (#87) I specified we should provide a template PVC so please do that eventually.

jackdelahunt · 2023-10-04T12:21:37Z

Also in the A/C in the issue (#87) I specified we should provide a template PVC so please do that eventually.

@piotrpdev

How will this work, will it be a template used for the pipelines aswell or just come with this work that you can then add in the pipelineRun?

Is the idea to create the PVC and the pod at the same time and upload the model and use this PVC to them use or copy it to somewhere else? My thought was that we just pass a PVC name that is already in the project.

piotrpdev · 2023-10-04T13:46:08Z

Is the idea to create the PVC and the pod at the same time and upload the model and use this PVC to them use or copy it to somewhere else? My thought was that we just pass a PVC name that is already in the project.

I added some more detail to the issue yesterday, my bad for it being unclear, #87 (comment)

In the issue description I specified how we're probably going to switch to using VolumeClaimTemplate so each PipelineRun creates its own PVC. In this scenario, the pipelines won't be using any PVC provided by the repo (apart from the template that I asked you to include for local model use).

piotrpdev · 2023-10-05T17:12:00Z

This is also a diagram for what I'm thinking:

LaVLaS

Can you rename the templates file so they are clearly djust to show that they are templates.

local-model-pvc-template.yaml
local-model-to-pvc-pod-template.yaml

pipelines/tekton/azureml-container-pipeline/model-upload/Makefile

grdryn · 2023-11-21T12:44:39Z

pipelines/tekton/azureml-container-pipeline/model-upload/local-model-to-pvc-pod.yaml

+- apiVersion: v1
+  kind: Pod
+  metadata:
+    name: local-model-to-pvc-pod
+  spec:
+    volumes:
+      - name: ${PVC}
+        persistentVolumeClaim:
+          claimName: ${PVC}
+    containers:
+    - name: local-model-to-pvc-container
+      image: ubi9
+      stdin: true
+      tty: true
+      securityContext:
+        allowPrivilegeEscalation: false
+      volumeMounts:
+          - mountPath: /workspace/${PVC}
+            name: ${PVC}


I think you could consider putting this into the object list of the other template, if they're always expected to be processed together, and the PVC parameter will always have the same value. WDYT?

The PVC and the pod want to be deleted at different times, this is because when you upload the model file to the PVC it is mounted to the pod. For another pod to mount the PVC the pod we create must un-mount it as you can't have both mounting at the same time.

So when you are deleting them you want to do it separately.

Do we really want to rely on the same PVC being remounted for various uses?

Shouldn't the Pod writing the model to the PV use one PVC, and then Pods consuming the model different PVC(s), possibly with different selectors for labels set by the populating Pod after it has finished storing the model?

Yes the pod writing the model does only use one PVC, it is mounted then oc cp is used and then unmounted and the pod is deleted.

For our usecase the PVC we upload to is then used as a workspace in the pipelines, so the PVC it uses for the pipelines can copy over the model file. So I think adding labels etc. isn't needed for this. (Atleast I don't think so)

pipelines/tekton/azureml-container-pipeline/model-upload/local-model-pvc.yaml

pipelines/tekton/azureml-container-pipeline/model-upload/local-model-to-pvc-pod-template.yaml

pipelines/tekton/azureml-container-pipeline/model-upload/Makefile

pipelines/tekton/build-container-image-pipeline/check-model-and-containerfile-exists.yaml

adelton · 2023-11-21T20:58:01Z

Please note that the use of model_dir seems to cause #175.

pipelines/tekton/azureml-container-pipeline/model-upload/Makefile

pipelines/tekton/azureml-container-pipeline/model-upload/local-model-pvc-template.yaml

biswassri · 2023-11-29T15:16:13Z

@jackdelahunt It might be a good idea to update the ReadMe file with steps for anyone trying this? Even just the make command and some details about local pvc upload I think might help.

jackdelahunt · 2023-11-29T16:25:14Z

@biswassri sure will do

pipelines/tekton/azureml-container-pipeline/model-upload/Makefile

grdryn

LGTM, thanks @jackdelahunt!

openshift-ci · 2023-11-30T11:12:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grdryn, jackdelahunt

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

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

adelton · 2023-12-13T08:50:30Z

Hello @jackdelahunt, was there a specific ready why the Using local models in pipelines documentation section went to the top-level README.md and not to pipelines/README.md, seeing the whole PR was in the pipelines/ space?

…pload Local model upload to PVC

jackdelahunt force-pushed the local-model-upload branch from 789ce27 to 6285490 Compare October 3, 2023 12:28

piotrpdev reviewed Oct 3, 2023

View reviewed changes

piotrpdev mentioned this pull request Oct 3, 2023

Let pipelines use model from PVC #87

Open

piotrpdev mentioned this pull request Oct 5, 2023

Add Jupyter Notebook to train tensorflow-housing model #126

Open

piotrpdev added kind/documentation Improvements or additions to documentation kind/enhancement New feature or request priority/high Important issue that needs to be resolved asap. Releases should not have too many o labels Oct 5, 2023

piotrpdev added priority/blocker Critical issue that needs to be fixed asap; blocks up coming releases and removed priority/high Important issue that needs to be resolved asap. Releases should not have too many o labels Oct 13, 2023

jackdelahunt force-pushed the local-model-upload branch 4 times, most recently from f2b6591 to 737bc9a Compare November 13, 2023 11:58

jackdelahunt changed the title ~~Draft: Local model upload to PVC~~ Local model upload to PVC Nov 16, 2023

LaVLaS self-requested a review November 21, 2023 03:21

LaVLaS reviewed Nov 21, 2023

View reviewed changes

pipelines/tekton/azureml-container-pipeline/model-upload/Makefile Outdated Show resolved Hide resolved

grdryn reviewed Nov 21, 2023

View reviewed changes

pipelines/tekton/azureml-container-pipeline/model-upload/local-model-pvc.yaml Outdated Show resolved Hide resolved

grdryn reviewed Nov 21, 2023

View reviewed changes

adelton requested changes Nov 21, 2023

View reviewed changes

openshift-ci bot assigned adelton Nov 21, 2023

adelton removed their assignment Nov 21, 2023

jackdelahunt added 3 commits November 27, 2023 11:40

feat: uploading model to pvc

8209a18

feat: pvc option for fetching

603e22e

refactor: added comments and renamed template files

b80741b

jackdelahunt force-pushed the local-model-upload branch from af707e0 to 4829e7b Compare November 27, 2023 11:40

fix: better defualt values for PVC template

7c62c86

jackdelahunt force-pushed the local-model-upload branch from 4829e7b to 7c62c86 Compare November 27, 2023 11:43

refactor: deleting pod from PVC upload is now easier

cd2c8d2

jackdelahunt force-pushed the local-model-upload branch from ab29662 to cd2c8d2 Compare November 28, 2023 10:25

jackdelahunt added 2 commits November 28, 2023 18:06

feat: now using single command to create and upload model

b2eed61

fix: paths working and defaults for make

811bae6

jackdelahunt requested a review from grdryn November 29, 2023 10:39

grdryn reviewed Nov 29, 2023

View reviewed changes

docs: changed README and updated Makefile docs

194632d

jackdelahunt force-pushed the local-model-upload branch from ff724b1 to 194632d Compare November 30, 2023 11:06

grdryn approved these changes Nov 30, 2023

View reviewed changes

openshift-ci bot assigned grdryn Nov 30, 2023

openshift-ci bot added the lgtm label Nov 30, 2023

openshift-ci bot added the approved label Nov 30, 2023

openshift-merge-bot bot merged commit 0594164 into opendatahub-io:main Nov 30, 2023
2 checks passed

adelton mentioned this pull request Dec 13, 2023

[BUG]: pipeline requires workspace with name "model-workspace" be provided by pipelinerun #190

Closed

jackdelahunt pushed a commit to jackdelahunt/ai-edge that referenced this pull request Jan 23, 2024

Merge pull request opendatahub-io#115 from jackdelahunt/local-model-u…

bb59f10

…pload Local model upload to PVC

jackdelahunt deleted the local-model-upload branch April 23, 2024 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local model upload to PVC #115

Local model upload to PVC #115

jackdelahunt commented Oct 3, 2023 •

edited

Loading

piotrpdev left a comment

piotrpdev commented Oct 3, 2023 •

edited

Loading

jackdelahunt commented Oct 4, 2023 •

edited

Loading

piotrpdev commented Oct 4, 2023 •

edited

Loading

piotrpdev commented Oct 5, 2023

LaVLaS left a comment

grdryn Nov 21, 2023

jackdelahunt Nov 21, 2023

adelton Nov 21, 2023

jackdelahunt Nov 27, 2023

adelton commented Nov 21, 2023

biswassri commented Nov 29, 2023

jackdelahunt commented Nov 29, 2023

grdryn left a comment

openshift-ci bot commented Nov 30, 2023

adelton commented Dec 13, 2023

Local model upload to PVC #115

Local model upload to PVC #115

Conversation

jackdelahunt commented Oct 3, 2023 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

piotrpdev left a comment

Choose a reason for hiding this comment

piotrpdev commented Oct 3, 2023 • edited Loading

jackdelahunt commented Oct 4, 2023 • edited Loading

piotrpdev commented Oct 4, 2023 • edited Loading

piotrpdev commented Oct 5, 2023

LaVLaS left a comment

Choose a reason for hiding this comment

grdryn Nov 21, 2023

Choose a reason for hiding this comment

jackdelahunt Nov 21, 2023

Choose a reason for hiding this comment

adelton Nov 21, 2023

Choose a reason for hiding this comment

jackdelahunt Nov 27, 2023

Choose a reason for hiding this comment

adelton commented Nov 21, 2023

biswassri commented Nov 29, 2023

jackdelahunt commented Nov 29, 2023

grdryn left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 30, 2023

adelton commented Dec 13, 2023

jackdelahunt commented Oct 3, 2023 •

edited

Loading

piotrpdev commented Oct 3, 2023 •

edited

Loading

jackdelahunt commented Oct 4, 2023 •

edited

Loading

piotrpdev commented Oct 4, 2023 •

edited

Loading