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

feat: Modify NIM enablement process #3455

Merged

Conversation

yzhao583
Copy link
Contributor

@yzhao583 yzhao583 commented Nov 8, 2024

https://issues.redhat.com/browse/NVPE-22

Description

Change the enablement process for NIM on the dashboard.

How Has This Been Tested?

Tested locally.

Test Impact

  • Go to the Applications -> Explore, and click the tile for NIM.
  • On the right side panel, there should be an enable button.
  • Click the enable button, a modal should be shown.
  • Enter the API key provided by NVIDIA, and click the submit button.
  • A processor should be shown to indicate the validation process.
  • If validation is successful, the modal should be closed, and a popup showing the success message should be displayed on the right-top corner. The odh-nim-account CR should be generated, and related resources should be generated as well.
  • Go to Application -> Enabled, the NIM tile should be there.
  • If validation fails, error message should be displayed in the modal and in the popup at the right-top corner.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.
image image image image image

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress This PR is in WIP state needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI labels Nov 8, 2024
Copy link
Contributor

openshift-ci bot commented Nov 8, 2024

Hi @yzhao583. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
frontend/src/pages/exploreApplication/GetStartedPanel.tsx Outdated Show resolved Hide resolved
frontend/src/pages/exploreApplication/GetStartedPanel.tsx Outdated Show resolved Hide resolved
frontend/src/utilities/useEnableApplication.tsx Outdated Show resolved Hide resolved
frontend/src/utilities/useEnableApplication.tsx Outdated Show resolved Hide resolved
@yzhao583 yzhao583 marked this pull request as ready for review November 11, 2024 18:02
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Nov 11, 2024
Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

/ok-to-test

backend/src/routes/api/components/list.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/nimUtils.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/nimUtils.ts Outdated Show resolved Hide resolved
frontend/src/utilities/useWatchIntegrationComponents.tsx Outdated Show resolved Hide resolved
frontend/src/utilities/useWatchIntegrationComponents.tsx Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/nimUtils.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/nimUtils.ts Outdated Show resolved Hide resolved
frontend/src/utilities/useWatchIntegrationComponents.tsx Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added ok-to-test The openshift bot needs `ok-to-test` to allow non member PRs to run the tests. and removed needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI labels Nov 12, 2024
Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Let me delete some resources on dev04 and give this a shot for real this time

backend/src/routes/api/integrations/nim/nimUtils.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
frontend/src/utilities/utils.ts Outdated Show resolved Hide resolved
Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

You got some issues around the useWatchIntegrationComponents hook...

frontend/src/utilities/useWatchIntegrationComponents.tsx Outdated Show resolved Hide resolved
frontend/src/utilities/useWatchIntegrationComponents.tsx Outdated Show resolved Hide resolved
frontend/src/utilities/useWatchIntegrationComponents.tsx Outdated Show resolved Hide resolved
Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

We are close... if we can restructure the new file to not have its own lifecycle and implementation we should be close to good to go. I'll test on the Dev04 cluster and see about how this all works out.

frontend/src/utilities/useWatchIntegrationApp.tsx Outdated Show resolved Hide resolved
Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Hah, 18 comments... not all of these are important for this PR. Lets get the critical ones done first, everything else needs to be logged and another PR put up asap to avoid it not getting done. I'll mark them in 3 stages for simplicity.

🔴 will need to be done -- they are critical / blockers to this PR. (there are 5 of them -- 2 backend, 2 around the mock, 1 delete file)
🟡 will need to be followed up -- but can miss the 2.16; more quality stuff
🟢 are stuff we need to address over time -- likely best to log issues and just approach them asap when time/duty permits

backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
backend/src/routes/api/integrations/nim/index.ts Outdated Show resolved Hide resolved
frontend/src/__mocks__/mockComponents.ts Outdated Show resolved Hide resolved
Comment on lines +51 to +56
<Button
variant={ButtonVariant.secondary}
onClick={onEnable}
isDisabled={!enablement || !canInstall}
isLoading={!loaded && !error}
>
Copy link
Member

@andrewballantyne andrewballantyne Nov 14, 2024

Choose a reason for hiding this comment

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

🟢 As noted in our discussions there are some issues around this button; making them again here for posterity.

  • Button doesn't hide after install (after modal closes)
  • Button loads then hides if installed
    • I noticed this recently and didn't say anything -- but the isLoading probably should be a Skeleton component until if we know it should be there or not
  • Button shows disabled but doesn't show why -- before it was just enablement feature but now it's possible there is another feature flag or criteria from the API

frontend/src/utilities/useWatchIntegrationComponents.tsx Outdated Show resolved Hide resolved
frontend/src/utilities/utils.ts Outdated Show resolved Hide resolved
frontend/src/utilities/useWatchIntegrationComponents.tsx Outdated Show resolved Hide resolved
frontend/src/utilities/useWatchIntegrationComponents.tsx Outdated Show resolved Hide resolved
);

if (response.error) {
// TODO: Show the error somehow
Copy link
Member

@andrewballantyne andrewballantyne Nov 14, 2024

Choose a reason for hiding this comment

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

🟢 We filtered out the item -- so this may end up needing to be a toast -- but those can be kinda finicky -- we don't want to spam it. We'll have to look into our options for this one.

@andrewballantyne
Copy link
Member

andrewballantyne commented Nov 14, 2024

/approve

Lets get some [manual] testing done and hopefully the CI passes -- then we should be good to wrap this one up.

Two remaining issues that we need follow up issues for -- one & two

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 27.70270% with 107 lines in your changes missing coverage. Please review.

Project coverage is 85.38%. Comparing base (8305246) to head (830602b).
Report is 43 commits behind head on main.

Files with missing lines Patch % Lines
frontend/src/utilities/useEnableApplication.tsx 1.69% 58 Missing ⚠️
...nd/src/utilities/useWatchIntegrationComponents.tsx 37.73% 33 Missing ⚠️
frontend/src/services/integrationAppService.ts 13.33% 13 Missing ⚠️
...d/src/pages/exploreApplication/GetStartedPanel.tsx 60.00% 2 Missing ⚠️
...pages/exploreApplication/useIntegratedAppStatus.ts 87.50% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3455      +/-   ##
==========================================
- Coverage   85.59%   85.38%   -0.21%     
==========================================
  Files        1344     1352       +8     
  Lines       30552    30860     +308     
  Branches     8471     8612     +141     
==========================================
+ Hits        26150    26349     +199     
- Misses       4402     4511     +109     
Files with missing lines Coverage Δ
.../pages/enabledApplications/EnabledApplications.tsx 100.00% <100.00%> (ø)
...ntend/src/pages/exploreApplication/EnableModal.tsx 32.07% <ø> (ø)
frontend/src/types.ts 100.00% <ø> (ø)
frontend/src/utilities/utils.ts 74.16% <100.00%> (+0.89%) ⬆️
...pages/exploreApplication/useIntegratedAppStatus.ts 87.50% <87.50%> (ø)
...d/src/pages/exploreApplication/GetStartedPanel.tsx 62.96% <60.00%> (-3.71%) ⬇️
frontend/src/services/integrationAppService.ts 13.33% <13.33%> (ø)
...nd/src/utilities/useWatchIntegrationComponents.tsx 37.73% <37.73%> (ø)
frontend/src/utilities/useEnableApplication.tsx 28.26% <1.69%> (-13.01%) ⬇️

... and 91 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8305246...830602b. Read the comment docs.

@openshift-ci openshift-ci bot added the lgtm label Nov 15, 2024
Copy link
Contributor

openshift-ci bot commented Nov 15, 2024

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

@openshift-merge-bot openshift-merge-bot bot merged commit 24024ae into opendatahub-io:main Nov 15, 2024
6 checks passed
andrewballantyne added a commit to andrewballantyne/odh-dashboard that referenced this pull request Nov 15, 2024
* 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]>
openshift-merge-bot bot pushed a commit that referenced this pull request Nov 15, 2024
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm ok-to-test The openshift bot needs `ok-to-test` to allow non member PRs to run the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants