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

Add OVMS OOTB with GPU support #1262

Merged

Conversation

lucferbux
Copy link
Contributor

@lucferbux lucferbux commented May 19, 2023

Description

Closes: #1257

How Has This Been Tested?

  1. Deploy the manifest files
  2. Check that you can see both Custom Serving Runtimes as pre-installed

Test Impact

Test will be done in the follow up testing section for Custom Serving Runtime

Screenshot 2023-05-19 at 16 01 27 Screenshot 2023-05-19 at 16 01 17

Request review criteria:

  • The commits have meaningful messages (squashes happen on merge by the bot).
  • 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.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

@lucferbux
Copy link
Contributor Author

@vconzola I've attached a screenshot just to validate if this is what you were expecting, maybe we can review the name of the GPU one

@lucferbux
Copy link
Contributor Author

@Xaenalt If we can validate the spec it would be super helpful, I'm still confused about the different images in ovms.

@andrewballantyne
Copy link
Member

@vconzola I've attached a screenshot just to validate if this is what you were expecting, maybe we can review the name of the GPU one

I think we wanted to go with (GPU Required) as it can't work on a non-GPU node.

@vconzola
Copy link

I didn't realize that both runtimes would have the exact same name. Yuck! But it is what it is. Instead of parenthetical text after the name, can we put "GPU required" in a label, just like "Pre-installed". I think that will draw more attention.

@vconzola
Copy link

@lucferbux Actually, seeing how this looks to the end user, maybe "Supports GPUs" makes more sense than "GPU required". What do you think?

@lucferbux
Copy link
Contributor Author

I didn't realize that both runtimes would have the exact same name. Yuck! But it is what it is. Instead of parenthetical text after the name, can we put "GPU required" in a label, just like "Pre-installed". I think that will draw more attention.

mmmm, the hole point of getting this to the display name is that is displayed in the selection of the runtime (I'm gonna attach new screenshots). We can add a label too, but what should we do in the modal selection?

@lucferbux
Copy link
Contributor Author

@vconzola

Screenshot 2023-05-19 at 16 01 27 Screenshot 2023-05-19 at 16 01 17

@vconzola
Copy link

I understand what you're saying. This looks good.

@Xaenalt
Copy link
Contributor

Xaenalt commented May 19, 2023

Looks good to me, we validated the correct image tag to use as well

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.

/hold

This looks good by all parties -- only holding until I get confirmation from Lucas.

@openshift-ci openshift-ci bot added do-not-merge/hold This PR is hold for some reason lgtm labels May 19, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 19, 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

@lucferbux
Copy link
Contributor Author

/hold

This looks good by all parties -- only holding until I get confirmation from Lucas.

It should be fine by now, based on conversations we had this should be the image, already tested so it's working as intended.

@lucferbux lucferbux removed the do-not-merge/hold This PR is hold for some reason label May 19, 2023
@openshift-merge-robot openshift-merge-robot merged commit 5d92707 into opendatahub-io:main May 19, 2023
5 checks passed
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.

Add GPU OVMS Custom Serving Runtime as OOTB
5 participants