feat(serving): add "Model URI" field to deployment modal #3807

emilys314 · 2025-02-26T15:11:55Z

https://issues.redhat.com/browse/RHOAIENG-19881

Description

Prompt the user for the OCI Model URI when selecting a OCI connection for model deployment.

If you paste in a URI with <...>://... it will remove the prefix
requires you have a non empty value, other than that the validation is sparse

Existing connection:

New connection:

How Has This Been Tested?

Create an ootb OCI connection deploy it
deploy and create the connection inline
test S3 and URI still work fine

Test Impact

Added jest test for the ConnectionSection.tsx

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
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 or cypress tests 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.

@simrandhaliw

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

emilys314 · 2025-02-26T15:12:24Z

@mturley @Gkrumbach07 @Griffin-Sullivan

emilys314 · 2025-02-26T15:14:24Z

frontend/src/concepts/connectionTypes/utils.ts

+export const isS3Connection = (connection?: Connection): boolean =>
+  !!(
+    connection?.data?.AWS_ACCESS_KEY_ID &&
+    connection.data.AWS_ACCESS_KEY_ID &&
+    connection.data.AWS_S3_ENDPOINT &&
+    connection.data.AWS_S3_BUCKET
+  ) ||
+  !!(
+    connection?.stringData?.AWS_ACCESS_KEY_ID &&
+    connection.stringData.AWS_ACCESS_KEY_ID &&
+    connection.stringData.AWS_S3_ENDPOINT &&
+    connection.stringData.AWS_S3_BUCKET
+  );
+export const isUriConnection = (connection?: Connection): boolean =>
+  !!connection?.data?.URI || !!connection?.stringData?.URI;
+export const isOciConnection = (connection?: Connection): boolean =>
+  !!connection?.data?.OCI_HOST || !!connection?.stringData?.OCI_HOST;


idk if there is a less verbose way to do this.

Otherwise you need to getConnectionModelServingCompatibleTypes(selectedConnectionType)[0] === ModelServingCompatibleTypes.S3ObjectStorage (or includes(...) instead of [0]) everytime

for the URI and OCI connection you're only checking one property, can you do the same for S3 or do we need to check all fields?

I overloaded the isModelServingTypeCompatible() function to work for Connections too so there are less imports / exports needed and it reuses the existing URIConnectionTypeKeys variables with the latest commit

codecov · 2025-02-26T15:51:52Z

Codecov Report

Attention: Patch coverage is 77.77778% with 18 lines in your changes missing coverage. Please review.

Project coverage is 84.66%. Comparing base (a05d232) to head (4203b83).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/InferenceServiceModal/ConnectionOciPathField.tsx	52.63%	9 Missing ⚠️
...d/src/pages/modelServing/screens/projects/utils.ts	77.77%	4 Missing ⚠️
frontend/src/concepts/connectionTypes/utils.ts	81.25%	3 Missing ⚠️
...ojects/InferenceServiceModal/ConnectionSection.tsx	92.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3807      +/-   ##
==========================================
- Coverage   84.67%   84.66%   -0.01%     
==========================================
  Files        1517     1519       +2     
  Lines       35134    35252     +118     
  Branches     9836     9882      +46     
==========================================
+ Hits        29748    29846      +98     
- Misses       5386     5406      +20

Files with missing lines	Coverage Δ
...erenceServiceModal/ConnectionS3FolderPathField.tsx	`100.00% <100.00%> (ø)`
...erenceServiceModal/DataConnectionExistingField.tsx	`0.00% <ø> (ø)`
...ts/InferenceServiceModal/DataConnectionSection.tsx	`0.00% <ø> (ø)`
...erenceServiceModal/ManageInferenceServiceModal.tsx	`97.10% <100.00%> (+0.04%)`	⬆️
...screens/projects/kServeModal/ManageKServeModal.tsx	`95.89% <100.00%> (+0.02%)`	⬆️
...ojects/InferenceServiceModal/ConnectionSection.tsx	`95.67% <92.00%> (-0.33%)`	⬇️
frontend/src/concepts/connectionTypes/utils.ts	`93.46% <81.25%> (-0.96%)`	⬇️
...d/src/pages/modelServing/screens/projects/utils.ts	`93.30% <77.77%> (+0.50%)`	⬆️
...s/InferenceServiceModal/ConnectionOciPathField.tsx	`52.63% <52.63%> (ø)`

... and 11 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a05d232...4203b83. Read the comment docs.

Griffin-Sullivan

/lgtm

Tested on my cluster and works as expected!

Griffin-Sullivan · 2025-02-26T18:13:53Z

frontend/src/concepts/connectionTypes/utils.ts

+export const isS3Connection = (connection?: Connection): boolean =>
+  !!(
+    connection?.data?.AWS_ACCESS_KEY_ID &&
+    connection.data.AWS_ACCESS_KEY_ID &&
+    connection.data.AWS_S3_ENDPOINT &&
+    connection.data.AWS_S3_BUCKET
+  ) ||
+  !!(
+    connection?.stringData?.AWS_ACCESS_KEY_ID &&
+    connection.stringData.AWS_ACCESS_KEY_ID &&
+    connection.stringData.AWS_S3_ENDPOINT &&
+    connection.stringData.AWS_S3_BUCKET
+  );
+export const isUriConnection = (connection?: Connection): boolean =>
+  !!connection?.data?.URI || !!connection?.stringData?.URI;
+export const isOciConnection = (connection?: Connection): boolean =>
+  !!connection?.data?.OCI_HOST || !!connection?.stringData?.OCI_HOST;


for the URI and OCI connection you're only checking one property, can you do the same for S3 or do we need to check all fields?

openshift-ci · 2025-02-26T18:22:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Griffin-Sullivan
Once this PR has been reviewed and has the lgtm label, please ask for approval from emilys314. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

frontend/OWNERS

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

mturley

/lgtm
I'll hold off on approving until others have a chance to look.

mturley · 2025-02-26T18:26:44Z

frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx

+        getConnectionTypeModelServingCompatibleTypes(selectedConnectionType)[0] ===
+          ModelServingCompatibleTypes.S3ObjectStorage && (


Is it maybe worth factoring out a function so you could do something like:

isModelServingConnectionType(selectedConnectionType, ModelServingCompatibleTypes.S3ObjectStorage)

(not sure on the best name here or whether this is worth it)

mturley · 2025-02-26T18:36:25Z

frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx

+              existingUriOption={
+                editInfo?.inferenceServiceEditInfo?.spec.predictor.model?.storageUri
+              }


Ah - @manaswinidas for https://issues.redhat.com/browse/RHOAIENG-19258 this is where we could pass our URI when prefilling from model registry if it is a RH registry URI (starts with oci://registry.redhat.io/). We probably want that logic encapsulated somewhere MR specific but we can maybe lift out an optional existingUriOption to be passed into ManageKServeModal and use it if present instead of this one from editInfo.

openshift-ci · 2025-02-26T20:08:04Z

New changes are detected. LGTM label has been removed.

emilys314 added 3 commits February 26, 2025 09:32

Add "Model URI" field to deployment modal

681d1e3

Fix requiring ACCESS_TYPE for oci deployments

842f467

Update storageUri setting

4203b83

openshift-ci bot requested review from christianvogt and ppadti February 26, 2025 15:12

emilys314 commented Feb 26, 2025

View reviewed changes

emilys314 changed the title ~~Add "Model URI" field to deployment modal~~ feat(serving): add "Model URI" field to deployment modal Feb 26, 2025

Griffin-Sullivan approved these changes Feb 26, 2025

View reviewed changes

openshift-ci bot assigned Griffin-Sullivan Feb 26, 2025

openshift-ci bot added the lgtm label Feb 26, 2025

mturley reviewed Feb 26, 2025

View reviewed changes

openshift-ci bot assigned mturley Feb 26, 2025

openshift-ci bot removed the lgtm label Feb 26, 2025

emilys314 force-pushed the add-oci-deployment-model-uri branch from 7766033 to 4203b83 Compare February 26, 2025 20:34

better isModelServingTypeCompatible()

1db3ef0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(serving): add "Model URI" field to deployment modal #3807

feat(serving): add "Model URI" field to deployment modal #3807

emilys314 commented Feb 26, 2025 •

edited

Loading

emilys314 commented Feb 26, 2025

emilys314 Feb 26, 2025 •

edited

Loading

Griffin-Sullivan Feb 26, 2025

emilys314 Feb 26, 2025

codecov bot commented Feb 26, 2025 •

edited

Loading

Griffin-Sullivan left a comment

Griffin-Sullivan Feb 26, 2025

openshift-ci bot commented Feb 26, 2025

mturley left a comment

mturley Feb 26, 2025

mturley Feb 26, 2025

openshift-ci bot commented Feb 26, 2025

		getConnectionTypeModelServingCompatibleTypes(selectedConnectionType)[0] ===
		ModelServingCompatibleTypes.S3ObjectStorage && (

feat(serving): add "Model URI" field to deployment modal #3807

Are you sure you want to change the base?

feat(serving): add "Model URI" field to deployment modal #3807

Conversation

emilys314 commented Feb 26, 2025 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

emilys314 commented Feb 26, 2025

emilys314 Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Griffin-Sullivan Feb 26, 2025

Choose a reason for hiding this comment

emilys314 Feb 26, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2025 • edited Loading

Codecov Report

Griffin-Sullivan left a comment

Choose a reason for hiding this comment

Griffin-Sullivan Feb 26, 2025

Choose a reason for hiding this comment

openshift-ci bot commented Feb 26, 2025

mturley left a comment

Choose a reason for hiding this comment

mturley Feb 26, 2025

Choose a reason for hiding this comment

mturley Feb 26, 2025

Choose a reason for hiding this comment

openshift-ci bot commented Feb 26, 2025

emilys314 commented Feb 26, 2025 •

edited

Loading

emilys314 Feb 26, 2025 •

edited

Loading

codecov bot commented Feb 26, 2025 •

edited

Loading