Validation for model names and disabled NIM metrics hyperlink #3314
Conversation
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @olavtar. 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 Once the patch is verified, the new status will be reflected by the 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3314 +/- ##
==========================================
+ Coverage 84.79% 85.04% +0.25%
==========================================
Files 1315 1329 +14
Lines 29491 29927 +436
Branches 8056 8197 +141
==========================================
+ Hits 25006 25451 +445
+ Misses 4485 4476 -9
... and 143 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
| </FormGroup> | ||
| ); | ||
| const ProjectSection: React.FC<ProjectSectionType> = ({ projectName }) => { | ||
| const [translatedName] = translateDisplayNameForK8sAndReport(projectName); |
There was a problem hiding this comment.
The project is not the problem -- it's the name used for the inference service and/or servingruntime, no?
| > | ||
| {displayName} | ||
| </Link> | ||
| ) : kserveMetricsSupported ? ( |
There was a problem hiding this comment.
Curious why you didn't just update the kserveMetricsSupported to include "not NIM"?
Although this code block we have looks very weird lol
There was a problem hiding this comment.
I did in the beginning but then I saw that modelMeshMetricsSupported and kserveMetricsSupported had duplicated logic, so I chose to add the isKServeNIMEnabled check separately for clarity and to make the logic explicit. I can update the kserveMetricsSupported to handle the "not NIM" case if you think it would make it cleaner.
There was a problem hiding this comment.
Yeah, I noticed that -- the more you change the longer it takes for me to verify and approve. I'll try to get this before my PTO -- but typically stay short and to the point unless the logic is not compatible. Every cleanup you do for us has me testing more to make sure we don't have some other brittle aspect that can slip out in non NIM ways. In the end it's likely a net positive, just expressing how I look at everything.
There was a problem hiding this comment.
Hi @olavtar pardon me cause I don't have all the context, but shouldn't this produce the same result?
<ResourceNameTooltip resource={inferenceService}>
{modelMeshMetricsSupported || kserveMetricsSupported ? (
<Link
data-testid={`metrics-link-${displayName}`}
to={
isGlobal
? `/modelServing/${inferenceService.metadata.namespace}/metrics/${inferenceService.metadata.name}`
: `/projects/${inferenceService.metadata.namespace}/metrics/model/${inferenceService.metadata.name}`
}
>
{displayName}
</Link>
) : (
displayName
)}
</ResourceNameTooltip>There's value on refactoring duplicate code, but how NIM is going to affect this component if the fallback is the same as having the feature selected? Maybe for readability, but in case we wanna add later metrics, we can add a comment explaining that nim will be enabled when we support metrics.
There was a problem hiding this comment.
Hi @lucferbux, the isKServeNIMEnabled check is important because we don’t want to show a link when NIM Metrics isn't enabled, even if other metrics conditions are met. So, removing this condition would change the behavior, as it would allow the link to be displayed when NIM Metrics are not enabled, which is not the intended outcome.
There was a problem hiding this comment.
Yeah we are going to definitely not want fix this logic and also just duplicate other logic.
Go with something like this:
const InferenceServiceTableRow: React.FC<InferenceServiceTableRowProps> = ({
obj: inferenceService,
servingRuntime,
onDeleteInferenceService,
onEditInferenceService,
isGlobal,
columnNames,
}) => {
const { projects } = React.useContext(ProjectsContext);
const project = projects.find(byName(inferenceService.metadata.namespace)) ?? null;
const isKServeNIMEnabled = project ? isProjectNIMSupported(project) : false;You'll have to move this logic to the top of the component to use it where we need it
const [modelMetricsEnabled] = useModelMetricsEnabled();
const kserveMetricsEnabled = useIsAreaAvailable(SupportedArea.K_SERVE_METRICS).status;
const modelMesh = isModelMesh(inferenceService);
const modelMeshMetricsSupported = modelMetricsEnabled && modelMesh;
const kserveMetricsSupported = modelMetricsEnabled && kserveMetricsEnabled && !modelMesh && !isKServeNIMEnabled;KServe Metrics are not supported when you're NIM -- flip this flag
const displayName = getDisplayNameFromK8sResource(inferenceService);
return (
<>
<Td dataLabel="Name">
<ResourceNameTooltip resource={inferenceService}>
{modelMeshMetricsSupported || kserveMetricsSupported ? (
<Link
data-testid={`metrics-link-${displayName}`}
to={
isGlobal
? `/modelServing/${inferenceService.metadata.namespace}/metrics/${inferenceService.metadata.name}`
: `/projects/${inferenceService.metadata.namespace}/metrics/model/${inferenceService.metadata.name}`
}
>
{displayName}
</Link>
) : (
displayName
)}
</ResourceNameTooltip>
</Td>Now it should be just straight forward -- if you have metrics -- show the link -- if not hide the link.
We could go one step further with a variable "hasMetrics" instead of the two checks being OR'ed -- but I don't think that's super needed imo (but won't hurt if you like it more).
…rics hyperlink on the Models tab Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
olavtar
force-pushed
the
model-name-validation
branch
from
2cc9597
to
e1e8c6f
Compare
| @@ -19,6 +20,10 @@ const NIMModelDeploymentNameSection: React.FC<NIMModelDeploymentNameSectionProps | |||
| data-testid="model-deployment-name-section" | |||
| value={data.name} | |||
| onChange={(e, name) => setData('name', name)} | |||
| onBlur={() => { | |||
| const [translatedName] = translateDisplayNameForK8sAndReport(data.name); | |||
| setData('name', translatedName); | |||
There was a problem hiding this comment.
Would this not just convert the field for what the user typed? Can we not support this under the hood and then supply it at the end? We have a typical two named set in our world... display name & k8s name -- we don't want to lose the display name to have a clean k8s name... let me see if I can see what guidance I can provide more specifically to solve this.
There was a problem hiding this comment.
Hah. funnily enough you're already doing this logic -- I imagine the logic here is not actually going to fix the aforementioned bug on the ticket; all numbers for a name.
What you need to do is this...
- Remove these lines
- Go to your submit function in the DeployNIMServiceModal
- Make this change:
translateDisplayNameForK8s(createDataInferenceService.name, { safeK8sPrefix: 'nim-' });
The logic behind translateDisplayNameForK8s does not take into account the full digit use-case without a suggested prefix to put before it.
There was a problem hiding this comment.
@andrewballantyne This doesn't resolve the issue of allowing illegal characters. For example, if you enter the name "אקדא אעגכדע" (which contains no digits and includes a space), the problem persists.
There was a problem hiding this comment.
If you make a display name with those characters it should work fine 🤔 (or so I would think) -- k8s names cannot have it. Today we trim those out, which got you 12456 as a resulting resource name -- this had problems being just numbers -- this suggested solution should fix that.
What error are you getting? @olavtar
There was a problem hiding this comment.
This utility is used on the create project modal... this is the result of just using those Hebrew characters:
If we supply the nim- as the safeK8sPrefix, it would be that instead; it just updates the gen- to be the safeK8sPrefix instead if provided.
If you have valid k8s characters after that (like a number) you'll get those characters instead.
This generated value only happens if it is fully unaccepted k8s characters.
You can use the translateDisplayNameForK8sAndReport function if you want to see what happened and address the generated use-case differently (with say a static "nim-deployment" or something)
Signed-off-by: Olga Lavtar <[email protected]>
Signed-off-by: Olga Lavtar <[email protected]>
|
[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 |
https://issues.redhat.com/browse/RHOAIENG-13881
https://issues.redhat.com/browse/RHOAIENG-14044
Description
Added validation for model names and disabled the NIM models metrics hyperlink on the Models tab
How Has This Been Tested?
Tested locally.
Test Impact
Tested functionality: Verified that KServe still displays the metrics link.
NIM validation: Confirmed that NIM does not display the metrics link.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main