Closed biswassri closed 1 month ago
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign grdryn for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
So was just looking at this and wanted to get a better understanding of what is happening.
Fetching from the model registry is now the default and the results from the model registry (info about model in s3 or git) are passed to the task that will then get the model? Is that correct?
Is it expected that we only use model registry now?
The PR check failure appears to be as a result of the following error in the PipelineRun:
[pod/aiedge-e2e-bike-rentals-auto-ml-k8s75-build-container-image-pod/step-build-and-push] [1/2] STEP 5/6: COPY $MODEL_DIR /opt/app-root/src/model/
[pod/aiedge-e2e-bike-rentals-auto-ml-k8s75-build-container-image-pod/step-build-and-push] Error: building at STEP "COPY $MODEL_DIR /opt/app-root/src/model/": checking on sources under "/workspace/source/model_dir-bike-rentals-auto-ml/bike-rentals-auto-ml": copier: stat: "/model_dir-bike-rentals-auto-ml": no such file or directory
Adding a reply for historical reference.
Also, is this somehow aligned with the CLI work? Currently we have 2 approaches to Model registry:
- CLI - where we store all pipeline params in the model registry
There was a discussion about adding a CLI to support this entire workflow but the scope of the work requires collaboration with the model registry in order to differentiate from the functionality included in the tekton cli.
- This - where we get model location from the model registry during the run of the pipeline
From an offline conversation with @biswassri about https://github.com/opendatahub-io/ai-edge/pull/246#discussion_r1617421656, the intent was to use the model-registry as an abstraction layer for retrieving the model storage information. Long term, I think this approach is the better solution as we want to have tasks that can be used to fetch from git or s3 directly with the option of putting some type of registry abstraction task in front that can retrieve the storage information
/hold
I'm putting this PR on hold while @biswassri adds changes based on recent comments
@LaVLaS, @MarianMacik @grdryn I have addressed all the comments (hopefully) and pushed the changes. Please let me know if I've missed any and if we are good to merge. FYI, I've also changed the way we are reading from MR based on Landon's suggestion.
/unhold
As discussed offline with @LaVLaS, this PR will interfere and complicate the refractor that is planned as a part of RHOAISTRAT 234. Hence closing out this PR. However, I will not delete this branch from my private repo and will have the changes incase we decide to revisit this in the future.
Adding support to use model-registry and fetch model properties from it when enabled and to continue supporting the existing pipeline without MR deployed. JIRA: RHOAIENG-4146
Description
Added a Task that fetches properties stored in model registry when the parameter
model-registry-enabled
is true. So far it only fetches the model source details (git repo, revision etc.) and not the containerfile details.The added task
fetch-data
runs always, whenmodel-registry-enabled
is set totrue
then the Task connects to MR and fetches all the model related properties from the MR specified in themodel-registry-hostname
. The fetch-git and fetch-s3 tasks runs based on the results of fetch-model from this task. When set tofalse
it passes on the properties from the PipelineRun file.Below is a sample curl command used to save the model properties in the MR.
How Has This Been Tested?
serviceRoute
set to enabled state.model-registry-hostname
parameter to the pipeline run file$oc apply -k tekton/aiedge-e2e/ $oc create -f tekton/aiedge-e2e/aiedge-e2e.tensorflow-housing.pipelinerun.yaml
You'd expect the below green pipelineRun :
Merge criteria: