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

Update CSI #154

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Update CSI #154

merged 2 commits into from
Jul 25, 2024

Conversation

lampajr
Copy link
Member

@lampajr lampajr commented Jun 23, 2024

Description

Proposed changes:

  • Update custom storage initializer
  • Add e2e tests
  • Add workflow testing CSI with dev model registry
  • Push CSI container image

How Has This Been Tested?

Added additional workflow that tests the CSI against the local model-registry codebase

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
  • 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

@lampajr lampajr force-pushed the csi_improvement branch 6 times, most recently from 656afe4 to 0fb202b Compare June 23, 2024 19:50
dhirajsb pushed a commit to dhirajsb/model-registry-kfp that referenced this pull request Jul 10, 2024
* Manage *ByParams errors

* Fix runtime movement to inference service

* Add mapper test
@lampajr lampajr force-pushed the csi_improvement branch 2 times, most recently from 7ad6312 to ffe1e3f Compare July 22, 2024 10:11
Signed-off-by: Andrea Lamparelli <[email protected]>
csi/Dockerfile.dev Outdated Show resolved Hide resolved
@lampajr
Copy link
Member Author

lampajr commented Jul 22, 2024

With the latest changes I was able to fix the e2e test in the CI, right now the e2e CI checks the custom storage initializer properly works with:

  • The local model-registry module as dependency (it uses replace directive)
  • Against the local model-registry server (built from the local source)

Push CSI container image

This is the only missing piece, as there are a couple of things to clarify:

Continuous build&push

  • Do we want to tag the CSI using a specific version of the model-registry library?
  • Or is it fine to build and push the CSI using the local model-registry module as I did for the test? (this mainly for the continuous main branch push)

Model registry release

  • How do we want to build, push and release the CSI? As a separate step making use of the specific model-registry version?
  • Or simply doing it all-in-once by using the local model-registry module?

IMHO using the local model-registry module is much easier and it guarantees that model-registry and CSI are built and release from the same commit, but I'm happy to hear other feedbacks here

@lampajr lampajr marked this pull request as ready for review July 22, 2024 13:15
@google-oss-prow google-oss-prow bot requested a review from rareddy July 22, 2024 13:15
@lampajr
Copy link
Member Author

lampajr commented Jul 22, 2024

Another question, do we want to move the csi folder under the clients? I am not completely sure about this as the CSI is not actually a "client" of the model-registry but it could be considered so as it interacts with it

@tarilabs
Copy link
Member

This is the only missing piece

@lampajr
Copy link
Member Author

lampajr commented Jul 23, 2024

As discussed and agreed during the last community mtg, I've added a new workflow to build, tag and push the CSI image using the local model-registry library. See commit a384507.

I would say also ensure some "reverse-DNS-name" for the labels being used, wdyt ?

@tarilabs sorry, I missed this comment yesterday, but I am not sure what you mean by that 🤔 could you please elaborate more?

location for CSI code (Update CSI #154 (comment))

I think we could move this discussion in a separate thread/issue to avoid blocking this PR, wdyt?

Signed-off-by: Andrea Lamparelli <[email protected]>
@tarilabs
Copy link
Member

As discussed and agreed during the last community mtg, I've added a new workflow to build, tag and push the CSI image using the local model-registry library. See commit a384507.

awesome, thank you.

I would say also ensure some "reverse-DNS-name" for the labels being used, wdyt ?
@tarilabs sorry, I missed this comment yesterday, but I am not sure what you mean by that 🤔 could you please elaborate more?

I was wrong, I confused with another capability; sorry.
--> nevermind.

location for CSI code (Update CSI #154 (comment))
I think we could move this discussion in a separate thread/issue to avoid blocking this PR, wdyt?

Agree!
Do you want to track a discussion point for the next KF MR biweekly meeting?

@tarilabs
Copy link
Member

thank you so much for your outstanding continued efforts @lampajr on this topic, the Model Registry WG appreciates this as also mentioned in the biweekly meeting last 2024-07-22.

/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tarilabs

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

@google-oss-prow google-oss-prow bot merged commit d1aae5b into kubeflow:main Jul 25, 2024
15 checks passed
@lampajr lampajr deleted the csi_improvement branch July 25, 2024 07:18
@lampajr
Copy link
Member Author

lampajr commented Jul 25, 2024

Agree!
Do you want to track a discussion point for the next KF MR biweekly meeting?

Yeah, sure!

thank you so much for your outstanding continued efforts @lampajr on this topic, the Model Registry WG appreciates this as also mentioned in the biweekly meeting last 2024-07-22.

Awesome, thanks a lot @tarilabs !!

I see the main-* tag has been properly pushed in Dockerhub 🚀

For the records I tested the pushed image locally with:

docker pull kubeflow/model-registry-storage-initializer:main-d1aae5b
MR_CSI_IMG=kubeflow/model-registry-storage-initializer:main-d1aae5b ./test/e2e_test.sh 

And it worked ✔️

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.

2 participants