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

Serving impovements #937

Conversation

andrewballantyne
Copy link
Member

@andrewballantyne andrewballantyne commented Feb 8, 2023

Closes: #910

Description

Misc improvements to Model Serving -- see ticket for the full list.

How Has This Been Tested?

You'll need multiple projects and a few models within' to test some of the effort on the global model serving page.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • 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

@andrewballantyne andrewballantyne requested review from lucferbux and Gkrumbach07 and removed request for maroroman February 8, 2023 18:57
@Gkrumbach07
Copy link
Member

Allow model server replicas to scale to 0

Configure becomes disabled even when a valid replica of 0 is set. Set to createData.numReplicas >= 0

@Gkrumbach07
Copy link
Member

(2) allow Request and Limit to be the same value

Same issue as above with validation

parseInt(createData.modelSize.resources.limits.cpu) >
parseInt(createData.modelSize.resources.requests.cpu) &&
parseInt(createData.modelSize.resources.limits.memory) >
parseInt(createData.modelSize.resources.requests.memory);

Also, is it an issue that a user can request a limit of 1 milicore but request a whole core (same with memory units)?
Maybe there should be an additional validation for differing units. Possibly also an error message as this could be a not obvious issue to a user

@andrewballantyne
Copy link
Member Author

(2) allow Request and Limit to be the same value

Ah good call on the units -- and wow, I totally missed no2 when I was going through. Thanks!

@Gkrumbach07
Copy link
Member

/lgtm

@andrewballantyne andrewballantyne mentioned this pull request Feb 9, 2023
3 tasks
@lucferbux
Copy link
Contributor

/lgtm
Code seems great, tested visual changes and everything seems to be checked, played around with the logic of the limits and requests and they behave as expected

@lucferbux
Copy link
Contributor

/approve

@andrewballantyne andrewballantyne added do-not-merge/hold This PR is hold for some reason approved labels Feb 9, 2023
@andrewballantyne
Copy link
Member Author

Trying to wait on #935 as we are going to conflict and I have another PR that also conflicts on 935 -- so I agreed with Chris he can merge first (assuming he is able to clean it up in a timely manner)

@lucferbux
Copy link
Contributor

Ok perfect!

@andrewballantyne
Copy link
Member Author

Looks to be up to date now with the main branch after gpu addition to the model serving

@lucferbux
Copy link
Contributor

Testing it now

Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 10, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 10, 2023

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: Gkrumbach07, lucferbux

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-robot openshift-merge-robot merged commit 8e6dd01 into opendatahub-io:main Feb 10, 2023
lucferbux pushed a commit to red-hat-data-services/odh-dashboard that referenced this pull request Feb 13, 2023
strangiato pushed a commit to strangiato/odh-dashboard that referenced this pull request Oct 18, 2023
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]: Misc Model Serving Improvements
4 participants