Fix editing isvc to maintain extraneous UI properties #4582
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/api/k8s/inferenceServices.ts (1)
150-152: LGTM! Excellent fix for preserving existing properties during updates.The spread operator usage correctly preserves all existing
specandpredictorproperties while allowing new properties to override as needed. This directly addresses the PR objective of maintaining extraneous UI properties during ISVC edits.Consider adding optional chaining for additional safety:
spec: { - ...inferenceService.spec, + ...inferenceService.spec, predictor: { - ...inferenceService.spec.predictor, + ...inferenceService.spec?.predictor,This would prevent potential runtime errors if the existing InferenceService has an unexpected structure, though the current implementation should work fine in normal scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/api/k8s/inferenceServices.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint
- GitHub Check: Cypress-Setup
|
Changing the serving runtime stuff to support this too would be great I think |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4582 +/- ##
===========================================
- Coverage 82.30% 29.59% -52.72%
===========================================
Files 1880 1920 +40
Lines 39129 39394 +265
Branches 11711 12117 +406
===========================================
- Hits 32207 11658 -20549
- Misses 6922 27736 +20814
... and 1573 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Griffin-Sullivan
force-pushed
the
edit-keep-props
branch
from
db42581 to
8a356f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/api/k8s/inferenceServices.ts(2 hunks)frontend/src/api/k8s/servingRuntimes.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/api/k8s/inferenceServices.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress-Setup
- GitHub Check: Lint
Griffin-Sullivan
force-pushed
the
edit-keep-props
branch
from
8a356f6 to
096d70c
Compare
|
To fix your lint issue you can try increasing the memory node has https://redhat-internal.slack.com/archives/C05SMJ09DD2/p1753989489311749 |
There was a problem hiding this comment.
So yeah I think there has to be a bit more nuance to this, which can make it tricky.
If i switch the model source from S3 to OCI then the S3 stuff remains
storage:
key: with-whitespace
path: test
storageUri: 'oci://quay.io/rh-ee-esamoylo/private:test'
There was a problem hiding this comment.
I was looking at the InferenceService spec here https://kserve.github.io/website/0.8/reference/api/#serving.kserve.io/v1beta1.ModelSpec
and spec.predictor.model also extends PredictorExtensionSpec which extends Container v1 core which will include a bunch of container specs. So we may want to also ...inferenceService.spec.predictor.model for it too.
This includes args, command, env, envFrom, image, imagePullPolicy, lifecycle ports, etc.
I think after a _.merge, we could manually clear storage: ... if there is a storageUri and also for removing the imagePullSecret
There was a problem hiding this comment.
Been messing around with this I think we would also need to remove args or env variables if they change
There was a problem hiding this comment.
Yeah it seems to be kinda funky doing it this way
There was a problem hiding this comment.
Do the args and env stuff get fully pulled into the UI field for it?
If so we can overwrite those 2 safely, because the UI controls it fully
Griffin-Sullivan
force-pushed
the
edit-keep-props
branch
2 times, most recently
from
385e0d0 to
dca0035
Compare
| if (!isModelMesh) { | ||
| predictor.minReplicas = minReplicas; | ||
| predictor.maxReplicas = maxReplicas; | ||
| predictor.imagePullSecrets = imagePullSecrets; |
There was a problem hiding this comment.
It may be possible if there are extraneous pull secrets added that are not the oci image pull secret.
But that is tricky to determine
|
/test-e2e@Modelserving |
|
E2E Tests #2473 triggered by #4582 (comment) |
|
E2E Tests #2473 finished with result: FAILURE |
|
/test-e2e@Modelserving |
|
E2E Tests #2474 triggered by #4582 (comment) |
|
E2E Tests #2474 finished with result: FAILURE |
|
First a 503 now an SSL error when trying to get the RHEL 9 repo??? |
|
/test-e2e@Modelserving |
|
E2E Tests #2475 triggered by #4582 (comment) |
|
E2E Tests #2475 finished with result: FAILURE |
| updatedInferenceService.spec.predictor.model = { | ||
| ...updatedInferenceService.spec.predictor.model, | ||
| resources, |
There was a problem hiding this comment.
The resources doesn't preserve the claims field, everything else is working though
There was a problem hiding this comment.
Also I think adding some unit tests for the new functionality should be fairly straightforward and valuable
| delete model.storage; | ||
| } else { | ||
| delete model.storageUri; |
There was a problem hiding this comment.
hmmm I don't think this is working actually...
There was a problem hiding this comment.
Nevermind this is because the ConnectionSection had a bug where selecting a new existing connection didn't clear the URI data
Griffin-Sullivan
force-pushed
the
edit-keep-props
branch
2 times, most recently
from
dccc131 to
c60eaa3
Compare
| inferenceServiceModalEdit.findConnectionFieldInput('URI').blur(); | ||
| inferenceServiceModalEdit.findSubmitButton().should('be.disabled'); |
There was a problem hiding this comment.
I'm not sure why I hit this? Did I mess something up with validation? You have to blur the field for the submit button to disable or else it will be enabled
There was a problem hiding this comment.
Screencast.From.2025-08-13.11-46-42.mp4
There was a problem hiding this comment.
I don't think it was me cause this is running main
There was a problem hiding this comment.
Same behavior in my PR too so not sure what changed but from user perspective it's the same and I updated the test to be more reliable
Griffin-Sullivan
force-pushed
the
edit-keep-props
branch
from
c60eaa3 to
fee7849
Compare
|
Looks good to me! I tested it by running locally, it preserved all the properties i changed, and then compared it to the old version which then swiftly removed those properties. Additionally the changing of connections seems to be working all smoothly still Custom values start: After saving with the new code: After saving with the old code: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: emilys314 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 |
openshift-merge-bot
bot
merged commit c73ccc3
into
opendatahub-io:main
https://issues.redhat.com/browse/RHOAIENG-30257
Description
Adding a fix for maintaining non-dashboard properties on an ISVC and SR when editing
How Has This Been Tested?
Create a project via OC
Create a ServingRuntime & InferenceService with as much bare bones as possible
Add a custom annotation & label (doesn't matter what)
Include dashboard label (related verification is RHOAIENG-26146)
Deploy resources into the project via OC
Go to the UI & edit the resource
Make no change if possible, but maybe change replicas if needed
Verify the resource still mains your custom annotation & label
Repeat for a property in the spec. See https://issues.redhat.com/browse/RHOAIENG-18976
Test Impact
We should probably add a unit test for this but I think how we assemble inference services will change soon
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit