feat(studio): DataDesigner details profiler results#428
Open
steramae-nvidia wants to merge 6 commits into
Open
feat(studio): DataDesigner details profiler results#428steramae-nvidia wants to merge 6 commits into
steramae-nvidia wants to merge 6 commits into
Conversation
…re (#398) Add a new E2E test suite (`test_jobs_auth.py`) that validates workspace isolation and principal propagation under an auth-enabled platform config. Introduce a reusable `diagnostics.py` module in the jobs controller layer to collect and log structured job/step/task state on errors, and wire it into the reconciler and scheduler for automatic debug-level diagnostics when steps transition to ERROR or encounter unexpected exceptions. Refactor `e2e/conftest.py` to support multiple running-services instances keyed by config hash, enabling per-test-module platform configs (e.g., `local-subprocess.yaml` with auth enabled) to coexist in a single session. Add a `local-subprocess.yaml` E2E config and extend `nmp_testing` utilities with `grant_workspace_role`, `unique_email`, and `TEST_ADMIN_EMAIL` helpers needed by the auth test scenarios. Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
* feat: surface experiment metadata as dynamic columns Union of metadata keys across loaded rows, sorted alphabetically, inserted after the Models column. Each key becomes a hideable column via the existing EditColumnsMenu. Cell rendering: null/undefined → '-', object/array → JSON.stringify, primitives → String(). Values over 50 chars truncate with a tooltip showing the full string. Signed-off-by: Nathan Walston <nwalston@nvidia.com> * fix: normalize metadata keys to lowercase to collapse case variants 'status' and 'Status' now produce a single column instead of two identically-labelled headers. The accessor finds the first key that lowercases to a match, so the value is still retrieved correctly regardless of how the producer cased the key. Signed-off-by: Nathan Walston <nwalston@nvidia.com> * style: fix prettier formatting in ExperimentGroupDataView Signed-off-by: Nathan Walston <nwalston@nvidia.com> --------- Signed-off-by: Nathan Walston <nwalston@nvidia.com>
Signed-off-by: Swarom Muley <smuley@nvidia.com>
…usages (#385) * fix(studio): agent rule for correct usage of KUI select API, global CSS patch Signed-off-by: Octavian Drulea <odrulea@nvidia.com> * fix(studio): add agent rule to prevent future bugs Signed-off-by: Octavian Drulea <odrulea@nvidia.com> * fix(studio): apply SelectListbox in 2 call sites that were modified Signed-off-by: Octavian Drulea <odrulea@nvidia.com> * fix(studio): fix breaking test Signed-off-by: Octavian Drulea <odrulea@nvidia.com> --------- Signed-off-by: Octavian Drulea <odrulea@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds dataset profiling, builder config parsing, a config side panel, and shared route hooks to the Data Designer job details page. Reworks the page into Profile and Output files tabs and updates the default build model constant. ChangesData Designer Job Details: Feature Expansion
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
🧹 Nitpick comments (4)
web/packages/studio/src/routes/DataDesignerJobDetailsRoute/builderConfig.ts (1)
17-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark summary interface fields as readonly.
These interfaces represent parsed summary snapshots and should be immutable at the type level.
Proposed diff
export interface BuilderConfigColumnSummary { - name: string; - type: string; - modelAlias?: string; + readonly name: string; + readonly type: string; + readonly modelAlias?: string; } export interface BuilderConfigModelSummary { - alias: string; - model: string; - provider?: string; + readonly alias: string; + readonly model: string; + readonly provider?: string; } export interface BuilderConfigSeedSummary { - type: string; - samplingStrategy?: string; + readonly type: string; + readonly samplingStrategy?: string; } export interface BuilderConfigSummary { - columnCount: number; - columns: BuilderConfigColumnSummary[]; - columnTypeBreakdown: Array<{ type: string; count: number }>; - models: BuilderConfigModelSummary[]; - seed?: BuilderConfigSeedSummary; - constraintCount: number; - profilerCount: number; - processorNames: string[]; - libraryVersion?: string; + readonly columnCount: number; + readonly columns: BuilderConfigColumnSummary[]; + readonly columnTypeBreakdown: Array<{ type: string; count: number }>; + readonly models: BuilderConfigModelSummary[]; + readonly seed?: BuilderConfigSeedSummary; + readonly constraintCount: number; + readonly profilerCount: number; + readonly processorNames: string[]; + readonly libraryVersion?: string; }As per coding guidelines:
web/**/*.{ts,tsx}: Use \readonly` for immutable properties in TypeScript interfaces and types`.🤖 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/DataDesignerJobDetailsRoute/builderConfig.ts` around lines 17 - 44, Add the readonly keyword to all property declarations in the four summary interfaces (BuilderConfigColumnSummary, BuilderConfigModelSummary, BuilderConfigSeedSummary, and BuilderConfigSummary) to mark them as immutable at the type level. For each property in these interfaces, prefix the property name with the readonly keyword to ensure the data structures represent immutable snapshots that cannot be modified after creation.Source: Coding guidelines
web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsx (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
FCas type-only.
FCis only used in type positions here.Proposed diff
-import { FC, useMemo } from 'react'; +import { useMemo, type FC } from 'react';As per coding guidelines:
web/**/*.{ts,tsx}: Use \import type` for type-only imports in TypeScript`.🤖 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/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsx` at line 14, The import statement at the top of DataDesignerConfigPanel.tsx imports both FC and useMemo from 'react' using a single import statement. Since FC is only used in type positions (as a functional component type), it should be imported using import type syntax instead of regular import. Split the import statement so that FC is imported with import type while useMemo remains in a regular import statement.Source: Coding guidelines
web/packages/studio/src/routes/DataDesignerJobDetailsRoute/JobOutputFilesetSection.tsx (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert type-only React imports to
import type.
ComponentPropsandFCare type-only and should useimport typein this TSX file.Proposed fix
-import { ComponentProps, FC, useCallback, useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; +import type { ComponentProps, FC } from 'react';As per coding guidelines,
web/**/*.{ts,tsx}: Useimport typefor type-only imports in TypeScript.🤖 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/DataDesignerJobDetailsRoute/JobOutputFilesetSection.tsx` at line 20, In the import statement at the top of JobOutputFilesetSection.tsx, separate the type-only imports from the runtime imports. Move ComponentProps and FC to a new line using import type syntax, keeping useCallback, useEffect, useMemo, and useState in the regular import statement from React. This ensures type-only imports are clearly distinguished from runtime dependencies according to the project's TypeScript guidelines.Source: Coding guidelines
web/packages/studio/src/routes/DataDesignerJobDetailsRoute/index.tsx (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
import typeforFCin this TSX module.
FCis type-only and should be imported withimport typeto match the TypeScript guideline.Proposed fix
-import { FC, useState } from 'react'; +import { useState } from 'react'; +import type { FC } from 'react';As per coding guidelines,
web/**/*.{ts,tsx}: Useimport typefor type-only imports in TypeScript.🤖 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/DataDesignerJobDetailsRoute/index.tsx` at line 25, The import statement at the top of the DataDesignerJobDetailsRoute file imports FC as a type-only import along with useState in a single import statement. Separate this into two imports: create a type-only import for FC using import type, and keep a regular import for useState from react. This ensures that type-only imports are explicitly marked as such, following TypeScript guidelines that help with tree-shaking and code clarity.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/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsx`:
- Around line 59-63: The key prop for the mapped KVPair components uses only
model.alias and column.name respectively, which can collide when duplicate
aliases or names exist (such as multiple "(unnamed)" entries), causing React to
mis-reconcile elements. Replace the key prop in both the model mapping (around
line 59-63 in the summary.models.map) and the column mapping (around line 75-79)
with a stable unique identifier that includes both the alias/name and the array
index, or another property that ensures uniqueness across duplicates, to prevent
reconciliation issues.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DatasetProfilerSection.tsx`:
- Around line 57-67: The loading state condition in the first if statement is
checking hasAnalysis instead of the actual analysis data, which causes the
empty-state message to appear when hasAnalysis is true but the data is still
loading. Change the condition from checking !hasAnalysis to checking !analysis
(or checking if analysis is falsy/undefined) to properly gate the skeleton
loading state. This ensures the skeleton loader displays whenever data is
actually still loading, regardless of the hasAnalysis flag status. The fix
applies to the loading branch condition at the beginning of this component
logic.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/datasetProfilerTypes.ts`:
- Around line 148-153: The getPercentComplete function can return values above
100 or below 0 due to backend count drift, which breaks progress rendering in
consumers expecting valid percentage values. Clamp the calculated percentage
result to the range [0, 100] by wrapping the return statement with Math.max(0,
Math.min(100, ...)) to ensure the function always returns a value between 0 and
100 inclusive.
- Around line 136-137: The type predicate isLLMColumnStatistics narrows to only
LLMTextColumnStatistics, but the runtime check using LLM_COLUMN_TYPES actually
validates against four different LLM column types (llm-text, llm-code,
llm-structured, and llm-judge). Create a union type that includes all four LLM
column statistic variants, then update the return type of the
isLLMColumnStatistics predicate to use this union type instead of
LLMTextColumnStatistics to align the type guard with the actual runtime
behavior.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerArtifactsFileset.ts`:
- Around line 26-29: The useDataDesignerListCreateJobResults hook call is not
capturing error information from its response. Currently when the hook fails,
it's being treated identically to the case where there are simply no results,
causing incorrect error messages to display downstream. Extract the error state
that the hook returns (in addition to the data and isLoading states already
being destructured) and ensure this error state is used by downstream code to
properly render an error state instead of conflating it with empty results.
Apply this same fix to all usages of useDataDesignerListCreateJobResults
including the location at lines 74-89.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerJobAnalysis.ts`:
- Around line 33-36: The useDataDesignerJobAnalysis hook is not capturing errors
from the useDataDesignerListCreateJobResults query call. When the results
listing fails, the hook currently reports hasAnalysis as false and isError as
false, misclassifying backend failures as missing analysis. Extract the error
state from the useDataDesignerListCreateJobResults destructuring (similar to how
data and isLoading are extracted) and incorporate it into the hook's return
value so that actual backend errors are properly surfaced to consumers rather
than being silently dropped.
---
Nitpick comments:
In `@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/builderConfig.ts`:
- Around line 17-44: Add the readonly keyword to all property declarations in
the four summary interfaces (BuilderConfigColumnSummary,
BuilderConfigModelSummary, BuilderConfigSeedSummary, and BuilderConfigSummary)
to mark them as immutable at the type level. For each property in these
interfaces, prefix the property name with the readonly keyword to ensure the
data structures represent immutable snapshots that cannot be modified after
creation.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsx`:
- Line 14: The import statement at the top of DataDesignerConfigPanel.tsx
imports both FC and useMemo from 'react' using a single import statement. Since
FC is only used in type positions (as a functional component type), it should be
imported using import type syntax instead of regular import. Split the import
statement so that FC is imported with import type while useMemo remains in a
regular import statement.
In `@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/index.tsx`:
- Line 25: The import statement at the top of the DataDesignerJobDetailsRoute
file imports FC as a type-only import along with useState in a single import
statement. Separate this into two imports: create a type-only import for FC
using import type, and keep a regular import for useState from react. This
ensures that type-only imports are explicitly marked as such, following
TypeScript guidelines that help with tree-shaking and code clarity.
In
`@web/packages/studio/src/routes/DataDesignerJobDetailsRoute/JobOutputFilesetSection.tsx`:
- Line 20: In the import statement at the top of JobOutputFilesetSection.tsx,
separate the type-only imports from the runtime imports. Move ComponentProps and
FC to a new line using import type syntax, keeping useCallback, useEffect,
useMemo, and useState in the regular import statement from React. This ensures
type-only imports are clearly distinguished from runtime dependencies according
to the project's TypeScript guidelines.
🪄 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: 63a66446-24f0-4fc3-a33c-ef9c32f8b383
📒 Files selected for processing (14)
web/packages/studio/src/constants/constants.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/CategoricalHistogramChart.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/ColumnProfileCard.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/DataDesignerConfigPanel.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/DatasetProfilerSection.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/JobOutputFilesetSection.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/builderConfig.test.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/builderConfig.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/datasetProfilerTypes.test.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/datasetProfilerTypes.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/index.tsxweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerArtifactsFileset.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerJobAnalysis.tsweb/packages/studio/src/routes/DataDesignerJobDetailsRoute/useDataDesignerJobFromRoute.ts
Contributor
|
Signed-off-by: Sean Teramae <steramae@nvidia.com>
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
5 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.
Summary by CodeRabbit
Release Notes
New Features
Improvements
nvidia-llama-3-3-nemotron-super-49b-v1.Tests