Prefill URI connections and fix prefill deploy model form from version #3590

manaswinidas · 2024-12-18T16:17:12Z

Closes: RHOAIENG-16146
Closes: RHOAIENG-14136

Description

Prefilling URI connections from Register model page(the dropdown should only contain connections filtered according to type):

Screen.Recording.2025-02-11.at.9.51.21.PM.mov

A few scenarios for prefilling connections:

If you have a registered model version registered with object storage and you have a matching connection, Existing Connection will be selected and prefilled with the matching connection and path. The connection selection field will only populate with the connection when there is only 1 recommended connection.(You can test this by deploying the version inside md-test-s3 model on the mdas project - it has one matching connection and that should be pre-selected and you should see Recommended label beside it when you click on the dropdown to see more connections on the psi-08 cluster)

Screen.Recording.2025-02-11.at.9.52.56.PM.mov

If you have more than one recommended(matching) connection, the Existing Connection will be selected, but not be pre-filled - you can click on the dropdown and see two connections with Recommended labels.

If you have a registered model version registered with URI and you have a matching connection, Existing connection will be selected and prefilled with connection name. You should have only 1 matching URI connection to see this(You can test this by deploying the version inside test-mr-md model on the mdas project - it has one matching URI connection on the psi-08 cluster). If you have multiple matching(Recommended) connections, Existing Connection is selected with no prefilled connections(Refer to point 2 screenshot)

Screen.Recording.2025-02-14.at.12.15.44.PM.mov

In case you deploy a model version on a project that has no matching connections, you should see an alert with New Connection pre-selected and prefilled with a few fields including bucket, region, endpoint and path.(Video 1)(You can test this by deploying the version inside md-test-s3 model on the mdas-new project - it has no matching connections - you should see the prefilled connection type and s3/object storage fields with alert on the psi-08 cluster). Same goes for URI field. (Video 2)(You can test this by deploying the version inside test-mr-md model on the mdas-new project - it has no matching connections - you should see the prefilled connection type and URI field with alert on the psi-08 cluster)
Video 1:

Screen.Recording.2025-02-12.at.12.33.29.AM.mov

Video 2:

Screen.Recording.2025-02-12.at.12.36.54.AM.mov

Unit tests and prefilling logic co-written with @ppadti

How Has This Been Tested?

Instructions added along with screen recordings in description.

Test Impact

Added unit tests and fixing existing and adding new Cypress tests

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.

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

mturley

A couple things I noted from our meeting @manaswinidas @alexcreasy and some resulting thoughts.

mturley · 2025-02-11T16:14:53Z

frontend/src/concepts/modelRegistry/utils.ts

+    if (uri.startsWith('https:')) {
+      return { s3Fields: null, uri };


URI should be the catch-all case since it could use any protocol (such as the upcoming oci://). We should flip this logic and have all the S3-specific logic here only happen if the URI starts with s3:, else return this URI version.

Initially I was going to say we should try to keep this function entirely only related to S3 and only call it in the S3 case (so as not to propagate this "URI-split-to-separate-fields" logic that @andrewballantyne has stated we should avoid propagating), but that would lead to needing a isS3URI() check that we call pretty much everywhere we use a URI in order to determine if we need this function. I think it maybe makes sense to keep this pattern where we pass around this object of "URI and anything we need to know from parsing it", even if in the future we're only adding isOCI to that or something.

I do think however that we should factor out the { s3Fields: ObjectStorageFields | null; uri: string | null } into a separate type since we repeat it. Is there ever a place anymore where we use ObjectStorageFields separate from this whole object? Maybe instead we can have a StorageFields type and the s3Fields can be nested in it. In fact, instead of StorageFields can we maybe call it RegisteredModelLocation? It's essentially an object representation of the location of a registered model (well, the location of an artifact of one of its versions, but RegisteredModelVersionLocation is getting long - and I do think we should maybe have "Registered" in the name if we're going to pass it around in model serving code, to distinguish it from how model locations are stored in connections there).

Please feel free to ignore everything here except the first paragraph if it becomes too much trouble for this PR.

mturley · 2025-02-11T16:20:19Z

frontend/src/pages/modelServing/screens/types.ts

@@ -91,6 +93,7 @@ export type InferenceServiceStorage = {
  type: InferenceServiceStorageType;
  path: string;
  dataConnection: string;
+  connectionType?: ModelLocationType | string;


As discussed on the call with @alexcreasy - If possible, we probably want to avoid adding model-registry-specific properties to these model serving types. isRecommended / LabeledConnection is sort of an exception because it's a generic feature we added to model serving - anything can make a connection recommended.

I know this complicates things though - I want to think through it a little but we could decide to just address it outside this PR. If we do, let's put a comment here and anywhere this is used saying it's temporary.

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

mturley · 2025-02-11T16:50:28Z

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

+  React.useEffect(() => {
+    const locationType = data.storage.connectionType;
+    if (locationType) {
+      if (locationType === 's3') {
+        setSelectedConnectionType(
+          withRequiredFields(
+            connectionTypes.find((t) => getResourceNameFromK8sResource(t) === locationType),
+            S3ConnectionTypeKeys,
+          ),
+        );
+        setConnectionValues(getMRConnectionValues(data.storage.awsData));
+      }
+      if (data.storage.uri) {
+        setSelectedConnectionType(
+          withRequiredFields(
+            connectionTypes.find(
+              (t) => getResourceNameFromK8sResource(t) === data.storage.connectionType,
+            ),
+            ['URI'],
+          ),
+        );
+        setConnectionValues(getMRConnectionValues(data.storage.uri));
+      }
+    }
+  }, [data.storage.connectionType, connectionTypes, data.storage.uri, data.storage.awsData]);


Ok, so recapping part of our discussion with @alexcreasy - this useEffect exists to update state in response to a user action, which should ideally not be done in a useEffect (it should be part of whatever callback was triggered by the user action) to avoid unintended state updates. Let me try to think out loud about what we could maybe do:

In our case, we.... already broke that rule with usePrefillDeployModalFromModelRegistry.... for.... reasons. (short version: we'd otherwise have to lift all the form state out of the whole ManageInferenceServiceModal so we could change its defaults for our MR-specific rendering of it... which was untenable due to the already-existing mess of useEffects we didn't want to touch). But if possible we should keep our useEffect crimes encapsulated in that one existing hook.

So, what's stopping us from doing that? There's no way to call setSelectedConnectionType and setConnectionValues from outside this ConnectionSection. The traditional way to address that would be to "lift the state up" - take the useStates for these and bring them up into ManageInferenceServiceModal. That's.... not really practical here because we'd also have to lift all the other enabledConnectionTypes logic from NewConnectionField, etc.

So what's another way we could address this? Well, we could introduce props/logic that is not specific to model registry that allow any consumer to optionally pre-fill default values for selectedConnectionType and connectionValues. If NewConnectionField had props initialConnectionTypeName and initialConnectionValues, those could be conditionally used within the React.useState() call for setting the initial state values there (using the connection type name as the prop because this is where connection types are fetched and we can look up the matching object here). Then we could drill those new props down: ConnectionSection could have props like initialNewConnectionTypeName and initialNewConnectionValues (naming gets weird here maybe), and then those could be passed in where we render ConnectionSection in ManageInferenceServiceModal. Now we're back to where we have centralized MR-specific logic - we could have usePrefillDeployModalFromModelRegistry conditionally return these initialNewConnectionType and initialNewConnectionValues if you are prefilling from MR.

That's just one idea -- and if that causes too much trouble to resolve in this PR we can come back to it. But I do think we should make sure to not leave this tech debt for long.

manaswinidas · 2025-02-11T19:20:00Z

@yih-wang Can you check the above screenshots/screen recordings?

My doubts:

Can you confirm the behavior in point 3 of the PR description - when will we select Existing URI and Existing storage? Currently I'm selecting Current URI when there is one matching URI connection and Existing connection when there are multiple matching connections.
Is the positioning of the alert okay in this screenshot? Should the alert appear between the radio button and the Connection type?

frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts

yih-wang · 2025-02-12T04:18:59Z

Can you confirm the behavior in point 3 of the PR description - when will we select Existing URI and Existing storage? Currently I'm selecting Current URI when there is one matching URI connection and Existing connection when there are multiple matching connections.

@manaswinidas So regarding the URI case, I don't think we should add the Current URI option to the deploy modal, unless I missed any decision. @vconzola @kywalker-rh Can you help confirm if there was any decision about adding a Current URI option came up during the model deployment / ModelCar-specific deployment conversations?

IMO it should behave exactly the same way with S3 connection, i.e.

If there's a matching connection for the URI this model contains, we should auto-select Existing connection and pre-fill the matching URI connection for the user;
If there're multiple matching connections for the URI, we should auto-select Existing connection but not pre-fill any connection for the user;
If there's not a matching connection, we should switch to New connection and prefill the URI address for the user. (work well in 4 - video2)

Is the positioning of the alert okay in this screenshot? Should the alert appear between the radio button and the Connection type?

The alert position looks good to me.

yih-wang · 2025-02-12T04:25:34Z

Overall the videos look really good, thank you @manaswinidas!

Got a few nitpicks on some minor details:

There're several places in the registration form > autofill from connection modal where we should update data connection to connection (to align with the new terminology). Also, the URI type cannot contain a bucket so the help text should be adjusted for the second field.
This alert will not be accurate when we reuse this deployment modal in the model catalog. It says Model location info is available in the registry... Do we want to simply omit the part in the registry and fudge where the flow starts, or is it better to conditionally change the alert to Model location info is available in the catalog... when user trigger deployment from the MC? @kaedward @mturley Any preference?

emilys314 · 2025-02-12T21:27:05Z

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

+  const [connections, connectionsLoaded, connectionsLoadError] =
+    usePrefillDeployModalFromModelRegistry(
+      projectContext,
+      createDataInferenceService,
+      setCreateDataInferenceService,
+      registeredModelDeployInfo,
+    );
+


Just an observation, but this seems confusing because traditionally connections would be of type Connection or an array of it. But having it a LabeledConnection seems confusing.

I don't know if there is a better solution though to add extra properties like this though

I followed what was done in #3074

It may be worth opening a tech debt issue to see if we want to refactor to a different approach, but yeah the LabeledConnection was the existing approach that was lost when connection types were introduced. An alternative could be to maybe return a separate list of recommendedConnections, and when we render the connections dropdown check each to see if it's present in there to mark it recommended. But I'm not sure that's much better.

manaswinidas · 2025-02-13T10:36:09Z

@yih-wang Changed the Autofill from connection content for both URI and object storage types.

mturley · 2025-02-13T16:43:04Z

@yih-wang

This alert will not be accurate when we reuse this deployment modal in the model catalog. It says Model location info is available in the registry... Do we want to simply omit the part in the registry and fudge where the flow starts, or is it better to conditionally change the alert to Model location info is available in the catalog... when user trigger deployment from the MC?

I'll defer to @kaedward 's opinion, but omitting that in the registry part and saying Model location info is available but no matching connection in the project seems fine to me. If that's not acceptable, I don't think it would be too difficult to have this text be conditional, but for this PR I think we can let it stay as-is and implement that conditional text in https://issues.redhat.com/browse/RHOAIENG-19257.

Incidentally though, I think we may want to correct the grammar there - "...but no matching connection in the project" should maybe say "...but there are no matching connections in the project" or "...but there is no matching connection in the project"?

mturley · 2025-02-13T16:48:24Z

@manaswinidas thank you for outlining the test cases and including the videos. That helps a lot to understand the complex changes.

yih-wang · 2025-02-14T04:01:38Z

Incidentally though, I think we may want to correct the grammar there

@mturley hmmm, so... I checked the content handoff doc and found there're several microcopies (in colorful rows) we forgot to refresh (possibly because the deployment pages are not available yet when the content is refreshing). I'm fine with not updating any content before summit and opening a separate ticket for tracking these changes. I'll defer to you...

manaswinidas · 2025-02-14T10:16:57Z

@yih-wang I've updated the screen recording for point 3 and added a few other UX fixes to ensure uniformity.
The Deploy button is disabled while the project is being selected for deploying the model version(you can see this in video 3).

Co-authored by: ppadti <[email protected]>

openshift-ci · 2025-02-14T14:26:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign manosnoam for approval. 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

codecov · 2025-02-14T17:02:55Z

Codecov Report

Attention: Patch coverage is 93.43434% with 13 lines in your changes missing coverage. Please review.

Project coverage is 84.46%. Comparing base (7fbab31) to head (609b63b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...end/src/pages/modelServing/ModelServingContext.tsx	44.44%	5 Missing ⚠️
...s/RegisterModel/RegistrationCommonFormSections.tsx	81.25%	3 Missing ⚠️
...ojects/InferenceServiceModal/ConnectionSection.tsx	93.18%	3 Missing ⚠️
...istry/screens/RegisterModel/ConnectionDropdown.tsx	90.90%	1 Missing ⚠️
...Registry/screens/RegisterModel/ConnectionModal.tsx	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3590      +/-   ##
==========================================
+ Coverage   84.34%   84.46%   +0.11%     
==========================================
  Files        1487     1487              
  Lines       34078    34185     +107     
  Branches     9440     9486      +46     
==========================================
+ Hits        28744    28873     +129     
+ Misses       5334     5312      -22

Files with missing lines	Coverage Δ
frontend/src/components/TypeaheadSelect.tsx	`67.58% <100.00%> (ø)`
frontend/src/concepts/connectionTypes/utils.ts	`94.34% <100.00%> (+1.11%)`	⬆️
frontend/src/concepts/modelRegistry/types.ts	`100.00% <ø> (ø)`
frontend/src/concepts/modelRegistry/utils.ts	`100.00% <100.00%> (ø)`
...ns/ModelVersionDetails/ModelVersionDetailsView.tsx	`75.92% <100.00%> (ø)`
.../screens/RegisterModel/RegisteredModelSelector.tsx	`100.00% <ø> (ø)`
...s/RegisterModel/usePrefillRegisterVersionFields.ts	`100.00% <100.00%> (ø)`
...pages/modelRegistry/screens/RegisterModel/utils.ts	`82.00% <ø> (ø)`
.../screens/RegisteredModels/useLabeledConnections.ts	`100.00% <100.00%> (ø)`
...edModels/usePrefillDeployModalFromModelRegistry.ts	`97.05% <100.00%> (+49.05%)`	⬆️
... and 14 more

... and 9 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 7fbab31...609b63b. Read the comment docs.

mturley · 2025-02-14T17:25:02Z

@mturley hmmm, so... I checked the content handoff doc and found there're several microcopies (in colorful rows) we forgot to refresh (possibly because the deployment pages are not available yet when the content is refreshing). I'm fine with not updating any content before summit and opening a separate ticket for tracking these changes. I'll defer to you...

oh no! agh, I wonder if this was me dropping the ball... we should probably open a ticket and not block summit, but it might be nice to try to get some of it done if we have time. I should have made sure these were covered in our "Post-MVP usability improvements" strat.

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Dec 18, 2024

openshift-ci bot requested review from DaoDaoNoCode and dpanshug December 18, 2024 16:17

manaswinidas force-pushed the prefill-uri-connections branch from 9015414 to 2516700 Compare January 22, 2025 17:33

manaswinidas mentioned this pull request Jan 30, 2025

Begin data connection clean up #3660

Merged

6 tasks

manaswinidas force-pushed the prefill-uri-connections branch 2 times, most recently from 6e27303 to 754e35f Compare February 11, 2025 14:59

mturley reviewed Feb 11, 2025

View reviewed changes

frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts Outdated Show resolved Hide resolved

mturley reviewed Feb 11, 2025

View reviewed changes

frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts Outdated Show resolved Hide resolved

emilys314 reviewed Feb 12, 2025

View reviewed changes

manaswinidas added 11 commits February 14, 2025 19:52

Add autofill modal and button for URI connections

285fd47

Porting DataConnection to Connection

f493cbb

Merge with 'main'

e67488e

Fix prefill for S3 connections and merge with main

5e24304

Fix prefill for S3 connections

6228343

Fix for prefilling URI connections

ad5fa65

Add alert to prefilled modal and some name changes

c654bf8

Fix conflicts

6fc8878

Add unit tests and UX fixes

18cc9f7

Fix unit tests

c8ccd56

Microcopy fixes

aba47ab

Fix Cypress tests

6c6a22d

Co-authored by: ppadti <[email protected]>

manaswinidas force-pushed the prefill-uri-connections branch from c2ad16f to 6c6a22d Compare February 14, 2025 14:26

manaswinidas changed the title ~~[WIP] Prefill URI connections and fix prefill deploy model form from version~~ Prefill URI connections and fix prefill deploy model form from version Feb 14, 2025

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Feb 14, 2025

manaswinidas added 2 commits February 14, 2025 21:31

Fix more Cypress tests

ecad4a6

Remove extra params in tests

609b63b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefill URI connections and fix prefill deploy model form from version #3590

Prefill URI connections and fix prefill deploy model form from version #3590

manaswinidas commented Dec 18, 2024 •

edited

Loading

mturley left a comment

mturley Feb 11, 2025

mturley Feb 11, 2025

mturley Feb 11, 2025

manaswinidas commented Feb 11, 2025

yih-wang commented Feb 12, 2025

yih-wang commented Feb 12, 2025

emilys314 Feb 12, 2025

manaswinidas Feb 13, 2025

mturley Feb 13, 2025

manaswinidas commented Feb 13, 2025

mturley commented Feb 13, 2025 •

edited

Loading

mturley commented Feb 13, 2025

yih-wang commented Feb 14, 2025

manaswinidas commented Feb 14, 2025

openshift-ci bot commented Feb 14, 2025

codecov bot commented Feb 14, 2025

mturley commented Feb 14, 2025 •

edited

Loading

		if (uri.startsWith('https:')) {
		return { s3Fields: null, uri };

Prefill URI connections and fix prefill deploy model form from version #3590

Are you sure you want to change the base?

Prefill URI connections and fix prefill deploy model form from version #3590

Conversation

manaswinidas commented Dec 18, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

mturley left a comment

Choose a reason for hiding this comment

mturley Feb 11, 2025

Choose a reason for hiding this comment

mturley Feb 11, 2025

Choose a reason for hiding this comment

mturley Feb 11, 2025

Choose a reason for hiding this comment

manaswinidas commented Feb 11, 2025

yih-wang commented Feb 12, 2025

yih-wang commented Feb 12, 2025

emilys314 Feb 12, 2025

Choose a reason for hiding this comment

manaswinidas Feb 13, 2025

Choose a reason for hiding this comment

mturley Feb 13, 2025

Choose a reason for hiding this comment

manaswinidas commented Feb 13, 2025

mturley commented Feb 13, 2025 • edited Loading

mturley commented Feb 13, 2025

yih-wang commented Feb 14, 2025

manaswinidas commented Feb 14, 2025

openshift-ci bot commented Feb 14, 2025

codecov bot commented Feb 14, 2025

Codecov Report

mturley commented Feb 14, 2025 • edited Loading

manaswinidas commented Dec 18, 2024 •

edited

Loading

mturley commented Feb 13, 2025 •

edited

Loading

mturley commented Feb 14, 2025 •

edited

Loading