-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Data Engineer mode with interactions with the active Notebook #1729
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
Conversation
|
|
This pull request introduces a significant number of changes across multiple files, primarily related to notebook tool support, descriptions, and settings, as well as a new NotebookService class and various configurations for notebook operations. Given the size and scope of the changes, it might be beneficial to split the pull request into smaller, logical sections to improve reviewability. Consider splitting the changes into:
This approach could help reviewers focus on specific aspects of the feature and ensure a more thorough review process. |
src/services/notebook/index.ts
Outdated
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.
The error message on line 332 mentions 'cell_index' rather than 'cellIndex'. For clarity and consistency, consider updating the message.
| throw new Error("cell_index is required for executing a cell.") | |
| throw new Error("cellIndex is required for executing a cell.") |
|
cool |
77bfbb0 to
89a6612
Compare
|
thanks to Sonnet MAX! now rebased on 3.9, and UI solved 😃, with i18n (en+zh) set to optimize some wrt workflow |
|
Myself roughly satisfied with what's done so far, rebased & squashed for ease of review. Now it automatically execute new/modified cells upon notebook edit. A flaw I encounter is that even though I wrote the prompts trying to allow multiple cells inserted/replaced in a single tool call, currently the framework wouldn't allow multiple What's your suggestions? |
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.
Typo on line 5: 'unless the <noexec/> parameter presents.' should be corrected to 'unless the <noexec/> parameter is present.'
| Description: Edit the active notebook in the editor. This tool allows you to insert new cells or replace existing cells. Note that new/modified code cells will be executed immediately, by default, unless the <noexec/> parameter presents. | |
| Description: Edit the active notebook in the editor. This tool allows you to insert new cells or replace existing cells. Note that new/modified code cells will be executed immediately, by default, unless the <noexec/> parameter is present. |
src/shared/ExtensionMessage.ts
Outdated
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.
Naming Convention Issue: In the ClineSayTool interface, properties such as cell_content, cell_index, start_index, end_index, cell_type, and language_id are written in snake_case, whereas most other properties in the file use camelCase. Consider renaming them to cellContent, cellIndex, startIndex, endIndex, cellType, and languageId for consistency.
|
The pull request is quite large, with 29 files changed and over 1800 lines added. It includes various changes such as adding notebook functionalities, tool interfaces, descriptions, configuration options, and localization strings. To improve the review process, it might be beneficial to split this pull request into smaller, more focused ones. Here are some suggestions on how it could be split:
This will help reviewers focus on specific areas and ensure a more thorough review process. Please consider splitting the pull request if these changes are not tightly coupled. |
|
@complyue cool, is it ready to review |
|
@samhvw8 yes, please review! |
f7d4572 to
c6033fb
Compare
Co-authored-by: fine <[email protected]>
b96f46b to
04f5e0e
Compare
|
Hey @complyue. This seems like a big change and we would like to have an issue opened first with an use-case for these changes to properly discuss it. We are closing this PR but feel free to create an issue to revisit this. We've now shifted to a clearer issue-first workflow to avoid this kind of situation going forward. Please create an issue first for any future contributions, as outlined there. We truly appreciate your patience and would be happy to have you continue contributing. |

Context
Adding a [Data Engineer] mode here, for Roo to work on the active Notebook together with the user.
Implementation
Tool functions are added to read/edit/exec cells in the active notebook.
Update 2:
Myself roughly satisfied with what's done so far, rebased & squashed for ease of review. Now it automatically execute new/modified cells upon notebook edit.
A flaw I encounter is that even though I wrote the prompts trying to allow multiple cells inserted/replaced in a single tool call, currently the framework wouldn't allow multiple
<cell_content/>args to be passed around. It works well so long as the agent does insert/modify/replace a single cell at a time.What's your suggestions?
Update:
Sonnet MAX saved the day! Now the UI works as expected 😄
Yet I fail to make the tool usage approving UI show up correctly, tho blindly approval works, neither auto-approval works. Also added 2 new settings to truncate huge nb cell outputs, as well as config nb exec timeout, failed to make them show up in settings UI either.Further enhancement may include allowing modify + exec the notebook in a single round, I'm not sure the best approach.
I'm open to ideas around this feature.
Please review!
Screenshots
How to Test
Switch to [Data Engineer] mode, ask Roo to read/edit/exec the active notebook with any goal.
Get in Touch
I prefer GH messages.
Important
Introduces Data Engineer mode for notebook interaction, with settings for output size and execution timeout, and updates localization and state management.
ChatView.tsx.NotebookSettings.tsxfor configuring output size limit and execution timeout.SettingsView.tsxto include notebook settings.ExtensionStateContext.tsxto manage new notebook-related states.ExtensionStateContext.test.tsxfor new state management.This description was created by
for db8f194c3a634d2f89e2d719dab6e6c043624b75. It will automatically update as commits are pushed.