-
Notifications
You must be signed in to change notification settings - Fork 668
feat(model): add top p generation setting #1680
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
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # Implementation Plan | ||
|
|
||
| ## Approach | ||
|
|
||
| Implement `topP` as an optional generation setting, mirroring the existing session generation settings pipeline while avoiding a forced default. | ||
|
|
||
| ## Affected Interfaces | ||
|
|
||
| - `src/shared/types/agent-interface.d.ts`: add optional `topP` to `SessionGenerationSettings`. | ||
| - `src/shared/contracts/common.ts`: add optional `topP` to route schemas. | ||
| - `src/shared/utils/generationSettingsValidation.ts`: validate `topP` as a finite number in `[0.1, 1]`. | ||
| - `src/main/presenter/sqlitePresenter/tables/deepchatSessions.ts`: add `top_p` storage and migration. | ||
| - `src/main/presenter/agentRuntimePresenter/index.ts`: sanitize, persist, and pass `topP` through runtime model config. | ||
| - `src/main/presenter/llmProviderPresenter/aiSdk/runtime.ts`: include `topP` in AI SDK `generateText` and `streamText` calls and request traces when defined. | ||
| - `src/renderer/src/stores/ui/draft.ts`: carry draft `topP` overrides for new sessions. | ||
| - `src/renderer/src/components/chat/ChatStatusBar.vue`: show and persist the compact `topP` control. | ||
| - i18n `chat.json` and `settings.json` files: add `topP` label, hover description, and validation. | ||
| - `topP` number inputs use min `0.1`, max `1`, and step `0.1` so values align with common AI SDK/provider constraints. | ||
|
|
||
| ## Data Flow | ||
|
|
||
| 1. User edits `topP` in ChatStatusBar. | ||
| 2. Draft sessions store it in Pinia; active sessions call `sessions.updateGenerationSettings`. | ||
| 3. Main process validates and stores `topP` as part of session generation settings. | ||
| 4. Agent runtime adds `topP` to runtime `ModelConfig`. | ||
| 5. AI SDK runtime includes `topP` only when defined. | ||
|
|
||
| ## Compatibility | ||
|
|
||
| - Existing databases migrate by adding nullable `top_p`. | ||
| - Existing sessions return no `topP` unless previously set. | ||
| - Requests without `topP` preserve current behavior. | ||
|
|
||
| ## Test Strategy | ||
|
|
||
| - Run formatting and i18n generation required by repository guidelines. | ||
| - Run lint to catch type/schema/template issues. | ||
| - Prefer focused type/lint validation over provider integration tests because this is a pass-through parameter. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # Model Top P Settings | ||
|
|
||
| ## User Need | ||
|
|
||
| Users need to adjust `top_p` for chat models because many upstream model APIs support nucleus sampling and expose it as a generation parameter. | ||
|
|
||
| ## Goal | ||
|
|
||
| Add an optional per-session `topP` generation setting for text chat requests and pass it through to AI SDK text generation when the user explicitly sets it. | ||
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| - Users can set `topP` from the chat model advanced settings panel for regular text chat models. | ||
| - `topP` accepts values greater than or equal to 0.1 and less than or equal to 1. | ||
| - The Top P UI uses the plain label "Top P" and moves explanatory copy into a hover help icon. | ||
| - Existing conversations and new sessions continue to work when no `topP` is set. | ||
| - `topP` persists with DeepChat session generation settings and survives app restart. | ||
| - Text `generateText` and streaming requests pass `topP` to AI SDK only when it is defined. | ||
| - Existing Voice.ai TTS `topP` configuration remains independent. | ||
|
|
||
| ## Constraints | ||
|
|
||
| - Follow current typed route/contracts and presenter boundaries. | ||
| - Use `topP` internally and let SDK/provider layers map provider payload details. | ||
| - Do not default-send `topP: 1`; omit the parameter when unset to preserve provider defaults. | ||
| - Use i18n keys for all user-facing strings. | ||
| - SQLite schema migration must be backward compatible. | ||
|
|
||
| ## Non-Goals | ||
|
|
||
| - Provider-specific `top_p` compatibility matrices. | ||
| - Building a full Provider DB `top_p` capability matrix. | ||
|
|
||
| ## Open Questions | ||
|
|
||
| - None. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| # Tasks | ||
|
|
||
| - [x] Create SDD artifacts for optional model `topP` setting. | ||
| - [x] Add shared types, contracts, and validation support for `topP`. | ||
| - [x] Add SQLite session persistence and migration for `topP`. | ||
| - [x] Propagate `topP` through runtime model config and AI SDK calls. | ||
| - [x] Add renderer draft state and advanced settings UI for `topP`. | ||
| - [x] Add i18n strings for `topP` controls and validation. | ||
| - [x] Refine Top P UI to use a help tooltip and `[0.1, 1]` number input range. | ||
| - [x] Run `pnpm run format`, `pnpm run i18n`, and `pnpm run lint`. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| # Plan | ||
|
|
||
| ## Approach | ||
|
|
||
| The current nested `Tooltip` is fragile inside the model settings `Popover` and can render beneath or outside the visible stacking context. Replace it with a controlled lightweight hover/focus panel anchored next to the Top P label. This keeps tooltip-style interaction while avoiding portal stacking conflicts. | ||
|
|
||
| ## Affected Interfaces | ||
|
|
||
| - `src/renderer/src/components/chat/ChatStatusBar.vue` | ||
|
|
||
| ## Data Flow | ||
|
|
||
| No data flow changes. The same `chat.advancedSettings.topPDescription` i18n string is displayed. | ||
|
|
||
| ## Compatibility | ||
|
|
||
| No persisted configuration changes. The chat settings Top P clamp behavior remains unchanged. | ||
|
|
||
| ## Test Strategy | ||
|
|
||
| - Static verification of template/script changes. | ||
| - Run project formatting/lint commands if package tooling is available. | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| # Chat Top P Tooltip | ||
|
|
||
| ## User Need | ||
|
|
||
| The chat settings panel Top P help icon must show explanatory text reliably. The settings dialog should keep inline description text, while the chat status bar should keep a hover tooltip interaction. | ||
|
|
||
| ## Goal | ||
|
|
||
| Fix the Top P help content in `ChatStatusBar.vue` so it is visible above the settings popover and has a readable width. | ||
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| - Hovering the Top P help icon in the chat settings panel shows the Top P description. | ||
| - The tooltip is not hidden behind the model settings popover. | ||
| - The tooltip content has a readable constrained width for the long description. | ||
| - The settings dialog Top P description remains inline text. | ||
| - The existing Top P range clamp behavior remains aligned between chat and settings pages. | ||
|
|
||
| ## Constraints | ||
|
|
||
| - Keep changes focused to the chat settings Top P help behavior. | ||
| - Preserve existing i18n keys and shadcn/reka UI styling patterns where practical. | ||
| - Do not introduce new dependencies. | ||
|
|
||
| ## Non-goals | ||
|
|
||
| - Redesign the full chat settings panel. | ||
| - Change Top P semantics or persisted data shape. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| # Tasks | ||
|
|
||
| - [x] Inspect current Top P tooltip implementation and popover stacking. | ||
| - [x] Add a layer-safe tooltip-style hover/focus panel for Top P in chat settings. | ||
| - [ ] Verify formatting and report any unavailable validation tooling. |
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plan text is stale relative to this PR’s Top P behavior.
Line 17 says clamp behavior is unchanged, but this PR also tightens Top P min/clamp behavior in
ChatStatusBar.vue(e.g., min set to0.1and commit-time clamping). Please update the plan so reviewers/testers aren’t working from incorrect assumptions.🤖 Prompt for AI Agents