-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Feature/condensing enhancements #3826
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
Feature/condensing enhancements #3826
Conversation
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
src/core/condense/index.ts
Outdated
| // Log when using custom prompt for debugging and telemetry | ||
| if (customCondensingPrompt?.trim()) { | ||
| console.log(`Task [${taskId}]: Using custom condensing prompt.`) | ||
| // TODO: Add telemetry for custom condensing prompt usage | ||
| // This would require extending the telemetry service with a new method like: | ||
| // telemetryService.captureCustomCondensingPromptUsed(taskId); | ||
| } | ||
|
|
||
| // Log when using custom API handler for condensing | ||
| if (condensingApiHandler) { | ||
| console.log(`Task [${taskId}]: Using custom API handler for condensing.`) | ||
| // TODO: Add telemetry for custom condensing API handler usage | ||
| // This would require extending the telemetry service with a new method like: | ||
| // telemetryService.captureCustomCondensingApiUsed(taskId, condensingApiConfigId); | ||
| } |
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.
Instead of this, how about we just add a couple new properties to telemetryService.captureContextCondensed(taskId, isAutomaticTrigger ?? false) above?
src/core/task/Task.ts
Outdated
| } = (await this.providerRef.deref()?.getState()) ?? {} | ||
|
|
||
| // Get condensing configuration for automatic triggers | ||
| const state = await this.providerRef.deref()?.getState() |
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.
Code might be a little cleaner if you put this at the top of the method const state = await this.providerRef.deref()?.getState() ?? {} and then just directly refer to state.customCondensingPrompt etc
| </div> | ||
| <VSCodeTextArea | ||
| resize="vertical" | ||
| value={customCondensingPrompt || ""} |
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.
I think this would be easier to use if the value defaulted to SUMMARY_PROMPT to make it obvious what the default is and to make it easier for people to make minor adjustments. In this case we wouldn't need a placeholder, and hitting the reset button would just populate with the default value.
mrubens
left a comment
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.
Awesome! Really excited about getting this live. Message me on Discord if you want to discuss any of the feedback. Thank you!
I've made the proposed changes. Please review them. |
Description
This pull request introduces significant enhancements to the context condensing feature, providing users with more control and flexibility. The changes that were implemented include:
condensingApiConfigId,customCondensingPrompt) were added and integrated into the extension's state management.summarizeConversation) and related task logic (Task.ts,sliding-window/index.ts) were updated to utilize these new settings, including validation and fallback mechanisms for the API handler.ExperimentalSettings.tsxalong with necessary i18n localization updates.src/core/condense/__tests__/index.test.tsand new integration tests were created ine2e/src/suite/condensing.test.tsto cover the new functionality.These changes allow for more tailored and potentially more effective context summarization based on user preferences and specific API capabilities.
Test Procedure
Automated Tests:
npm run test:unit src/core/condense/__tests__/index.test.tsnpm run test:e2e e2e/src/suite/condensing.test.ts(or the full e2e suite viapnpm test).pnpm testrun will provide final confirmation.Manual Testing Steps:
Type of Change
srcor test files.Pre-Submission Checklist
npm run lint/ pre-commit hooks).console.log) has been removed.npm test/pnpm test). (Currently running - will update this checkbox based on results)mainbranch. (Please verify before merging)npm run changesetif this PR includes user-facing changes or dependency updates. (Consider if this user-facing feature warrants a changeset:pnpm changeset)Screenshots / Videos
OLD
NEW
Documentation Updates
Information about the new configurable settings needs to be be added in the docs.
Additional Notes
Get in Touch
Important
Enhancements to context condensing feature include customizable API configurations and system prompts, updated backend state management, improved error handling, and comprehensive testing.
condensingApiConfigIdandcustomCondensingPromptadded to state management.summarizeConversationupdated to use new settings with validation and fallback.ExperimentalSettings.tsx.index.test.tsand integration tests incondensing.test.tsfor new functionality.This description was created by
for a0f0f6a. You can customize this summary. It will automatically update as commits are pushed.