-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat(csi): restructure the CSI as different mr executable #601
Conversation
Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
14f9f2b
to
5cc47ab
Compare
5cc47ab
to
694053a
Compare
Signed-off-by: Andrea Lamparelli <a.lamparelli95@gmail.com>
694053a
to
d09c694
Compare
As discussed in slack, here the final proposal with the suggested changes 🚀 |
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
I will close #598 in favor of this, thank you for taking care of this issue 🙏
All tests pass locally and on CI, I also checked possible differences in image size and everything works as intended.
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.
per discussions in the KF Model Registry biweekly meeting last 2024-12-09, thanks a lot @lampajr for these contributions and thank you @Al-Pragliola for the great help too
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Al-Pragliola, 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 |
Fixes #338
Description
With this PR I am proposing a possible solution to the way CSI is managed here (ref. #338).
Instead of having a separate Go module, I am just proposing to simply create a new executable in a dedicated folder that makes use of model registry source code (as it was previously though)
This should completely remove the issue we were encountaring with false negative on dependabot.
Pro
Cons
kserve
dependency in the model registry even if for the model-registry itself is not actually requiredHow Has This Been Tested?
Updated the existing CI tests
Merge criteria:
DCO
check)If you have UI changes