Synth data goal #422
Synth data goal #422
Conversation
WalkthroughA new Intro UI component was added, and the Splits Svelte component was removed. Splits parsing and subtitle logic were refactored into a utility module, with affected pages updated to use these utilities instead of the Splits UI. Synthetic data generation flows were made more interactive with dialogs, API integration, and improved state management. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DataGenIntro
participant API
participant GuidanceDataModel
User->>DataGenIntro: Click "Generate Synthetic Eval Data"
DataGenIntro->>API: Fetch evals
API-->>DataGenIntro: Return evals list
User->>DataGenIntro: Select eval
DataGenIntro->>GuidanceDataModel: on_setup("eval", template_id, eval_id, project_id, task_id, splits)
GuidanceDataModel->>GuidanceDataModel: load(..., splits)
sequenceDiagram
participant User
participant DataGenIntro
participant API
participant GuidanceDataModel
User->>DataGenIntro: Click "Generate Fine-tuning Data"
DataGenIntro->>API: Fetch fine-tune dataset info
API-->>DataGenIntro: Return dataset info
User->>DataGenIntro: Select tag (if multiple)
DataGenIntro->>GuidanceDataModel: on_setup("training", template_id, null, project_id, task_id, splits)
GuidanceDataModel->>GuidanceDataModel: load(..., splits)
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
app/web_ui/src/routes/(app)/+layout.svelte (1)
220-267: Replace hard-coded SVG stroke color to honor UI themesThe new “Evals” icon uses
stroke="#1C274C"while other icons rely oncurrentColor. The hard-coded hex overrides DaisyUI / Tailwind theme colors and will look out of place in dark-mode or custom themes.- stroke="#1C274C" + stroke="currentColor"Apply the same change to all four occurrences inside this icon to keep styling consistent and theme-aware.
app/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/add_data/+page.svelte (1)
18-23: Consider removing unnecessary tick() callThe
tick()call appears unnecessary here since you're only reading from$page.url.searchParams, which doesn't require waiting for DOM updates.onMount(async () => { - await tick() const splitsParam = $page.url.searchParams.get("splits") splits = get_splits_from_url_param(splitsParam) splits_subtitle = get_splits_subtitle(splits) })app/web_ui/src/lib/ui/intro.svelte (1)
5-11: Consider making href and onClick mutually exclusiveThe current ActionButton type allows both
hrefandonClickto be defined simultaneously, which could lead to confusion about which action takes precedence.Consider using a discriminated union type:
type ActionButton = { label: string - href?: string - new_tab?: boolean - onClick?: () => void is_primary: boolean + } & ( + | { href: string; new_tab?: boolean } + | { onClick: () => void } + )app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/+page.svelte (1)
117-142: Consider stronger error handling for duplicate setup calls.While the guard against multiple setup calls is good, using only
console.errormight not be sufficient. Consider:
- Throwing an error or showing a user-visible warning
- Logging this to error tracking if available
- Returning early to prevent any side effects
function setup( gen_type: "training" | "eval", template_id: string | null, eval_id: string | null, project_id: string, task_id: string, splits: Record<string, number>, ) { if (!gen_type || !task) { return } if (is_setup) { - console.error("Setup already called. This should not happen.") + const error = new Error("Setup already called. This should not happen.") + console.error(error) + // Consider: task_error = createKilnError(error) + return } is_setup = true guidance_data.load( template_id, eval_id, project_id, task_id, gen_type, task, splits, ) splits_subtitle = get_splits_subtitle(splits) }app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/data_gen_intro.svelte (1)
219-223: Consider responsive design for grid layout.The grid uses fixed pixel widths (
270px) which may cause issues on smaller screens.<div class="grid grid-cols-2 gap-x-32 gap-y-4 items-start font-light text-sm" - style="grid-template-columns: 270px 270px;" + style="grid-template-columns: repeat(2, minmax(270px, 1fr));" >Alternatively, consider using Tailwind's responsive grid classes:
-<div - class="grid grid-cols-2 gap-x-32 gap-y-4 items-start font-light text-sm" - style="grid-template-columns: 270px 270px;" -> +<div class="grid grid-cols-1 md:grid-cols-2 gap-x-8 md:gap-x-32 gap-y-4 items-start font-light text-sm max-w-[600px]">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/web_ui/src/lib/ui/intro.svelte(1 hunks)app/web_ui/src/lib/ui/splits.svelte(0 hunks)app/web_ui/src/lib/utils/splits_util.ts(1 hunks)app/web_ui/src/routes/(app)/+layout.svelte(1 hunks)app/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/add_data/+page.svelte(1 hunks)app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/+page.svelte(6 hunks)app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/data_gen_intro.svelte(1 hunks)app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/synth_data_guidance_datamodel.test.ts(7 hunks)app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/synth_data_guidance_datamodel.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/web_ui/src/lib/ui/splits.svelte
🧰 Additional context used
🧠 Learnings (2)
app/web_ui/src/lib/utils/splits_util.ts (1)
Learnt from: scosman
PR: Kiln-AI/Kiln#296
File: libs/core/kiln_ai/utils/dataset_import.py:116-152
Timestamp: 2025-05-09T17:33:29.787Z
Learning: The tag_splits implementation in dataset_import.py uses deterministic allocation via integer division with remaining items assigned to the largest split, which is preferable over probability-based approaches to guarantee no splits are starved, especially with small datasets.
app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/+page.svelte (1)
Learnt from: scosman
PR: Kiln-AI/Kiln#296
File: libs/core/kiln_ai/utils/dataset_import.py:116-152
Timestamp: 2025-05-09T17:33:29.787Z
Learning: The tag_splits implementation in dataset_import.py uses deterministic allocation via integer division with remaining items assigned to the largest split, which is preferable over probability-based approaches to guarantee no splits are starved, especially with small datasets.
🔇 Additional comments (16)
app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/synth_data_guidance_datamodel.test.ts (1)
90-90: LGTM: Test method signature updates are consistentAll test calls to the
loadmethod have been correctly updated to include the newsplitsparameter with appropriate default values. The changes maintain existing test behavior while accommodating the updated API.Also applies to: 99-107, 113-121, 143-151, 180-188, 206-214, 225-233
app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/synth_data_guidance_datamodel.ts (2)
22-22: LGTM: Clean addition of splits propertyThe new
splitsproperty is properly typed and initialized with a sensible default value.
61-61: LGTM: Method signature and assignment are correctThe
loadmethod signature is properly updated to include the splits parameter, and the assignment correctly stores the splits data in the instance property.Also applies to: 68-68
app/web_ui/src/routes/(app)/dataset/[project_id]/[task_id]/add_data/+page.svelte (1)
9-12: LGTM: Clean import transition to utility functionsThe import change properly replaces the Splits UI component with the new utility functions, aligning with the refactoring objectives.
app/web_ui/src/lib/utils/splits_util.ts (2)
1-28: LGTM: Robust parsing with proper validationThe
get_splits_from_url_paramfunction implements comprehensive validation including:
- Range validation (0-1) for split values
- Sum validation with appropriate floating-point tolerance
- Graceful error handling with console warnings
The error handling approach of returning an empty object is appropriate for URL parameter parsing.
30-35: LGTM: Clean formatting functionThe
get_splits_subtitlefunction provides a clear, human-readable format for displaying split percentages. The conditional return for empty splits is well-handled.app/web_ui/src/lib/ui/intro.svelte (1)
16-49: LGTM: Well-structured reusable componentThe component provides a clean, flexible interface for intro screens with proper slot usage and conditional button rendering. The styling is consistent and the layout is well-organized.
app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/+page.svelte (5)
22-25: LGTM! Good modularization of split utilities.Moving split parsing logic to a dedicated utility module improves maintainability and reusability.
49-49: Verify setup state initialization.The
is_setupflag is initialized tofalse, which is correct. However, ensure that the setup process is idempotent in case of component re-mounts or navigation edge cases.
103-115: Well-structured URL parameter handling.The implementation correctly:
- Checks for required
reasonparameter- Handles optional parameters (eval_id, template_id, splits)
- Uses the utility function for split parsing
- Triggers setup only for valid generation types
306-307: Good integration of split tag selection.The implementation correctly uses the
get_random_split_tagfunction to assign split tags when saving samples, with proper handling of undefined returns.
410-411: Clean component integration with setup flow.The binding of
on_setupcallback andis_setupstate to the DataGenIntro component is well-implemented, enabling proper communication between parent and child components.app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/data_gen_intro.svelte (4)
2-6: LGTM! Well-organized imports.Good separation of concerns with proper imports for UI components, API client, error handling, and types.
12-23: Well-designed component interface.The export interface properly:
- Maintains backward compatibility with optional callback
- Provides comprehensive type safety for the setup callback
- Uses proper two-way binding for
is_setupstate
86-149: Excellent implementation of fine-tuning data flow.The implementation has several good design choices:
- Auto-triggers when only the default tag exists, improving UX
- Properly sorts tags by count for better visibility
- Always includes the default
fine_tune_datatag- Clean error handling pattern
385-481: Well-structured dialog implementations.Both dialogs demonstrate excellent UX patterns:
- Clear loading states with spinners
- User-friendly error messages
- Intuitive card-based selection for evals
- Smart button styling to highlight recommended options
- Proper empty state handling
| function get_random_split_tag() { | ||
| const splits = guidance_data.splits | ||
| if (Object.keys(splits).length === 0) return undefined | ||
|
|
||
| const random = Math.random() | ||
| let cumulative = 0 | ||
|
|
||
| for (const [tag, probability] of Object.entries(splits)) { | ||
| cumulative += probability | ||
| if (random <= cumulative) { | ||
| return tag | ||
| } | ||
| } | ||
|
|
||
| // Fallback (should never reach here if splits sum to 1) | ||
| return Object.keys(splits)[0] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for split probabilities sum.
The weighted random selection implementation is correct, but it assumes splits sum to 1.0. If they don't, the selection might be biased.
function get_random_split_tag() {
const splits = guidance_data.splits
if (Object.keys(splits).length === 0) return undefined
+ // Validate that splits sum to approximately 1.0
+ const sum = Object.values(splits).reduce((a, b) => a + b, 0)
+ if (Math.abs(sum - 1.0) > 0.001) {
+ console.warn(`Split probabilities sum to ${sum}, expected 1.0`)
+ }
+
const random = Math.random()
let cumulative = 0
for (const [tag, probability] of Object.entries(splits)) {
cumulative += probability
if (random <= cumulative) {
return tag
}
}
// Fallback (should never reach here if splits sum to 1)
return Object.keys(splits)[0]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function get_random_split_tag() { | |
| const splits = guidance_data.splits | |
| if (Object.keys(splits).length === 0) return undefined | |
| const random = Math.random() | |
| let cumulative = 0 | |
| for (const [tag, probability] of Object.entries(splits)) { | |
| cumulative += probability | |
| if (random <= cumulative) { | |
| return tag | |
| } | |
| } | |
| // Fallback (should never reach here if splits sum to 1) | |
| return Object.keys(splits)[0] | |
| } | |
| function get_random_split_tag() { | |
| const splits = guidance_data.splits | |
| if (Object.keys(splits).length === 0) return undefined | |
| // Validate that splits sum to approximately 1.0 | |
| const sum = Object.values(splits).reduce((a, b) => a + b, 0) | |
| if (Math.abs(sum - 1.0) > 0.001) { | |
| console.warn(`Split probabilities sum to ${sum}, expected 1.0`) | |
| } | |
| const random = Math.random() | |
| let cumulative = 0 | |
| for (const [tag, probability] of Object.entries(splits)) { | |
| cumulative += probability | |
| if (random <= cumulative) { | |
| return tag | |
| } | |
| } | |
| // Fallback (should never reach here if splits sum to 1) | |
| return Object.keys(splits)[0] | |
| } |
🤖 Prompt for AI Agents
In app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/+page.svelte
around lines 355 to 371, the function get_random_split_tag assumes that the
probabilities in splits sum to 1. Add validation to check if the sum of all
split probabilities is close to 1 before performing the weighted random
selection. If the sum is not close to 1, log a warning or handle the error
appropriately to avoid biased selection.
| function select_eval(evaluator: Eval) { | ||
| const eval_set_filter_id = evaluator.eval_set_filter_id | ||
| const eval_configs_filter_id = evaluator.eval_configs_filter_id | ||
| const splits: Record<string, number> = {} | ||
| if ( | ||
| eval_set_filter_id.startsWith("tag::") && | ||
| eval_configs_filter_id.startsWith("tag::") | ||
| ) { | ||
| const eval_set_tag = eval_set_filter_id.split("::")[1] | ||
| const eval_configs_tag = eval_configs_filter_id.split("::")[1] | ||
| splits[eval_set_tag] = 0.8 | ||
| splits[eval_configs_tag] = 0.2 | ||
| } else { | ||
| alert( | ||
| "We can't generate synthetic data for this eval as it's eval sets are not defined by tag filters. Select an eval which uses tags to define eval sets.", | ||
| ) | ||
| return | ||
| } | ||
| const eval_id = project_id + "::" + task_id + "::" + (evaluator.id ?? "") | ||
| const template_id = evaluator.template ?? null | ||
|
|
||
| on_setup?.("eval", template_id, eval_id, project_id, task_id, splits) | ||
|
|
||
| evals_dialog?.close() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve UX for non-tag-based eval handling.
The current implementation uses alert() for non-tag-based evals, which provides poor UX. Also, the 80/20 split ratio is hardcoded.
function select_eval(evaluator: Eval) {
const eval_set_filter_id = evaluator.eval_set_filter_id
const eval_configs_filter_id = evaluator.eval_configs_filter_id
const splits: Record<string, number> = {}
if (
eval_set_filter_id.startswith("tag::") &&
eval_configs_filter_id.startsWith("tag::")
) {
const eval_set_tag = eval_set_filter_id.split("::")[1]
const eval_configs_tag = eval_configs_filter_id.split("::")[1]
- splits[eval_set_tag] = 0.8
- splits[eval_configs_tag] = 0.2
+ // Consider making these configurable or document why 80/20
+ const EVAL_SET_RATIO = 0.8
+ const EVAL_CONFIG_RATIO = 0.2
+ splits[eval_set_tag] = EVAL_SET_RATIO
+ splits[eval_configs_tag] = EVAL_CONFIG_RATIO
} else {
- alert(
- "We can't generate synthetic data for this eval as it's eval sets are not defined by tag filters. Select an eval which uses tags to define eval sets.",
- )
+ // Show inline error message instead of alert
+ evals_error = new KilnError(
+ "We can't generate synthetic data for this eval as it's eval sets are not defined by tag filters. Select an eval which uses tags to define eval sets."
+ )
return
}
const eval_id = project_id + "::" + task_id + "::" + (evaluator.id ?? "")
const template_id = evaluator.template ?? null
on_setup?.("eval", template_id, eval_id, project_id, task_id, splits)
evals_dialog?.close()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
app/web_ui/src/routes/(app)/generate/[project_id]/[task_id]/data_gen_intro.svelte
between lines 60 and 84, replace the alert() call for non-tag-based evals with a
more user-friendly UI notification or modal to improve UX. Additionally, remove
the hardcoded 80/20 split ratio and instead make the split values configurable
or derived dynamically to allow flexibility.
Require the user specify a goal before starting synth data gen.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests