-
Notifications
You must be signed in to change notification settings - Fork 532
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
Ai collab explicit #22836
base: main
Are you sure you want to change the base?
Ai collab explicit #22836
Conversation
…r one shared api surface
…to aiCollabExplicit
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 Coverage Summary
↑ packages.dds.tree.src.simple-tree:
Line Coverage Change: 0.07% Branch Coverage Change: 0.20%
Metric Name | Baseline coverage | PR coverage | Coverage Diff |
---|---|---|---|
Branch Coverage | 93.29% | 93.49% | ↑ 0.20% |
Line Coverage | 97.13% | 97.20% | ↑ 0.07% |
↑ packages.dds.tree.src.simple-tree.api:
Line Coverage Change: 0.01% Branch Coverage Change: 1.14%
Metric Name | Baseline coverage | PR coverage | Coverage Diff |
---|---|---|---|
Branch Coverage | 86.53% | 87.67% | ↑ 1.14% |
Line Coverage | 82.12% | 82.13% | ↑ 0.01% |
↑ packages.framework.ai-collab.src:
Line Coverage Change: 30.72% Branch Coverage Change: No change
Metric Name | Baseline coverage | PR coverage | Coverage Diff |
---|---|---|---|
Branch Coverage | 0.00% | 0.00% | → No change |
Line Coverage | 0.00% | 30.72% | ↑ 30.72% |
↑ packages.framework.ai-collab.src.explicit-strategy:
Line Coverage Change: 57.98% Branch Coverage Change: 75.00%
Metric Name | Baseline coverage | PR coverage | Coverage Diff |
---|---|---|---|
Branch Coverage | 0.00% | 75.00% | ↑ 75.00% |
Line Coverage | 0.00% | 57.98% | ↑ 57.98% |
↑ packages.framework.ai-collab.src.explicit-strategy:
Line Coverage Change: 57.98% Branch Coverage Change: 75.00%
Metric Name | Baseline coverage | PR coverage | Coverage Diff |
---|---|---|---|
Branch Coverage | 0.00% | 75.00% | ↑ 75.00% |
Line Coverage | 0.00% | 57.98% | ↑ 57.98% |
Baseline commit: 199b9d0
Baseline build: 302875
Happy Coding!!
Code coverage comparison check passed!!
…n exported from tree
…logic because its not in use.
*/ | ||
|
||
/** | ||
* TBD |
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.
@taylorsw04 @noencke Suggestions for this? (and others?)
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 want to make sure these get documented before we merge into main, otherwise they'll probably be lost 🫤
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've given a go at adding jsdoc for these types. I'd like the SharedTree team to still review and confirm the jsdoc. There are also some remarks asking what different thing are.
*/ | ||
export const typeField = "__fluid_type"; | ||
/** | ||
* TBD |
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.
This is auto-generated and injected into nodes before passing data to the LLM to ensure the LLM can refer to nodes in a stable way.
import type { JsonPrimitive } from "./jsonTypes.js"; | ||
|
||
/** | ||
* TODO: The current scheme does not allow manipulation of arrays of primitive values because you cannot refer to them. |
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.
We should probably file follow-up tasks for many of these. I suspect most/all aren't blockers for the alpha release, but they should still be on our radar. Some will certainly be blocked on missing SharedTree functionality - identifying those blockers would be useful.
@taylorsw04 maybe you can help point us to the relevant backlog items that would be blockers for these.
packages/framework/ai-collab/src/explicit-strategy/idGenerator.ts
Outdated
Show resolved
Hide resolved
packages/framework/ai-collab/src/explicit-strategy/idGenerator.ts
Outdated
Show resolved
Hide resolved
this.assignIds(element); | ||
}); | ||
} else { | ||
// TODO: SharedTree Team needs to either publish TreeNode as a class to use .instanceof() or a typeguard. |
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.
@taylorsw04 are you guys tracking this?
import { generateGenericEditTypes } from "./typeGeneration.js"; | ||
import { fail } from "./utils.js"; | ||
|
||
const DEBUG_LOG: string[] = []; |
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.
Probably best to remove this?
|
||
const isRootNode = Tree.parent(options.treeNode) === undefined; | ||
const simpleSchema = isRootNode | ||
? getSimpleSchema(normalizeFieldSchema(options.treeView.schema).allowedTypes) |
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 believe the following should work and is a bit simpler:
? getSimpleSchema(normalizeFieldSchema(options.treeView.schema).allowedTypes) | |
? getSimpleSchema(options.treeView.schema) |
export interface GenerateTreeEditsOptions<TSchema extends ImplicitFieldSchema> { | ||
openAI: OpenAiClientOptions; | ||
treeView: TreeView<TSchema>; | ||
treeNode: TreeNode; |
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.
It would be good to document how treeNode
and treeView
relate here. Is treeView
needed?
const schema = isRootNode | ||
? normalizeFieldSchema(view.schema) | ||
: normalizeFieldSchema(Tree.schema(treeNode)); | ||
const promptFriendlySchema = getPromptFriendlyTreeSchema(getJsonSchema(schema.allowedTypes)); |
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 believe you should be able to simplify this to
const promptFriendlySchema = getPromptFriendlyTreeSchema(getJsonSchema(schema.allowedTypes)); | |
const promptFriendlySchema = getPromptFriendlyTreeSchema(getJsonSchema(schema)); |
} | ||
|
||
/** | ||
* TBD |
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.
@noencke @taylorsw04 recommendations?
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.
(and for the others)
packages/framework/ai-collab/src/explicit-strategy/typeGeneration.ts
Outdated
Show resolved
Hide resolved
* Licensed under the MIT License. | ||
*/ | ||
|
||
/** |
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.
Do we have anything tracking moving these utilities to a shared location? fail
should probably live next to our assert
. The map utilities could probably go in core-utils
.
@noencke thoughts?
…rom integ test, adds baseline comments to agentEditTypes.ts
…ailure error message as intented to generateTreeEdits
I've removed |
numberSchema, | ||
stringSchema, | ||
booleanSchema, | ||
handleSchema, | ||
nullSchema, |
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.
Are these needed? Can we not use the schemaFactory.number
, etc. pattern? Not super concerned about exposing these as @internal
, just wondering if it's strictly necessary.
Adds the Explicit Strategy to the ai-collab package, exported under a new, shared API surface.
Description
The following PR implements the explicit strategy to the ai-collab package and exports it under a new, simple API surface. (See
aiCollabApi.ts
).Folder Structure
/explicit-strategy
: The new explicit strategy, utilizing the prototype built during the fall FHL, with a few adjustments.agentEditReducer
: This file houses the logic for taking in aTreeEdit
, which the LLM produces, and applying said edit to theagentEditTypes.ts
: The types of edits an LLM is prompted to produce in order to modify a SharedTree.idGenerator.ts
: `A manager for producing and mapping simple id's in place of UUID hashes when generating prompts for an LLMjsonTypes.ts
: utility JSON related types used in parsing LLM response and generating LLM prompts.promptGeneration.ts
: Logic for producing the different types of prompts sent to an LLM in order to edit a SharedTree.typeGeneration.ts
: Generates serialized(/able) representations of a SharedTree Schema which is used within prompts and the generated of the structured output JSON schemautils.ts
: Utilities for interacting with a SharedTree/implicit-strategy
: (... original implicit strategy code, unmodified and move to a new subfolder)You'll see that the ai-collab package has been restructured to contain
explicit-strategy
andimplicit-strategy
subfolders while the root has theaiCollab.ts
file which is the actual exported API surface of this package.There are many new
@internal
exports that needed to be exposed from SharedTree to get this package working properly.In a follow up PR, all exports from
implicit-strategy/index.js
will be removed. They are left in place to minimize the blast radius of this PR (which involves editing the ai sample application to use the new code.Usage
Your SharedTree types file
This file is where we define the types of our task management application's SharedTree data
Example 1: Collaborate with AI
Once the
aiCollab
function call is initiated, an LLM will immediately begin attempting to make changes to your Shared Tree using the provided user prompt, the types of your SharedTree and the provided app guidance. The LLM produces multiple changes, in a loop asynchronously. Meaning, you will immediatley see changes if your UI's render loop is connected to your SharedTree App State.How the explicit strategy has changed since FHL
The logic remains intact with small adjustments:
generateTreeEdits
and related API's have been slightly adjusted, with the only logic change being they now accepts a TreeNode instead of the entire tree, allowing the LLM to work on subtree's and omitting data from parts of the tree that are not intended to be worked on. This also means that in places where the tree's schema must be accessed, there is branching logic based on whether the root or a child node is passed to the function, which determines whether root tree's schema needs to be generated or just the schema of the tree node.Known Issues & limitations
Reviewer Guidance
explicit-strategy
folder code.aiCollabApi.ts