Add connection type to model serving create modal #3356

emilys314 · 2024-10-21T17:18:22Z

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

Description

Replaces the data connections section in the model serving modals with connection types. The existing connection has fairly similar behavior. The new connection options has the ability to create a new connection inline,

Updated the single kserving modal and the modelmesh multi model modals.

How Has This Been Tested?

Set disableConnectionTypes dev flag to false
either navigate to a project and open the "Models" details section OR go to /modelServing which is the "Model Serving" on the left nav bar
Make sure you have a runtime setup
- (either single serving or multi. you can have 2 projects with each having one of them)
Edit or deploy a new model
- Try various combinations of connections types and connection instances on the project

Test Impact

Jest tests added

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

openshift-ci · 2024-10-21T17:18:27Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2024-10-21T18:01:52Z

Codecov Report

Attention: Patch coverage is 72.98851% with 47 lines in your changes missing coverage. Please review.

Project coverage is 84.93%. Comparing base (c96cc39) to head (aaad986).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ojects/InferenceServiceModal/ConnectionSection.tsx	69.79%	29 Missing ⚠️
frontend/src/utilities/useWatchConnectionTypes.tsx	69.23%	4 Missing ⚠️
...ts/connectionTypes/ConnectionDetailsHelperText.tsx	76.92%	3 Missing ⚠️
frontend/src/concepts/connectionTypes/utils.ts	75.00%	3 Missing ⚠️
...d/src/pages/modelServing/screens/projects/utils.ts	76.92%	3 Missing ⚠️
...erenceServiceModal/ManageInferenceServiceModal.tsx	75.00%	2 Missing ⚠️
...screens/projects/kServeModal/ManageKServeModal.tsx	75.00%	2 Missing ⚠️
...jects/screens/detail/connections/useConnections.ts	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3356      +/-   ##
==========================================
- Coverage   84.96%   84.93%   -0.03%     
==========================================
  Files        1333     1335       +2     
  Lines       29951    30104     +153     
  Branches     8201     8253      +52     
==========================================
+ Hits        25448    25570     +122     
- Misses       4503     4534      +31

Files with missing lines	Coverage Δ
...eens/detail/connections/ManageConnectionsModal.tsx	`93.54% <100.00%> (ø)`
...jects/screens/detail/connections/useConnections.ts	`85.71% <90.00%> (-1.79%)`	⬇️
...erenceServiceModal/ManageInferenceServiceModal.tsx	`96.05% <75.00%> (-2.50%)`	⬇️
...screens/projects/kServeModal/ManageKServeModal.tsx	`95.10% <75.00%> (-1.22%)`	⬇️
...ts/connectionTypes/ConnectionDetailsHelperText.tsx	`76.92% <76.92%> (ø)`
frontend/src/concepts/connectionTypes/utils.ts	`89.26% <75.00%> (-1.39%)`	⬇️
...d/src/pages/modelServing/screens/projects/utils.ts	`95.96% <76.92%> (-1.31%)`	⬇️
frontend/src/utilities/useWatchConnectionTypes.tsx	`69.23% <69.23%> (-30.77%)`	⬇️
...ojects/InferenceServiceModal/ConnectionSection.tsx	`69.79% <69.79%> (ø)`

... and 8 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 c96cc39...aaad986. Read the comment docs.

christianvogt · 2024-10-24T20:23:20Z

frontend/src/utilities/useWatchConnectionTypes.tsx

+    const secrets = await fetchConnectionTypes();
+
+    let connectionTypes = secrets;


Suggested change

const secrets = await fetchConnectionTypes();

let connectionTypes = secrets;

let connectionTypes = await fetchConnectionTypes();

christianvogt · 2024-10-24T20:25:04Z

frontend/src/utilities/useWatchConnectionTypes.tsx

+    if (modelServingCompatible) {
+      connectionTypes = connectionTypes.filter((type) => {
+        const compatibleTypes = getCompatibleTypes(
+          type.data?.fields?.filter(isConnectionTypeDataField).map((field) => field.envVar) ?? [],
+        );
+
+        return compatibleTypes.includes(CompatibleTypes.ModelServing);


With the changes UX wants to do to the Compatibility column, I'm going to rework how these checks are made...

okay. yeah I was looking at this for connections as well, it's a little tricky to accommodate the different data structures the env variables can be represented as. And for connections the values need to be checked to be truthy as well

christianvogt · 2024-10-24T20:30:07Z

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

@@ -110,6 +113,10 @@ const ManageKServeModal: React.FC<ManageKServeModalProps> = ({
      registeredModelDeployInfo,
    );

+  const isConnectionTypesEnabled = useConnectionTypesEnabled();
+  const [connection, setConnection] = React.useState<Connection>();
+  const [isConnectionValid, setIsConnectionvalid] = React.useState(false);


Suggested change

const [isConnectionValid, setIsConnectionvalid] = React.useState(false);

const [isConnectionValid, setIsConnectionValid] = React.useState(false);

christianvogt · 2024-10-24T21:02:53Z

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

+  data: CreatingInferenceServiceObject;
+  setData: UpdateObjectAtPropAndValue<CreatingInferenceServiceObject>;
+  setConnection: (connection?: Connection) => void;
+  setIsConnectionvalid: (isValid: boolean) => void;


Suggested change

setIsConnectionvalid: (isValid: boolean) => void;

setIsConnectionValid: (isValid: boolean) => void;

christianvogt · 2024-10-24T21:04:41Z

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

+          setSelectedConnectionType(
+            withRequiredFields(
+              connectionTypes.find((t) => getResourceNameFromK8sResource(t) === type),
+              ['AWS_ACCESS_KEY_ID', 'AWS_SECRET_ACCESS_KEY', 'AWS_S3_ENDPOINT', 'AWS_S3_BUCKET'],


Should be a const set of env vars that is shared with isModelServingCompatibility.

christianvogt · 2024-10-24T21:06:06Z

frontend/src/concepts/connectionTypes/utils.ts

+): ConnectionTypeConfigMapObj | undefined => {
+  for (const field of connectionType?.data?.fields ?? []) {
+    if (isConnectionTypeDataField(field) && envVars?.includes(field.envVar)) {
+      field.required = true;


This should not mutate the original value.

christianvogt · 2024-10-24T21:07:00Z

frontend/src/concepts/connectionTypes/utils.ts

 export const assembleConnectionSecret = (
-  project: ProjectKind,
+  project: ProjectKind | string,


Change to projectName: string and all callers.

christianvogt · 2024-10-25T19:58:32Z

/lgtm
/approve

openshift-ci · 2024-10-25T19:58:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt

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:

~~frontend/OWNERS~~ [christianvogt]

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

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

emilys314 marked this pull request as ready for review October 22, 2024 19:12

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Oct 22, 2024

openshift-ci bot requested review from lucferbux and ppadti October 22, 2024 19:12

emilys314 force-pushed the add-connections-to-model-serving branch 4 times, most recently from 2409ca4 to 2162712 Compare October 23, 2024 14:58

christianvogt reviewed Oct 24, 2024

View reviewed changes

emilys314 force-pushed the add-connections-to-model-serving branch 4 times, most recently from b5a1d16 to 27d60f3 Compare October 25, 2024 17:16

emilys314 added 5 commits October 25, 2024 13:24

Add connection type to model serving create modal

ad22f28

Jest tests

27488be

Fixes

5a9548d

Show types with missing required fields, but set them required

701cfcb

Add connection type to model serving create modal

aaad986

emilys314 force-pushed the add-connections-to-model-serving branch from 27d60f3 to aaad986 Compare October 25, 2024 17:24

openshift-ci bot assigned christianvogt Oct 25, 2024

openshift-ci bot added the lgtm label Oct 25, 2024

openshift-ci bot added the approved label Oct 25, 2024

openshift-merge-bot bot merged commit 05e394b into opendatahub-io:main Oct 25, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connection type to model serving create modal #3356

Add connection type to model serving create modal #3356

emilys314 commented Oct 21, 2024 •

edited

Loading

openshift-ci bot commented Oct 21, 2024

codecov bot commented Oct 21, 2024 •

edited

Loading

christianvogt Oct 24, 2024

christianvogt Oct 24, 2024

emilys314 Oct 25, 2024

christianvogt Oct 24, 2024

christianvogt Oct 24, 2024

christianvogt Oct 24, 2024

christianvogt Oct 24, 2024

christianvogt Oct 24, 2024

christianvogt commented Oct 25, 2024

openshift-ci bot commented Oct 25, 2024

		const secrets = await fetchConnectionTypes();

		let connectionTypes = secrets;

	const [isConnectionValid, setIsConnectionvalid] = React.useState(false);
	const [isConnectionValid, setIsConnectionValid] = React.useState(false);

	setIsConnectionvalid: (isValid: boolean) => void;
	setIsConnectionValid: (isValid: boolean) => void;

Add connection type to model serving create modal #3356

Add connection type to model serving create modal #3356

Conversation

emilys314 commented Oct 21, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Oct 21, 2024

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Oct 25, 2024

openshift-ci bot commented Oct 25, 2024

emilys314 commented Oct 21, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading