-
Notifications
You must be signed in to change notification settings - Fork 167
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: added ability to deploy more than one NIM model. #3453
feat: added ability to deploy more than one NIM model. #3453
Conversation
Hi @olavtar. Thanks for your PR. I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
frontend/src/pages/modelServing/screens/global/DeleteInferenceServiceModal.tsx
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3453 +/- ##
==========================================
- Coverage 85.65% 85.61% -0.04%
==========================================
Files 1347 1347
Lines 30676 30704 +28
Branches 8554 8554
==========================================
+ Hits 26275 26287 +12
- Misses 4401 4417 +16
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
/ok-to-test |
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.
Code-level changes/questions -- testing it out.
a630a93
to
22c02c8
Compare
const count = await fetchInferenceServiceCount(projectName); | ||
if (count === 0) { | ||
return resourcesToDelete; | ||
} |
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.
Since you updated the logic to throw an error -- instead of returning 0 -- the only way to get into this block is if you don't have the resources... did we want to try and delete the PVC still? Or... not?
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.
the if (count === 0) condition has been removed.
@@ -123,3 +124,59 @@ export const updateServingRuntimeTemplate = ( | |||
} | |||
return updatedServingRuntime; | |||
}; | |||
|
|||
export const getNIMResourcesToDelete = async ( |
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.
Anyway we can get a unit test for this? It seems the logic is not working as expected (at least on the cluster)
db50f23
to
dc25eb5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
a0fbc98
to
f6d2c37
Compare
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.
Works as expected.
Couldn't reproduce the two model PVC issue & the secrets get cleaned up.
/hold
@olavtar Can you let me know what issue it is for the unit test or push it to this PR? I want to make sure we have that covered in the backlog or in this PR itself. Thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne 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 |
Discussed with Olga -- tests are done but have an issue -- unholding to wrap up one more thing in the NIM space. /unhold |
Jira for unit test: https://issues.redhat.com/browse/NVPE-33 |
…o#3453) * feat: added ability to deploy more than one NIM model. Signed-off-by: Olga Lavtar <[email protected]> * feat: refactored NIM related logic to nimUtils Signed-off-by: Olga Lavtar <[email protected]> * feat: changes to the error handling Signed-off-by: Olga Lavtar <[email protected]> * feat: changes to the error handling Signed-off-by: Olga Lavtar <[email protected]> * feat: changes .some for .forEach Signed-off-by: Olga Lavtar <[email protected]> * feat: deleting secrets fix Signed-off-by: Olga Lavtar <[email protected]> * feat: deploying the same model pvc fix Signed-off-by: Olga Lavtar <[email protected]> * feat: added a unit test for getNIMResourcesToDelete Signed-off-by: Olga Lavtar <[email protected]> * feat: will add a unit test for getNIMResourcesToDelete later Signed-off-by: Olga Lavtar <[email protected]> --------- Signed-off-by: Olga Lavtar <[email protected]>
* feat: added ability to deploy more than one NIM model. (#3453) * feat: added ability to deploy more than one NIM model. Signed-off-by: Olga Lavtar <[email protected]> * feat: refactored NIM related logic to nimUtils Signed-off-by: Olga Lavtar <[email protected]> * feat: changes to the error handling Signed-off-by: Olga Lavtar <[email protected]> * feat: changes to the error handling Signed-off-by: Olga Lavtar <[email protected]> * feat: changes .some for .forEach Signed-off-by: Olga Lavtar <[email protected]> * feat: deleting secrets fix Signed-off-by: Olga Lavtar <[email protected]> * feat: deploying the same model pvc fix Signed-off-by: Olga Lavtar <[email protected]> * feat: added a unit test for getNIMResourcesToDelete Signed-off-by: Olga Lavtar <[email protected]> * feat: will add a unit test for getNIMResourcesToDelete later Signed-off-by: Olga Lavtar <[email protected]> --------- Signed-off-by: Olga Lavtar <[email protected]> * feat: Modify NIM enablement process (#3455) * Modify NIM enablement process * Clean up code and remove unnecessary manifests * Add logic to check the conditions of the odh-nim-account CR * Added more check for enabled integration apps * If failed to fetch integration app status, should show error on the related tile in the enable application page, should not remove the tile * check integration app status in explore application page * Fix lint issue * Fix lint issue * add annotations to the secret and the account CR * Update backend/src/routes/api/integrations/nim/index.ts Co-authored-by: Andrew Ballantyne <[email protected]> * Update backend/src/routes/api/components/list.ts Co-authored-by: Andrew Ballantyne <[email protected]> * clean up * feat: listing all NIM accounts in the namespace, returning the first one. Signed-off-by: Olga Lavtar <[email protected]> * Avoid mutating object in useWatchIntegrationComponents * Clean up * feat: added logic for displaying the tile correctly Signed-off-by: Olga Lavtar <[email protected]> * feat: changes for enabling Signed-off-by: Olga Lavtar <[email protected]> * feat: updated mock component with the new properties. Signed-off-by: Olga Lavtar <[email protected]> * feat: addressed PR comments with updates and improvements Signed-off-by: Olga Lavtar <[email protected]> * feat: backend bug fix Signed-off-by: Olga Lavtar <[email protected]> * feat: Missed change from previous commit Signed-off-by: Olga Lavtar <[email protected]> * feat: fix for the enabled page and enabled.cy.ts Signed-off-by: Olga Lavtar <[email protected]> --------- Signed-off-by: Olga Lavtar <[email protected]> Co-authored-by: Andrew Ballantyne <[email protected]> Co-authored-by: Olga Lavtar <[email protected]> * Fix NIM selection issue (#3482) * Fix NIM selection issue * Switch back to using a numerical value --------- Signed-off-by: Olga Lavtar <[email protected]> Co-authored-by: olavtar <[email protected]> Co-authored-by: yu zhao <[email protected]> Co-authored-by: Olga Lavtar <[email protected]>
https://issues.redhat.com/browse/NVPE-15
Description
Ability to deploy more than one NIM model.
How Has This Been Tested?
Tested locally.
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main