Skip to content

Deploy a Model from a PVC #4449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

katieperry4
Copy link
Contributor

@katieperry4 katieperry4 commented Jul 3, 2025

Closes RHOAIENG-28016

Description

Added to the kserve deploy modal so you can deploy from a pvc
Screenshot 2025-07-03 at 10 55 07 AM
Screenshot 2025-07-03 at 10 55 11 AM
Screenshot 2025-07-03 at 10 55 17 AM
Screenshot 2025-07-03 at 10 55 29 AM
(If only one PVC present)
Screenshot 2025-07-07 at 10 53 19 AM
(Radio doesn't show at all if there are no existing PVCs, similar to when there are no existing connections)

Loads pvcs from project namespace into the dropdown and shows the modal name based on the annotations in the pvc (if present, if there's a path annotation but no name annotation, it lists it as unknown model)
Input trimming and input validation (no leading slash for path, no whitespace)
Generates a uri: 'pvc://model-pvc/fraud'

How Has This Been Tested?

tested locally and npm run test

To test:

  • Kind of a pain
  • Remember to use the devFeatureFlags to enable PVC serving
  • DOCS

oc login

oc project katie-test-single (your project though you'll have to replace the namespace in the yaml below but otherwise should be copy/pasteable)

I put all the yaml in a folder on my desktop

PVC yaml -> oc apply or import yaml
oc apply -f ~/Desktop/PVC-YAML/pvc.yml

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: model-pvc
  namespace: katie-test-single
spec:
  accessModes:
    - ReadWriteOnce
  storageClassName: gp3-csi
  resources:
    requests:
      storage: 5Gi

once that’s made go in and add this to the pvc in ocp

  labels:
    opendatahub.io/dashboard: 'true'

and if you want to add the model name and model path annotations you can do that too:

    dashboard.opendatahub.io/model-name: fraud-model
    dashboard.opendatahub.io/model-path: fraud

(If you use the model I linked below and unzip it so it's just a folder called fraud on your desktop, the path will just be fraud you can add nested folders to mess with the path ex: dashboard.opendatahub.io/model-path: fraud2/layer-folder/test-folder but do that before you copy it into the pvc with the pod)

we need to mount the PVC on a pod temporarily
Pod yaml oc apply -f ~/Desktop/PVC-YAML/pod.yml

apiVersion: v1
kind: Pod
metadata:
  name: model-store-pod
  namespace: katie-test-single
spec:
  securityContext:
    runAsNonRoot: true
    seccompProfile:
      type: RuntimeDefault
  volumes:
    - name: model-store
      persistentVolumeClaim:
        claimName: model-pvc # <-- this PVC must already exist
  containers:
    - name: model-store
      image: ubuntu
      command: ["sleep"]
      args: ["infinity"]
      volumeMounts:
        - mountPath: "/mnt/models"
          name: model-store
      securityContext:
        allowPrivilegeEscalation: false
        capabilities:
          drop: ["ALL"]
        runAsNonRoot: true
        runAsUser: 1000
        seccompProfile:
          type: RuntimeDefault

I’m using a zip file from a uri: fraud -> I downloaded it locally though (unzip it locally)
So now we’ll exec into the pod
oc exec -it model-store-pod -n katie-test-single -- bash

in another terminal:
oc cp ~/Desktop/fraud katie-test-single/model-store-pod:/mnt/models/

back in the pod shell:
find /mnt/models
get some kind of output showing the files and folders look for something along the lines of fraud/1/model.onnx

exit

from this point you can deploy from the UI

Test Impact

Added unit and mock 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

Summary by CodeRabbit

  • New Features

    • Added support for selecting existing Persistent Volume Claims (PVCs) as a storage option when deploying models.
    • Introduced a new interface for browsing and selecting available PVCs, including annotation display and access mode warnings.
    • Users can now input and validate model paths within selected PVCs.
    • Integrated PVC selection into model serving deployment workflows with automatic namespace-aware PVC fetching.
  • Bug Fixes

    • Improved UI logic to handle scenarios with no existing connections but available PVCs.
  • Tests

    • Added end-to-end tests for deploying models using PVC storage.
    • Enhanced test utilities and selectors for PVC-related UI elements.
    • Added unit tests for PVC annotation extraction utilities.
  • Documentation

    • Updated internal documentation and test coverage for new PVC annotation utilities.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jul 3, 2025
Copy link
Contributor

coderabbitai bot commented Jul 3, 2025

"""

Walkthrough

This change introduces full support for Persistent Volume Claim (PVC) storage as a selectable option for model serving. It adds UI components, hooks, utilities, and test coverage to enable users to select existing PVCs, configure model paths, and deploy models using PVC-based storage.

Changes

File(s) Change Summary
.../InferenceServiceModal/ConnectionSection.tsx, .../PVCSelect.tsx, .../PVCFields.tsx Added props and UI for PVC storage option, new PVC selection and path input components, and related logic.
.../projects/kServeModal/ManageKServeModal.tsx, .../usePvcs.ts Integrated PVC fetching hook and passed PVC data to connection section.
.../projects/utils.ts, .../utils.ts, .../screens/types.ts Added PVC storage type enum, PVC annotation utility, and storage URI handling for PVCs.
.../mockPVCK8sResource.ts Enhanced mock resource generator to support annotations and labels for PVCs.
.../tests/cypress/cypress/pages/modelServing.ts, .../servingRuntimeList.cy.ts Added Cypress test selectors and a new test case for deploying models with PVC storage.
.../modelServing/tests/utils.spec.ts Added tests for PVC annotation extraction utility.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UI
    participant usePvcs Hook
    participant API

    User->>UI: Open Deploy Model modal
    UI->>usePvcs Hook: Fetch PVCs for namespace
    usePvcs Hook->>API: getDashboardPvcs(namespace)
    API-->>usePvcs Hook: Return list of PVCs
    usePvcs Hook-->>UI: Provide PVCs
    UI->>User: Show PVC storage option if enabled
    User->>UI: Select PVC storage, choose PVC, set model path
    UI->>API: Deploy model with pvc://<pvcName>/<path> URI
Loading

Suggested labels

lgtm, approved, area/quality

Suggested reviewers

  • andrewballantyne
  • emilys314

Poem

A bunny hops through fields of code,
Bringing PVC support to lighten the load.
With dropdowns and paths, the models deploy,
Storage choices now bring extra joy!
Tests and hooks all freshly spun—
Hooray for features, this merge is done!
🐇✨
"""


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 778ed46 and f15e003.

📒 Files selected for processing (12)
  • frontend/src/__mocks__/mockPVCK8sResource.ts (2 hunks)
  • frontend/src/__tests__/cypress/cypress/pages/modelServing.ts (2 hunks)
  • frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (4 hunks)
  • frontend/src/pages/modelServing/__tests__/utils.spec.ts (2 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx (7 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (1 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCSelect.tsx (1 hunks)
  • frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx (3 hunks)
  • frontend/src/pages/modelServing/screens/projects/utils.ts (1 hunks)
  • frontend/src/pages/modelServing/screens/types.ts (1 hunks)
  • frontend/src/pages/modelServing/usePvcs.ts (1 hunks)
  • frontend/src/pages/modelServing/utils.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
  • frontend/src/pages/modelServing/utils.ts
  • frontend/src/pages/modelServing/screens/types.ts
  • frontend/src/tests/cypress/cypress/pages/modelServing.ts
  • frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx
  • frontend/src/pages/modelServing/usePvcs.ts
  • frontend/src/pages/modelServing/screens/projects/utils.ts
  • frontend/src/pages/modelServing/tests/utils.spec.ts
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCSelect.tsx
  • frontend/src/mocks/mockPVCK8sResource.ts
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx
  • frontend/src/tests/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.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: Lint
  • GitHub Check: Cypress-Setup
✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

openshift-ci bot commented Jul 3, 2025

[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 antowaddle 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:

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 194ee40 and ce5ce97.

📒 Files selected for processing (9)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx (5 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx (4 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (1 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCSelect.tsx (1 hunks)
  • frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx (6 hunks)
  • frontend/src/pages/modelServing/screens/projects/utils.ts (1 hunks)
  • frontend/src/pages/modelServing/screens/types.ts (1 hunks)
  • frontend/src/pages/modelServing/usePvcs.ts (1 hunks)
  • frontend/src/pages/modelServing/utils.ts (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/src/pages/modelServing/usePvcs.ts (1)
Learnt from: Gkrumbach07
PR: opendatahub-io/odh-dashboard#4336
File: frontend/src/pages/notebookController/SetupCurrentNotebook.tsx:20-25
Timestamp: 2025-06-24T14:17:43.285Z
Learning: The useNamespaces hook in frontend/src/pages/notebookController/useNamespaces.ts returns workbenchNamespace that falls back to dashboardNamespace if undefined, so the returned workbenchNamespace is always a defined string value and doesn't require additional type safety checks.
🧬 Code Graph Analysis (4)
frontend/src/pages/modelServing/utils.ts (1)
frontend/src/k8sTypes.ts (1)
  • PersistentVolumeClaimKind (326-349)
frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (1)
frontend/src/concepts/connectionTypes/utils.ts (2)
  • trimInputOnBlur (454-461)
  • trimInputOnPaste (463-472)
frontend/src/pages/modelServing/usePvcs.ts (2)
frontend/src/utilities/useFetch.ts (1)
  • FetchStateObject (37-43)
frontend/src/api/k8s/pvcs.ts (1)
  • getDashboardPvcs (63-70)
frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx (4)
frontend/src/k8sTypes.ts (1)
  • PersistentVolumeClaimKind (326-349)
frontend/src/utilities/useWatchConnectionTypes.tsx (1)
  • useWatchConnectionTypes (7-22)
frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCSelect.tsx (1)
  • PvcSelect (20-121)
frontend/src/concepts/k8s/utils.ts (1)
  • getResourceNameFromK8sResource (13-14)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Cypress-Setup
  • GitHub Check: Lint
🔇 Additional comments (20)
frontend/src/pages/modelServing/usePvcs.ts (2)

40-48: LGTM - Well-implemented React hook following established patterns.

The hook correctly implements the standard fetch pattern with proper state management, error handling, and memoization. The eslint-disable comment is justified since fetchPvcs is already included in the dependency array through the useCallback hook, avoiding unnecessary re-renders.


17-38: Proper error handling and state management.

The fetchPvcs function correctly handles both success and error cases, ensuring the state is always updated with appropriate loading flags and error information. The promise chaining properly returns the data for consumers.

frontend/src/pages/modelServing/screens/types.ts (1)

106-106: LGTM - Consistent enum addition for PVC storage support.

The new PVC_STORAGE enum member follows the established naming convention and integrates well with the existing storage type options.

frontend/src/pages/modelServing/utils.ts (2)

35-35: LGTM - Import addition supports new PVC functionality.

The import of PersistentVolumeClaimKind is correctly placed and necessary for the new utility function.


398-405: LGTM - Well-designed utility function for PVC annotations.

The function correctly extracts OpenDataHub-specific annotations from PVC metadata with appropriate null defaults. The annotation keys follow the established dashboard.opendatahub.io/ namespace convention.

frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx (3)

15-15: LGTM - Proper imports for platform detection.

The imported functions getProjectModelServingPlatform and useServingPlatformStatuses are the correct utilities for determining the current project's serving platform.

Also applies to: 36-36


77-81: LGTM - Robust platform detection with proper null handling.

The platform detection correctly handles the case where currentProject might be undefined, falling back to undefined for the platform value. This follows the established pattern in the codebase.


247-247: LGTM - Platform context properly passed to ConnectionSection.

The platform prop enables the ConnectionSection component to conditionally render PVC storage options based on the current serving platform.

frontend/src/pages/modelServing/screens/projects/utils.ts (1)

366-368: LGTM - Consistent PVC storage handling in inference service creation.

The PVC_STORAGE case correctly assigns the storage URI and follows the same pattern as other storage types. This integrates PVC storage seamlessly into the existing inference service creation workflow.

frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (1)

18-42: LGTM! Well-structured component with proper accessibility.

The component correctly implements the PVC model path input with proper form structure, accessibility attributes, and test IDs. The URI format display is clear and helpful for users.

frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx (3)

119-135: LGTM! Platform determination logic is well-structured.

The getPlatform helper function correctly prioritizes current project context over editing context and handles both ModelMesh and KServe platform detection appropriately.


163-167: LGTM! Clean integration of platform and PVC data.

The platform determination and PVC fetching are properly implemented using the appropriate hooks and passed correctly to child components.


504-505: LGTM! Props correctly passed to ConnectionSection.

The pvcs.data and platform props are appropriately passed to enable PVC storage functionality in the connection section.

frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx (3)

42-48: LGTM! Appropriate imports for PVC functionality.

The new imports correctly bring in the necessary types and components for PVC storage support.


288-291: LGTM! Correct PVC selection logic.

The selectedPVC computation properly finds the matching PVC by comparing metadata names with the data connection identifier.


312-344: LGTM! Well-integrated PVC storage option.

The PVC storage radio option is properly gated by feature flag and platform check, integrates cleanly with existing storage options, and correctly manages state transitions when selected.

frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCSelect.tsx (4)

31-43: LGTM! Proper PVC selection state management.

The effect correctly handles PVC selection changes, extracts annotations for the model path, and manages state transitions appropriately.


60-82: LGTM! Well-structured PVC options with helpful annotations.

The options generation properly displays PVC names with model serving labels, providing clear visual indicators for annotated PVCs and handling the "unknown model" fallback appropriately.


102-107: LGTM! Appropriate access mode validation.

The warning for non-ReadWriteMany access modes helps users understand potential limitations with their PVC selection.


86-120: LGTM! Clean UI structure with proper conditional rendering.

The component structure with Stack layout properly organizes the TypeaheadSelect, warning alert, and PVCFields components with appropriate conditional rendering.

Copy link

codecov bot commented Jul 3, 2025

Codecov Report

Attention: Patch coverage is 91.20879% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.55%. Comparing base (90aefc5) to head (f15e003).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...ojects/InferenceServiceModal/ConnectionSection.tsx 82.60% 4 Missing ⚠️
...reens/projects/InferenceServiceModal/PVCFields.tsx 84.21% 3 Missing ⚠️
...reens/projects/InferenceServiceModal/PVCSelect.tsx 96.87% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4449      +/-   ##
==========================================
- Coverage   82.63%   82.55%   -0.09%     
==========================================
  Files        1760     1764       +4     
  Lines       36804    36942     +138     
  Branches    10911    10952      +41     
==========================================
+ Hits        30413    30496      +83     
- Misses       6391     6446      +55     
Files with missing lines Coverage Δ
...screens/projects/kServeModal/ManageKServeModal.tsx 94.28% <100.00%> (-0.68%) ⬇️
...d/src/pages/modelServing/screens/projects/utils.ts 94.65% <100.00%> (+0.04%) ⬆️
frontend/src/pages/modelServing/screens/types.ts 100.00% <100.00%> (ø)
frontend/src/pages/modelServing/usePvcs.ts 100.00% <100.00%> (ø)
frontend/src/pages/modelServing/utils.ts 90.79% <100.00%> (+0.29%) ⬆️
...reens/projects/InferenceServiceModal/PVCSelect.tsx 96.87% <96.87%> (ø)
...reens/projects/InferenceServiceModal/PVCFields.tsx 84.21% <84.21%> (ø)
...ojects/InferenceServiceModal/ConnectionSection.tsx 92.50% <82.60%> (-1.44%) ⬇️

... and 14 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 90aefc5...f15e003. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (2)

2134-2149: Consider enhancing UI verification and error scenarios.

While the test correctly verifies the basic PVC workflow, consider these improvements:

  1. More UI state verification: The test only checks that the path input has the expected value. Consider also verifying that the PVC is actually selected in the dropdown.
  2. Error scenario testing: The test only covers the happy path. Consider adding tests for scenarios like no PVCs available, PVCs without required annotations, etc.

Example enhancement:

+ // Verify PVC is selected in dropdown
+ kserveModal.findPVCSelect().should('contain.text', 'Test PVC');
+ // Verify model name is populated from annotation  
+ kserveModal.findModelNameFromPVC().should('have.text', 'test-model');

2106-2220: Consider adding edge case test coverage.

The current test provides good coverage for the standard PVC deployment workflow. To make the test suite more robust, consider adding separate test cases for:

  1. PVC without model annotations: Test behavior when PVC lacks dashboard.opendatahub.io/model-name or dashboard.opendatahub.io/model-path annotations
  2. Multiple PVCs scenario: Test PVC selection when multiple PVCs are available
  3. Empty PVC list: Test behavior when no PVCs are available in the namespace
  4. Path validation: Test path input validation specific to PVC contexts

Example additional test structure:

it('Handle PVC without annotations gracefully', () => {
  // Test with PVC missing required annotations
});

it('Select from multiple available PVCs', () => {
  // Test PVC selection dropdown with multiple options
});
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f31244 and e4266fa.

📒 Files selected for processing (13)
  • frontend/src/__mocks__/mockPVCK8sResource.ts (2 hunks)
  • frontend/src/__tests__/cypress/cypress/pages/modelServing.ts (2 hunks)
  • frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (4 hunks)
  • frontend/src/pages/modelServing/__tests__/utils.spec.ts (2 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx (5 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx (4 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (1 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCSelect.tsx (1 hunks)
  • frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx (6 hunks)
  • frontend/src/pages/modelServing/screens/projects/utils.ts (1 hunks)
  • frontend/src/pages/modelServing/screens/types.ts (1 hunks)
  • frontend/src/pages/modelServing/usePvcs.ts (1 hunks)
  • frontend/src/pages/modelServing/utils.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx
  • frontend/src/pages/modelServing/screens/projects/utils.ts
  • frontend/src/pages/modelServing/screens/types.ts
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx
  • frontend/src/pages/modelServing/utils.ts
  • frontend/src/pages/modelServing/usePvcs.ts
  • frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCSelect.tsx
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx
🧰 Additional context used
🧠 Learnings (1)
frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (3)

undefined

<retrieved_learning>
Learnt from: christianvogt
PR: #4381
File: frontend/src/tests/cypress/tsconfig.json:9-9
Timestamp: 2025-06-19T20:38:32.485Z
Learning: In the ODH Dashboard project, the frontend/src/__tests__/cypress/tsconfig.json file intentionally has an empty files array to disable type checking for Cypress test files. This is part of the monorepo structure where Cypress was separated into its own package but type checking is deliberately disabled for it.
</retrieved_learning>

<retrieved_learning>
Learnt from: ConorOM1
PR: #4444
File: frontend/src/tests/cypress/cypress/tests/e2e/modelRegistry/testRegisterModel.cy.ts:139-195
Timestamp: 2025-07-02T10:12:55.016Z
Learning: In ODH Dashboard Cypress tests marked with @NonConcurrent tag, tests are intentionally designed to run sequentially to simulate realistic user workflows. For model registry tests, this allows testing scenarios like adding versions to existing models rather than creating isolated test scenarios, which better reflects how users would actually interact with the system.
</retrieved_learning>

<retrieved_learning>
Learnt from: ConorOM1
PR: #4423
File: frontend/src/tests/cypress/cypress/fixtures/resources/yaml/model_registry.yaml:12-12
Timestamp: 2025-06-27T11:00:06.871Z
Learning: For ModelRegistry resources in frontend/src/__tests__/cypress/cypress/fixtures/resources/yaml/model_registry.yaml, the oauthProxy: {} empty configuration is sufficient to enable the OAuth proxy. The ModelRegistry operator enables the proxy by default when this key is present, without requiring explicit enabled: true flags or image specifications.
</retrieved_learning>

🧬 Code Graph Analysis (3)
frontend/src/pages/modelServing/__tests__/utils.spec.ts (2)
frontend/src/__mocks__/mockPVCK8sResource.ts (1)
  • mockPVCK8sResource (18-64)
frontend/src/pages/modelServing/utils.ts (1)
  • getModelServingPVCAnnotations (398-405)
frontend/src/__mocks__/mockPVCK8sResource.ts (1)
frontend/src/k8sTypes.ts (1)
  • PersistentVolumeClaimKind (326-349)
frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (4)
frontend/src/__mocks__/mockK8sResourceList.ts (1)
  • mockK8sResourceList (3-17)
frontend/src/__mocks__/mockPVCK8sResource.ts (1)
  • mockPVCK8sResource (18-64)
frontend/src/__tests__/cypress/cypress/pages/projects.ts (1)
  • projectDetails (416-416)
frontend/src/__tests__/cypress/cypress/pages/modelServing.ts (2)
  • modelServingSection (783-783)
  • kserveModal (786-786)
⏰ 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
🔇 Additional comments (10)
frontend/src/pages/modelServing/__tests__/utils.spec.ts (2)

9-9: LGTM!

The imports for the new function and mock resource are correctly added to support the new test coverage.

Also applies to: 12-12


359-380: Well-structured test coverage for the PVC annotations utility.

The test suite properly validates both scenarios:

  1. When annotations are present - verifies correct extraction
  2. When annotations are missing - verifies null fallback behavior

The mock configuration uses the correct annotation keys that match the function implementation, and the assertions are precise and appropriate.

frontend/src/__tests__/cypress/cypress/pages/modelServing.ts (3)

252-254: LGTM! New PVC selector method follows established patterns.

The findPVCSelect() method correctly implements the selector pattern used throughout the test page objects.


544-546: LGTM! New PVC connection option selector is correctly implemented.

The findPVCConnectionOption() method follows the established selector pattern and uses an appropriate test ID.


548-550: Good approach to avoid method duplication.

Commenting out the duplicate findPVCSelect() method is correct since KServeModal inherits this method from InferenceServiceModal through the mixin pattern.

frontend/src/__mocks__/mockPVCK8sResource.ts (2)

14-15: LGTM! Type extensions correctly support PVC metadata.

The optional annotations and labels properties are properly typed as Record<string, string>, which aligns with Kubernetes resource specifications.


33-34: LGTM! Mock function properly supports custom annotations and labels.

The implementation correctly:

  • Uses default empty objects to prevent undefined issues
  • Merges custom annotations/labels with existing defaults using spread operator
  • Preserves the existing default annotations and labels

This enhancement properly supports the PVC metadata requirements for the model serving feature.

Also applies to: 42-42, 48-48

frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (3)

100-100: LGTM! Clean integration of PVC serving configuration.

The addition of the disablePVCServing parameter follows the established pattern for feature toggles in the test suite, with appropriate default values and proper propagation to the dashboard config mock.

Also applies to: 114-114, 174-174


2106-2133: Comprehensive test setup with proper PVC mocking.

The test initialization properly enables PVC serving and sets up realistic mock data with appropriate annotations. The PVC mock includes all necessary fields for the dashboard integration.


2150-2220: Excellent API verification for PVC deployment.

The test thoroughly validates both dry-run and actual API requests, ensuring the correct PVC-based storage URI (pvc://test-pvc/test-path) is generated and all expected metadata is included in the requests. The assertions cover both ServingRuntime and InferenceService creation with proper annotation handling.

@katieperry4 katieperry4 changed the title [WIP] Deploy a Model from a PVC Deploy a Model from a PVC Jul 7, 2025
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jul 7, 2025
@katieperry4 katieperry4 force-pushed the 28016/deploy-from-pvc branch from e4266fa to 9be2aab Compare July 7, 2025 15:06
const [modelPath, setModelPath] = React.useState('');
const lastPVCNameRef = React.useRef<string | undefined>();

React.useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you explored reducing the number of useEffects? They are generally a code smell unless you are doing networking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah fair, I combined them into one useEffect

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I more meant trying to put the logic outside of a useEffect.

For example looking for opportunities to set initial values, or doing things on a onChange
This is a common one. This is just an example, I don't think you can remove yours out of the useEffect easily:

const [myState, setMyState] = useState()

React.useEffect(() => {
  if (!myState) {
    setMyState(initValue)  // this could be moved into the `useState(initValue)`
  }
,[]}

Then for doing stuff as a value changes

const [myState, setMyState] = useState()
const [isvalid, setIsValid] = useState()

React.useEffect(() => {
  if (myState === badThing) {
    setIsValid(false) 
  }
,[]}

return <Button onChange={(value) => setMyState(value} />

could become doing things as it changes, not in response to

const [myState, setMyState] = useState()
const [isvalid, setIsValid] = useState()

return 
  <Button 
    onChange={(value) => {
      setMyState(value} 
      setIsvalid(....)
    }
  />

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you have to use the useEffects, then it's fine to keep them responsible to only 1 thing each. Like 1 for the init. And a 2nd for another thing, and so on

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh I see what you mean! I made some changes and removed the useEffect in PVCSelect I did have to have a little one in PVCFields for when the input gets prefilled by the path from the annotation / the input changes

@andrewballantyne
Copy link
Member

andrewballantyne commented Jul 7, 2025

image

Is the unknown model because it has a path but not a name?

@katieperry4
Copy link
Contributor Author

@andrewballantyne yeah!

@andrewballantyne
Copy link
Member

Awesome, that sounds good from a PO perspective for now. We'll want to make sure the whole experience fits together in the end. We can talk with UX tomorrow about that.

@katieperry4 katieperry4 force-pushed the 28016/deploy-from-pvc branch 4 times, most recently from f9ec98c to 88806d1 Compare July 8, 2025 15:08
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (1)

68-69: Fix incorrect usage and parameters of trim utility functions.

There are two issues here:

  1. The functions return event handlers but are being called incorrectly (as noted in a previous review)
  2. The functions are being passed incorrect parameters - they should receive setModelPath not setModelUri
-            onBlur={trimInputOnBlur(modelPath, setModelUri)}
-            onPaste={trimInputOnPaste(modelPath, setModelUri)}
+            onBlur={trimInputOnBlur(modelPath, setModelPath)}
+            onPaste={trimInputOnPaste(modelPath, setModelPath)}
🧹 Nitpick comments (2)
frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (2)

27-28: Move regex and utility function outside component for better performance.

The pathRegex doesn't have dependencies and generateModelUri is a pure function. Consider moving them outside the component to avoid recreation on each render.

+const PATH_REGEX = /^pvc:\/\/[a-zA-Z0-9-]+\/[^/\s][^\s]*$/;
+const generateModelUri = (pvcName: string, path: string): string => `pvc://${pvcName}/${path}`;
+
export const PVCFields: React.FC<PVCFieldsProps> = ({
  selectedPVC,
  setModelUri,
  modelPath,
  setModelPath,
  setIsConnectionValid,
}) => {
-  const pathRegex = React.useMemo(() => /^pvc:\/\/[a-zA-Z0-9-]+\/[^/\s][^\s]*$/, []);
-  const generateModelUri = (pvcName: string, path: string): string => `pvc://${pvcName}/${path}`;

30-33: Ensure pathRegex variable is updated after moving to constant.

After moving pathRegex to a constant, update this function to use the new constant name.

  const validateModelPath = (newPath: string): boolean => {
    const uri = generateModelUri(selectedPVC.metadata.name, newPath);
-    return pathRegex.test(uri);
+    return PATH_REGEX.test(uri);
  };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f9ec98c and 88806d1.

📒 Files selected for processing (12)
  • frontend/src/__mocks__/mockPVCK8sResource.ts (2 hunks)
  • frontend/src/__tests__/cypress/cypress/pages/modelServing.ts (2 hunks)
  • frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts (4 hunks)
  • frontend/src/pages/modelServing/__tests__/utils.spec.ts (2 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx (7 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (1 hunks)
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCSelect.tsx (1 hunks)
  • frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx (3 hunks)
  • frontend/src/pages/modelServing/screens/projects/utils.ts (1 hunks)
  • frontend/src/pages/modelServing/screens/types.ts (1 hunks)
  • frontend/src/pages/modelServing/usePvcs.ts (1 hunks)
  • frontend/src/pages/modelServing/utils.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
  • frontend/src/pages/modelServing/screens/types.ts
  • frontend/src/pages/modelServing/usePvcs.ts
  • frontend/src/pages/modelServing/screens/projects/utils.ts
  • frontend/src/pages/modelServing/tests/utils.spec.ts
  • frontend/src/pages/modelServing/utils.ts
  • frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx
  • frontend/src/tests/cypress/cypress/pages/modelServing.ts
  • frontend/src/mocks/mockPVCK8sResource.ts
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCSelect.tsx
  • frontend/src/tests/cypress/cypress/tests/mocked/modelServing/runtime/servingRuntimeList.cy.ts
  • frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/ConnectionSection.tsx
🧰 Additional context used
🧠 Learnings (1)
frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (1)
Learnt from: DaoDaoNoCode
PR: opendatahub-io/odh-dashboard#4407
File: frontend/src/components/MemoryField.tsx:63-67
Timestamp: 2025-06-24T16:15:44.961Z
Learning: In React components with checkbox-controlled inputs, using onBlur to update a ref that stores the previous value is more user-centric than useEffect with prop dependencies. The onBlur approach captures the value when the user finishes editing rather than on every prop change, ensuring the stored value represents actual user input.
🧬 Code Graph Analysis (1)
frontend/src/pages/modelServing/screens/projects/InferenceServiceModal/PVCFields.tsx (1)
frontend/src/concepts/connectionTypes/utils.ts (2)
  • trimInputOnBlur (454-461)
  • trimInputOnPaste (463-472)

@katieperry4 katieperry4 force-pushed the 28016/deploy-from-pvc branch from 88806d1 to 778ed46 Compare July 8, 2025 17:59
@katieperry4 katieperry4 force-pushed the 28016/deploy-from-pvc branch from 778ed46 to f15e003 Compare July 8, 2025 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants