-
Notifications
You must be signed in to change notification settings - Fork 19
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
[RHOAIEDGE-15] Adding option to fetch from git #112
Conversation
Other options that I've tried:
Problems with the above changes:
I'm fairly new to tekton so all suggestions are welcome :) |
pipelines/tekton/azureml-container-pipeline/kserve-download-model.yaml
Outdated
Show resolved
Hide resolved
pipelines/tekton/azureml-container-pipeline/kserve-download-model.yaml
Outdated
Show resolved
Hide resolved
pipelines/tekton/azureml-container-pipeline/kserve-download-model.yaml
Outdated
Show resolved
Hide resolved
pipelines/tekton/azureml-container-pipeline/kserve-download-model.yaml
Outdated
Show resolved
Hide resolved
pipelines/tekton/build-container-image-pipeline/build-container-image-pipeline.yaml
Outdated
Show resolved
Hide resolved
@biswassri Do these new steps need to be ordered within the main pipeline using runAfter ? |
Actually not. We only have one step(as task) added in the main-pipeline and since it's when condition based it's difficult to be order. The tasks following the download model one are ordered as before. The success/execution of this new |
pipelines/tekton/build-container-image-pipeline/build-container-image-pipeline.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've finished the review, some additional changes are likely needed.
pipelines/tekton/build-container-image-pipeline/kserve-download-model.yaml
Outdated
Show resolved
Hide resolved
...es/tekton/build-container-image-pipeline/build-container-image-pipelinerun-bike-rentals.yaml
Outdated
Show resolved
Hide resolved
pipelines/tekton/build-container-image-pipeline/build-container-image-pipeline.yaml
Outdated
Show resolved
Hide resolved
pipelines/tekton/build-container-image-pipeline/build-container-image-pipeline.yaml
Show resolved
Hide resolved
pipelines/tekton/build-container-image-pipeline/build-container-image-pipeline.yaml
Show resolved
Hide resolved
...es/tekton/build-container-image-pipeline/build-container-image-pipelinerun-bike-rentals.yaml
Outdated
Show resolved
Hide resolved
Updating from an offline conversation: @biswassri will be updating the affected Pipelineruns with the missing parameters |
- name: fetch-model | ||
type: string | ||
- name: git-model-repo | ||
type: string | ||
default: "" | ||
- name: git-revision |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some parameter names are camelCase
while others are kebab-case
should we try make these consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
/approve The build-container workflow worked for me when deploying locally and fetching from S3. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: biswassri, jackdelahunt, LaVLaS 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 |
@@ -56,12 +56,16 @@ oc create -f tekton/build-container-image-pipeline/aws-env-real.yaml | |||
|
|||
### Deploy and run the build pipeline | |||
|
|||
#### For S3 fetch | |||
Update the `aws-bucket-name` parameter value from its default `rhoai-edge-models` in | |||
[`build-container-image-pipelinerun-bike-rentals.yaml`](tekton/build-container-image-pipeline/build-container-image-pipelinerun-bike-rentals.yaml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer seems to be true -- filed as #174. The README should describe the default behaviour more clearly, and the proper steps to change the default behaviour (switch from S3 to git and vice versa).
- name: gitInitImage | ||
value: registry.redhat.io/openshift-pipelines/pipelines-git-init-rhel8@sha256:1a50511583fc02a27012d17d942e247813404104ddd282d7e26f99765174392c | ||
- name: subdirectory | ||
value: /model_dir/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something about the static model_dir
path is wrong: #175.
[RHOAIEDGE-15] Adding option to fetch from git
Description
Fixes #57
Added a git-clone task to clone the model repo from a git source. This addition required some model paths to be updated in multiple places.
How Has This Been Tested?
Created a new pipeline run with the changes in my
srbiswas-pipeline-dev
project and was able to have a successful pipeline run.Merge criteria: