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

Cleanup deprecated usage of gpu #2182

Conversation

Gkrumbach07
Copy link
Member

closes: #1894

Description

this pr does the following

  • removes backend /gpu and all related utils, types, frontend services, frontend UI components, hooks that use that api endpoint
  • moves model of accelerator profile out of openshift file and into odh file
  • renames variables, components, params, hooks, utils so that
    • when talking about hardware accelerators (identifiers, detected counts) then you dont need to use profiles in the name
    • when talking about actual accelerator profiles (the CRD), then I changed the name to reflect that. In some cases for looping functions i use the name cr (custom resource)
  • renamed two modal texts to be clear it is talking about accelerator profiles and not accelerators (wording has been used from what Katie suggested)
  • Did not touch manifests or annotation names

How Has This Been Tested?

  • all tests pass
  • run through e2e flow of:
    • creating, editing, removing accelerator profiles
    • setting recommended accelerators for byon
    • making sure workbenches, runtimes, and notebook spawner all get the correct, toelrations, annotations, and resources from the selected accelerator

Test Impact

all tests pass. had to edit delete test bc name of button changed

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).

no UI changes, just naming of code. any wording changes have already been verified by Katie

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

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.

Can you actually remove gpuSetting from the DashboardConfig CRD -- I'm fairly certain this will just drop the value from the config since it's not longer used or needed. Please test though.

wording and linter

updated naming

remove gpuSetting
return getCompatibleAcceleratorIdentifiers(obj).some(
(accelerator) => accelerator === acceleratorIdentifier,
);
return getCompatibleAcceleratorIdentifiers(obj).some((cr) => cr === acceleratorIdentifier);
Copy link
Member

Choose a reason for hiding this comment

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

Weird to call it cr 🤔 But variable names are not the end of the world...

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yeah in that area it is weird i agree. should be something like identifier

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.

Your description is a little off -- but otherwise this looks good to me.

@openshift-ci openshift-ci bot added the lgtm label Nov 27, 2023
Copy link
Contributor

openshift-ci bot commented Nov 27, 2023

[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 c50da8c into opendatahub-io:f/accelerator-admin-support Nov 27, 2023
4 checks passed
@andrewballantyne andrewballantyne linked an issue Nov 29, 2023 that may be closed by this pull request
1 task
@Gkrumbach07 Gkrumbach07 deleted the cleanup-gpu branch January 2, 2024 16:02
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.

[Feature Request]: accelerator feature cleanup [Bug]: GPU Error shows up on Non-GPU Clusters
2 participants