fix(studio): Performance optimization for ModelSelect model fetch - #950
Merged
Conversation
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
Contributor
|
Contributor
📝 WalkthroughWalkthroughModel selection now uses workspace-scoped, paginated search with entity lookup and virtualized dropdown rendering. Studio components consume shared search state, provider filters, and workspace props, while Data Designer autofills seeded models through workspace candidate resolution. ChangesModel selection flow
Sequence Diagram(s)sequenceDiagram
participant User
participant WorkspaceModelSelect
participant useModelSearch
participant ModelDropdownList
participant ModelCompareRoute
User->>WorkspaceModelSelect: Open and search models
WorkspaceModelSelect->>useModelSearch: Request workspace-scoped results
useModelSearch-->>WorkspaceModelSelect: Groups, loading, and pagination state
ModelDropdownList->>useModelSearch: Load additional page
useModelSearch-->>ModelDropdownList: Append model results
ModelCompareRoute->>useModelSearch: Check model availability
ModelCompareRoute->>useModelEntity: Resolve query model
useModelEntity-->>ModelCompareRoute: Return selected entity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/studio/src/components/ModelConfigPanel/index.tsx (1)
114-125: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing
hasModelProviderfilter allows selecting providerless models.Unlike
ModelChatPanel/ModelColumnSelect, thisWorkspaceModelSelecthas noinclude={hasModelProvider}.handleModelChangethen setsproviderviaproviderForSelection, which yields''for such models — andvalidateModelsnever checks provider presence, so the job build proceeds and fails at submission time with "the model does not have a provider" (per the comment inmodels.ts). See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/components/ModelConfigPanel/index.tsx` around lines 114 - 125, Update the WorkspaceModelSelect usage in ModelConfigPanel to pass hasModelProvider through its include filter, matching ModelChatPanel and ModelColumnSelect, so providerless models cannot be selected and handleModelChange only receives valid provider-backed models.
🧹 Nitpick comments (3)
web/packages/studio/src/routes/ModelCompareRoute/index.tsx (1)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup external and internal imports separately.
The new
@nemo/commonimports are interleaved with external packages and@studioaliases. As per coding guidelines, group external libraries first, then internal modules, then relative imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/ModelCompareRoute/index.tsx` around lines 4 - 8, Reorder the imports in ModelCompareRoute so external package imports come first, followed by internal `@nemo/common` and `@studio` alias imports, with relative imports last. Keep the imported symbols unchanged.Source: Coding guidelines
web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts (1)
109-138: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant duplicate "first page" fetches across pending models.
fetchAutoFillCandidatesis called once per pending model, and each call independently re-fetches the same unfiltered first page of the workspace. With multiple pending models this fires duplicate identical requests. Consider fetching the shared first page once and reusing it across all pending models.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts` around lines 109 - 138, Update the auto-fill effect around fetchAutoFillCandidates so the workspace’s unfiltered first-page candidates are fetched once and reused for every pending model, while preserving preferred-model-specific resolution behavior. Avoid issuing duplicate identical requests from the pending.map flow, and continue applying each model’s resolved value through the existing setValue logic.web/packages/common/src/api/models/useModelEntity.ts (1)
22-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
useModelEntity's enabled-gating logic.Logic is correct but untested (unlike
useModelSearch.test.tsx). Theenabled && !!parts?.workspace && !!parts?.namegate and malformed-URN handling are exactly the kind of boolean logic that regresses silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/api/models/useModelEntity.ts` around lines 22 - 31, Add tests for useModelEntity covering enabled=true with a valid URN, enabled=false, and malformed or missing URNs. Assert that useModelsGetModel receives the parsed workspace/name and that its query.enabled reflects enabled && !!parts?.workspace && !!parts?.name, including disabled calls when required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/packages/studio/src/components/PromptTuningForm/ModelDetailsSection/index.tsx`:
- Around line 41-45: Surface the useModelSearch error in ModelDetailsSection by
applying the existing useSetFieldErrorOnApiError pattern used by
JudgeModelSelect. Pass the search error and the appropriate model field/form
context so failed prompt-tuneable model searches set a visible field error
instead of being silently ignored.
In `@web/packages/studio/src/components/sidePanels/MetricRunSidePanel/index.tsx`:
- Around line 164-165: Update the model search flow in the MetricRunSidePanel
component to surface modelSearch.error to the user, matching the existing
error-handling behavior used by ModelDetailsSection. Preserve the current
workspace, open, and modelSelectOpen search enablement logic.
In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/models.ts`:
- Around line 55-89: Update fetchAutoFillCandidates so both preferred matches
and the initial model page only include entities with a non-empty
model_providers value before grouping them. Ensure resolveTemplateModel can only
receive provider-backed models, while preserving the existing deduplication and
workspace grouping behavior.
In `@web/packages/studio/src/routes/ModelCompareRoute/index.tsx`:
- Around line 51-75: Update the preselection commit in the effect using
preselectModel so it only calls setModels when preselectModel.workspace matches
workspace and hasModelProvider(preselectModel) returns true. Keep the existing
pending and didPreselectRef behavior, and mark preselection complete after the
model has been validated or rejected.
- Around line 37-40: Update the ModelCompareRoute logic around availableModels
and hasNoModels to handle availableModels.error separately from an exhausted
successful search. Render the error/retry state when a search fails, and set the
no-models condition only when loading is complete, no error exists, the search
is exhausted, and models is empty.
- Around line 62-75: Update the preselection logic in the useEffect around
didPreselectRef so the guard is reset when the URL’s model selection changes,
including transitions from no modelParam to a modelParam and between different
model values. Track the last processed preselectModel URN or equivalent
selection state, while preserving the existing early returns and model update
behavior for already-processed selections.
---
Outside diff comments:
In `@web/packages/studio/src/components/ModelConfigPanel/index.tsx`:
- Around line 114-125: Update the WorkspaceModelSelect usage in ModelConfigPanel
to pass hasModelProvider through its include filter, matching ModelChatPanel and
ModelColumnSelect, so providerless models cannot be selected and
handleModelChange only receives valid provider-backed models.
---
Nitpick comments:
In `@web/packages/common/src/api/models/useModelEntity.ts`:
- Around line 22-31: Add tests for useModelEntity covering enabled=true with a
valid URN, enabled=false, and malformed or missing URNs. Assert that
useModelsGetModel receives the parsed workspace/name and that its query.enabled
reflects enabled && !!parts?.workspace && !!parts?.name, including disabled
calls when required.
In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts`:
- Around line 109-138: Update the auto-fill effect around
fetchAutoFillCandidates so the workspace’s unfiltered first-page candidates are
fetched once and reused for every pending model, while preserving
preferred-model-specific resolution behavior. Avoid issuing duplicate identical
requests from the pending.map flow, and continue applying each model’s resolved
value through the existing setValue logic.
In `@web/packages/studio/src/routes/ModelCompareRoute/index.tsx`:
- Around line 4-8: Reorder the imports in ModelCompareRoute so external package
imports come first, followed by internal `@nemo/common` and `@studio` alias imports,
with relative imports last. Keep the imported symbols unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec58eb88-1701-4dc0-a011-d496ba2a78f3
📒 Files selected for processing (35)
web/packages/common/src/api/models/useModelEntity.tsweb/packages/common/src/api/models/useModelSearch.test.tsxweb/packages/common/src/api/models/useModelSearch.tsweb/packages/common/src/components/ModelSelectV2/ModelDropdown.test.tsxweb/packages/common/src/components/ModelSelectV2/ModelDropdown.tsxweb/packages/common/src/components/ModelSelectV2/ModelDropdownItem.tsxweb/packages/common/src/components/ModelSelectV2/ModelDropdownList.tsxweb/packages/common/src/components/ModelSelectV2/ModelSelectV2.stories.tsxweb/packages/common/src/components/ModelSelectV2/ModelSelectV2.tsxweb/packages/common/src/components/ModelSelectV2/WorkspaceModelSelect.tsxweb/packages/common/src/components/ModelSelectV2/index.tsxweb/packages/common/src/components/ModelSelectV2/types.tsweb/packages/common/src/utils/models.tsweb/packages/studio/src/components/AddModelPalette/AddModelPalette.stories.tsxweb/packages/studio/src/components/AddModelPalette/index.tsxweb/packages/studio/src/components/ModelChatPanel/ModelChatPanel.test.tsxweb/packages/studio/src/components/ModelChatPanel/index.tsxweb/packages/studio/src/components/ModelCompareChat/index.tsxweb/packages/studio/src/components/ModelComparePrompts/ModelColumnSelect.tsxweb/packages/studio/src/components/ModelComparePrompts/ModelCompareTable.tsxweb/packages/studio/src/components/ModelComparePrompts/index.tsxweb/packages/studio/src/components/ModelComparePrompts/types.tsweb/packages/studio/src/components/ModelConfigPanel/index.tsxweb/packages/studio/src/components/NewCustomizationForm/ModelSelectionSection.tsxweb/packages/studio/src/components/PromptTuningForm/ModelDetailsSection/index.tsxweb/packages/studio/src/components/evaluation/JudgeModelSelect.tsxweb/packages/studio/src/components/sidePanels/MetricRunSidePanel/index.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderConfigPane.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/index.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/models.test.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/models.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.tsweb/packages/studio/src/routes/DeploymentsListRoute/CreateDeploymentSidePanel/WorkspaceSourceFields.tsxweb/packages/studio/src/routes/ModelCompareRoute/index.tsx
💤 Files with no reviewable changes (2)
- web/packages/studio/src/components/ModelComparePrompts/types.ts
- web/packages/studio/src/components/ModelCompareChat/index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/packages/common/src/components/ModelSelectV2/ModelDropdownItem.tsx (1)
125-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
model.adapters!— restructure for narrowing instead of asserting.
hasAdaptersaliasesmodel.adapters && model.adapters.length > 0, but TS's aliased-condition narrowing doesn't propagate to non-readonlyobject properties, hence the!at line 168. Extractingmodel.adaptersinto a localconstfirst narrows normally without an assertion, per guideline: "Use type assertions sparingly. Prefer type guards and narrowing in TypeScript."♻️ Proposed fix
- const hasAdapters = !hideAdapters && model.adapters && model.adapters.length > 0; + const adapters = hideAdapters ? undefined : model.adapters; + const hasAdapters = adapters !== undefined && adapters.length > 0;And later:
- {sortAdaptersByNewest(model.adapters!).map((adapter) => ( + {sortAdaptersByNewest(adapters).map((adapter) => (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/ModelSelectV2/ModelDropdownItem.tsx` around lines 125 - 168, In ModelDropdownItem, extract model.adapters into a local constant and use that value for the hasAdapters check and adapter iteration. Replace sortAdaptersByNewest(model.adapters!) with the narrowed local collection, preserving the existing adapter rendering behavior without a non-null assertion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.test.tsx`:
- Around line 75-81: Update the useJobBuilder test setup to seed a model with a
non-empty provider instead of models: []. Assert getBuilderValues().models
retains that seeded model unchanged, while preserving the expectation that
mockListModels is not called.
---
Nitpick comments:
In `@web/packages/common/src/components/ModelSelectV2/ModelDropdownItem.tsx`:
- Around line 125-168: In ModelDropdownItem, extract model.adapters into a local
constant and use that value for the hasAdapters check and adapter iteration.
Replace sortAdaptersByNewest(model.adapters!) with the narrowed local
collection, preserving the existing adapter rendering behavior without a
non-null assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 87b49875-fe11-4a33-872b-581d0bac199b
📒 Files selected for processing (6)
web/packages/common/src/components/ModelSelectV2/ModelDropdown.test.tsxweb/packages/common/src/components/ModelSelectV2/ModelDropdown.tsxweb/packages/common/src/components/ModelSelectV2/ModelDropdownItem.tsxweb/packages/common/src/components/ModelSelectV2/ModelDropdownList.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.test.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts
- web/packages/common/src/components/ModelSelectV2/ModelDropdownList.tsx
- web/packages/common/src/components/ModelSelectV2/ModelDropdown.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/packages/studio/src/routes/ModelCompareRoute/index.test.tsx (1)
4-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the external import before internal aliases.
react-router-domis an external dependency but appears after internal@nemoand@studioimports. As per coding guidelines, group imports as external libraries, then internal modules, then relative imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/ModelCompareRoute/index.test.tsx` around lines 4 - 10, Reorder the imports in the ModelCompareRoute test so the external react-router-dom import appears before the internal `@nemo` and `@studio` aliases, preserving the existing imports and grouping conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/packages/studio/src/routes/ModelCompareRoute/index.test.tsx`:
- Around line 4-10: Reorder the imports in the ModelCompareRoute test so the
external react-router-dom import appears before the internal `@nemo` and `@studio`
aliases, preserving the existing imports and grouping conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 37be1eca-82c8-47b2-8aab-9db6f5c0f5d3
📒 Files selected for processing (6)
web/packages/studio/src/components/PromptTuningForm/ModelDetailsSection/index.tsxweb/packages/studio/src/components/sidePanels/MetricRunSidePanel/index.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/models.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.test.tsxweb/packages/studio/src/routes/ModelCompareRoute/index.test.tsxweb/packages/studio/src/routes/ModelCompareRoute/index.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- web/packages/studio/src/routes/ModelCompareRoute/index.tsx
- web/packages/studio/src/components/sidePanels/MetricRunSidePanel/index.tsx
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/models.ts
- web/packages/studio/src/components/PromptTuningForm/ModelDetailsSection/index.tsx
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.test.tsx
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
steramae-nvidia
force-pushed
the
steramae/model-fetch-perf
branch
from
July 29, 2026 16:37
552f2e7 to
bb7a44a
Compare
htolentino-nvidia
approved these changes
Jul 29, 2026
htolentino-nvidia
left a comment
htolentino-nvidia
left a comment
Contributor
There was a problem hiding this comment.
LGTM, paginates fast!
This was referenced Jul 31, 2026
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Sean Teramae steramae@nvidia.com
Summary by CodeRabbit