Minor accelerator fixes #1764

Gkrumbach07 · 2023-09-06T20:23:32Z

closes: #1765

Description

Minor UX and bug fixes found by testers

making naming in the error states of the dropdown dynamic so they can use image or servingruntime
improved backend logging to be more verbose and not miss any exit points
remove double array usage in accelerator state hook
remove usage of unknown when we know that there is not an accelerator for description fields and logging - replaced with None when there is no accelerator, and unknown when we use existing settings
prevent count from going to 0 -> same logic as other number inputs. The default and limit are now 1.
removed italics from dropdown None option

How Has This Been Tested?

Migration should always give a logging on error, success, or skipping
test that errors display correctly in migration (try removing crd for acceleratorprofiles so it throws a 404 error on migration)
serving runtime dropdown errors should not reference an image
count should always default to 1 and should not be able to go to 0

Test Impact

no tests are made for this feature yet. These will be created once the feature has gone through development

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
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 tests & storybook 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 (find relevant UX in the SMEs section).

There are UI changes, but they are not changes in the UX, just small bugs. I will still tag UX, however there is not much action needed.
@yannnz

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

lucferbux

Just a small nit, rest of the changes lgtm, I like the detail of the resources 👍

backend/src/routes/api/accelerators/acceleratorUtils.ts

backend/src/utils/resourceUtils.ts

lucferbux · 2023-09-08T12:23:47Z

backend/src/utils/resourceUtils.ts

-          e.response?.body?.message || e.message,
-        );
+        throw `A ${e.statusCode} error occurred when trying to create gpu migration configmap: ${
+          e.response?.body?.message || e?.response?.statusMessage


For example you are using or here

Gkrumbach07 · 2023-09-08T14:11:30Z

@lucferbux I migrated everything that was using || to use ??

andrewballantyne · 2023-09-08T15:51:40Z

frontend/src/pages/modelServing/screens/projects/ServingRuntimeDetails.tsx

@@ -47,7 +47,7 @@ const ServingRuntimeDetails: React.FC<ServingRuntimeDetailsProps> = ({ obj }) =>
      <DescriptionListGroup>
        <DescriptionListTerm>Accelerator</DescriptionListTerm>
        <DescriptionListDescription>
-          {accelerator.accelerator?.spec.displayName || 'unknown'}
+          {accelerator.accelerator?.spec.displayName || 'None'}


Would this cause a problem with existing Model Servers?

for exisitng model servers, the useAcceleratorState still gets called and we evaluate what accelerator they are using. In this flow exisitng serving runtimes would wither fall into the useExisiting state or the Nvidia GPU state. It wont be linked to a profile, but it would be uses the fake profile (soft migration or whatever we call it)

andrewballantyne · 2023-09-08T15:53:16Z

frontend/src/pages/notebookController/screens/server/NotebookServerDetails.tsx

@@ -108,7 +108,7 @@ const NotebookServerDetails: React.FC = () => {
        <DescriptionListGroup>
          <DescriptionListTerm>Accelerator</DescriptionListTerm>
          <DescriptionListDescription>
-            {accelerator.accelerator?.spec.displayName || 'unknown'}
+            {accelerator.accelerator?.spec.displayName || 'None'}


Same here -- if the value is 0 -- none makes sense... if the value > 0 -- this is odd.

we should have an unknown and a none
so three cases
accelerator is defined: use displayName
accelerator is undefined AND useExisting is true: use "Unknown"
accelerator is undefined AND useExisting is false: use "None"

making image/servingruntime naming dynamic fix count going back to 0 prevent 0 count and ux style fix removed usage of unknown when not needed remove double array usage improved backend logging fix logging undefined error make ?? consistent fixed "||" and fixed unknown / none details

andrewballantyne

@Gkrumbach07 please update your screenshots

/hold

Remove the hold when you have done so -- it'll be easier for Yan to read next week.

openshift-ci · 2023-09-08T17:26:56Z

[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:

~~OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from alexcreasy and DaoDaoNoCode September 6, 2023 20:23

Gkrumbach07 changed the title ~~Minor fixes~~ Minor accelerator fixes Sep 6, 2023

andrewballantyne added the pr/no-tests-allowed Added by an official approver - this PR is allowed no tests. Omitted, a test must accompany the PR label Sep 7, 2023

Gkrumbach07 assigned Gkrumbach07 and unassigned Gkrumbach07 Sep 7, 2023

Gkrumbach07 linked an issue Sep 7, 2023 that may be closed by this pull request

[Bug]: Minor fixes to accelerator support feature #1765

Closed

1 task

Gkrumbach07 force-pushed the minor-fixes branch from 4c72bbd to d0a261d Compare September 7, 2023 19:15

lucferbux requested changes Sep 8, 2023

View reviewed changes

openshift-ci bot assigned lucferbux Sep 8, 2023

Gkrumbach07 force-pushed the minor-fixes branch from d0a261d to 00e91b0 Compare September 8, 2023 14:11

lucferbux self-requested a review September 8, 2023 15:19

andrewballantyne reviewed Sep 8, 2023

View reviewed changes

Gkrumbach07 force-pushed the minor-fixes branch from 00e91b0 to a92ab5b Compare September 8, 2023 16:35

andrewballantyne approved these changes Sep 8, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Sep 8, 2023

openshift-ci bot assigned andrewballantyne Sep 8, 2023

openshift-ci bot added the lgtm label Sep 8, 2023

openshift-ci bot added the approved label Sep 8, 2023

Gkrumbach07 removed the do-not-merge/hold This PR is hold for some reason label Sep 8, 2023

openshift-merge-robot merged commit dcd23ec into opendatahub-io:f/accelerator-support Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor accelerator fixes #1764

Minor accelerator fixes #1764

Gkrumbach07 commented Sep 6, 2023 •

edited

Loading

lucferbux left a comment

lucferbux Sep 8, 2023

Gkrumbach07 commented Sep 8, 2023

andrewballantyne Sep 8, 2023

Gkrumbach07 Sep 8, 2023

andrewballantyne Sep 8, 2023

Gkrumbach07 Sep 8, 2023

andrewballantyne left a comment

openshift-ci bot commented Sep 8, 2023

Minor accelerator fixes #1764

Minor accelerator fixes #1764

Conversation

Gkrumbach07 commented Sep 6, 2023 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

lucferbux left a comment

Choose a reason for hiding this comment

lucferbux Sep 8, 2023

Choose a reason for hiding this comment

Gkrumbach07 commented Sep 8, 2023

andrewballantyne Sep 8, 2023

Choose a reason for hiding this comment

Gkrumbach07 Sep 8, 2023

Choose a reason for hiding this comment

andrewballantyne Sep 8, 2023

Choose a reason for hiding this comment

Gkrumbach07 Sep 8, 2023

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 8, 2023

Gkrumbach07 commented Sep 6, 2023 •

edited

Loading