feat(studio): model info on fileset details page#205
Open
aray12 wants to merge 2 commits into
Open
Conversation
Signed-off-by: Alex Ray <alray@nvidia.com>
aray12
force-pushed
the
astd-189-filesets-add-model-entity-deployment-info-on-the-model
branch
from
f48b98b to
4437116
Compare
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. 📝 WalkthroughWalkthroughAdd fileset filter support to model list API: OpenAPI schema and Python backend schema now accept optional ChangesModel Fileset Filtering and Metadata Display
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openapi/openapi.yaml (1)
20319-20322: ⚡ Quick winAdd pattern validation for fileset reference format.
Description claims
{workspace}/{fileset_name}format but schema accepts any string. Addpatternconstraint:Proposed validation
fileset: description: Filter by fileset reference in the form {workspace}/{fileset_name}. title: Fileset type: string + pattern: '^[^/]+/[^/]+$'🤖 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 `@openapi/openapi.yaml` around lines 20319 - 20322, The fileset schema currently allows any string but claims the format "{workspace}/{fileset_name}"; update the fileset property in openapi.yaml to add a pattern regex enforcing a single slash-separated pair (e.g. pattern: '^[^/]+/[^/]+$') so that values like workspace/fileset_name are required; locate the fileset schema block (properties -> fileset with title "Fileset") and add the pattern constraint (and optionally an example) to validate the expected format.
🤖 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/ModelDetailRoute/ModelMetadataPanel/utils.tsx`:
- Around line 177-187: Update formatFinetuningType to map all backend enum
values (e.g., 'all_weights', 'lora_merged', 'p_tuning_v2', plus existing 'lora'
and 'p_tuning') to human-friendly labels like "All Weights", "LoRA (merged)",
"P-Tuning v2", etc., instead of returning raw snake_case; modify the switch in
formatFinetuningType to include cases for 'all_weights', 'lora_merged',
'p_tuning_v2' and any other backend enums, and fall back to a normalized
title-case conversion (replace underscores with spaces and capitalize words) for
unknown values so snake_case never renders raw.
---
Nitpick comments:
In `@openapi/openapi.yaml`:
- Around line 20319-20322: The fileset schema currently allows any string but
claims the format "{workspace}/{fileset_name}"; update the fileset property in
openapi.yaml to add a pattern regex enforcing a single slash-separated pair
(e.g. pattern: '^[^/]+/[^/]+$') so that values like workspace/fileset_name are
required; locate the fileset schema block (properties -> fileset with title
"Fileset") and add the pattern constraint (and optionally an example) to
validate the expected format.
🪄 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: 94f022ae-be41-46e3-8a0e-2d705048aae2
📒 Files selected for processing (8)
openapi/openapi.yamlservices/core/models/src/nmp/core/models/schemas.pyweb/packages/studio/src/mocks/entity-store/models.tsweb/packages/studio/src/mocks/handlers/models.tsweb/packages/studio/src/routes/ModelDetailRoute/ModelCardTab/index.tsxweb/packages/studio/src/routes/ModelDetailRoute/ModelMetadataPanel/index.tsxweb/packages/studio/src/routes/ModelDetailRoute/ModelMetadataPanel/utils.spec.tsxweb/packages/studio/src/routes/ModelDetailRoute/ModelMetadataPanel/utils.tsx
Comment on lines
+177
to
+187
| const formatFinetuningType = (type: string): string => { | ||
| switch (type.toLowerCase()) { | ||
| case 'lora': | ||
| return 'LoRA'; | ||
| case 'p_tuning': | ||
| return 'P-Tuning'; | ||
| case 'sft': | ||
| return 'SFT'; | ||
| default: | ||
| return type; | ||
| } |
There was a problem hiding this comment.
Map actual finetuning enum values, not just sft.
Line 183 maps 'sft', but the backend enum uses values like 'all_weights', 'lora_merged', and 'p_tuning_v2'; these will render raw snake_case today.
Suggested fix
const formatFinetuningType = (type: string): string => {
- switch (type.toLowerCase()) {
- case 'lora':
- return 'LoRA';
- case 'p_tuning':
- return 'P-Tuning';
- case 'sft':
- return 'SFT';
- default:
- return type;
- }
+ const normalizedType = type.toLowerCase();
+
+ if (['lora', 'qlora', 'adalora', 'dora', 'lora_plus', 'lora_merged'].includes(normalizedType)) {
+ return 'LoRA';
+ }
+ if (
+ ['prompt_tuning', 'prefix_tuning', 'p_tuning', 'p_tuning_v2', 'soft_prompt'].includes(
+ normalizedType
+ )
+ ) {
+ return 'P-Tuning';
+ }
+ if (normalizedType === 'all_weights') {
+ return 'SFT';
+ }
+ return type;
};🤖 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/ModelDetailRoute/ModelMetadataPanel/utils.tsx`
around lines 177 - 187, Update formatFinetuningType to map all backend enum
values (e.g., 'all_weights', 'lora_merged', 'p_tuning_v2', plus existing 'lora'
and 'p_tuning') to human-friendly labels like "All Weights", "LoRA (merged)",
"P-Tuning v2", etc., instead of returning raw snake_case; modify the switch in
formatFinetuningType to include cases for 'all_weights', 'lora_merged',
'p_tuning_v2' and any other backend enums, and fall back to a normalized
title-case conversion (replace underscores with spaces and capitalize words) for
unknown values so snake_case never renders raw.
…-model-entity-deployment-info-on-the-model Signed-off-by: Alex Ray <alray@nvidia.com>
Contributor
Documentation preview is readyPreview: https://nvidia-nemo.github.io/nemo-platform/pr-preview/pr-205/pr-205/ Built from This preview is deployed from this PR branch, updates when docs changes are pushed, and will be removed when the PR closes. |
There was a problem hiding this comment.
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/routes/FilesetDetailRoute/FilesetMetadataPanel/utils.spec.tsx (1)
44-50:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix
getMetadataSectionsAPI mismatch withutils.spec.tsx(entities arg not supported)
web/packages/studio/src/routes/FilesetDetailRoute/FilesetMetadataPanel/utils.tsxdefinesgetMetadataSections(fileset, readmeMetadata)(2 args), butweb/packages/studio/src/routes/FilesetDetailRoute/FilesetMetadataPanel/utils.spec.tsxcalls it with a 3rdentitiesargument (getMetadataSections(huggingfaceFileset, ..., [])/[entity]), which should fail TypeScript compilation.- The “model entity sections” assertions (
model-entity-*and “Deployment” rows) aren’t produced by the currentgetMetadataSectionsimplementation (it only returnssourceand optionaldetails).🤖 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/FilesetDetailRoute/FilesetMetadataPanel/utils.spec.tsx` around lines 44 - 50, The test calls getMetadataSections with a third entities argument but the implementation getMetadataSections(fileset, readmeMetadata) only accepts two params, so update the spec to match the API: remove the third argument (e.g., change getMetadataSections(huggingfaceFileset, { license: 'MIT', license_link: 'javascript:alert(1)' }, []) to getMetadataSections(huggingfaceFileset, { license: 'MIT', license_link: 'javascript:alert(1)' })) and adjust the expectations/assertions to only assert the sections the function currently returns (e.g., 'source' and optional 'details'), removing any assertions for "model-entity-*" or "Deployment" rows that the current getMetadataSections does not produce; reference getMetadataSections, huggingfaceFileset, and the test file utils.spec.tsx while making these edits.
🤖 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.
Outside diff comments:
In
`@web/packages/studio/src/routes/FilesetDetailRoute/FilesetMetadataPanel/utils.spec.tsx`:
- Around line 44-50: The test calls getMetadataSections with a third entities
argument but the implementation getMetadataSections(fileset, readmeMetadata)
only accepts two params, so update the spec to match the API: remove the third
argument (e.g., change getMetadataSections(huggingfaceFileset, { license: 'MIT',
license_link: 'javascript:alert(1)' }, []) to
getMetadataSections(huggingfaceFileset, { license: 'MIT', license_link:
'javascript:alert(1)' })) and adjust the expectations/assertions to only assert
the sections the function currently returns (e.g., 'source' and optional
'details'), removing any assertions for "model-entity-*" or "Deployment" rows
that the current getMetadataSections does not produce; reference
getMetadataSections, huggingfaceFileset, and the test file utils.spec.tsx while
making these edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e8b85f62-9d71-4e9a-81d2-cee26b87d825
📒 Files selected for processing (2)
openapi/openapi.yamlweb/packages/studio/src/routes/FilesetDetailRoute/FilesetMetadataPanel/utils.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- openapi/openapi.yaml
Contributor
|
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
1 participant
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.
Summary by CodeRabbit
New Features
{workspace}/{fileset_name}reference format.Tests