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

[RHOAIEDGE-15] Adding option to fetch from git #112

Merged
merged 6 commits into from
Nov 15, 2023

Conversation

biswassri
Copy link
Contributor

@biswassri biswassri commented Oct 2, 2023

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:

  • 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

@biswassri
Copy link
Contributor Author

Other options that I've tried:

  1. adding curl in one step : but the base image doesn't contain curl in that case
  2. using when condition: This field is not supported in the step level in Tekton

Problems with the above changes:

  1. Currently passing the git path through a param but the download is dependent on the extension of the file (here it's .pkl). Should we parameterize the file extension also? Is that a good solution?

  2. The step that is false, still has a container created. Are we good with that approach?

I'm fairly new to tekton so all suggestions are welcome :)

@jackdelahunt jackdelahunt mentioned this pull request Oct 3, 2023
3 tasks
@piotrpdev piotrpdev added 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 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
@LaVLaS
Copy link
Contributor

LaVLaS commented Nov 2, 2023

@biswassri Do these new steps need to be ordered within the main pipeline using runAfter ?

@biswassri
Copy link
Contributor Author

biswassri commented Nov 2, 2023

@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 git-clone-model-repo task can be confirmed by the check-model-and-containerfile-exists hence i've skipped the ordering.

@biswassri biswassri requested a review from grdryn November 2, 2023 18:19
Copy link
Contributor

@adelton adelton left a 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.

@LaVLaS
Copy link
Contributor

LaVLaS commented Nov 13, 2023

Updating from an offline conversation: @biswassri will be updating the affected Pipelineruns with the missing parameters

@biswassri biswassri changed the title Adding option to fetch from git [RHOAIEDGE-15] Adding option to fetch from git Nov 13, 2023
Comment on lines +24 to +29
- name: fetch-model
type: string
- name: git-model-repo
type: string
default: ""
- name: git-revision
Copy link
Contributor

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?

Copy link
Contributor

@jackdelahunt jackdelahunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@LaVLaS
Copy link
Contributor

LaVLaS commented Nov 15, 2023

/approve

The build-container workflow worked for me when deploying locally and fetching from S3.

Copy link

openshift-ci bot commented Nov 15, 2023

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -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)
Copy link
Contributor

@adelton adelton Nov 16, 2023

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/
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved kind/enhancement New feature or request lgtm priority/blocker Critical issue that needs to be fixed asap; blocks up coming releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider fetching one model from Git rather than S3
5 participants