[13/n] Add output CRUD operations to AIConfig #14
Conversation
This file contains 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
Ports over parameterization handling for AIConfig. Still some work left around getting outputs properly -- as well as tests, which will come in upcoming diffs
Next will add stubs for ModelParser. Then we can begin implementing
…lass Still getting library structure set up in TS
Used the executeCellWithDependencies implementation, which should be consistent with this.
Handles both chat and non-chat models (two separate model parsers for those). Handles parametrization properly across system prompt and previous messages. Next will add run methods for OpenAI which handle streaming and nonstreaming.
Some minor API design changes to help with returning streaming results back. Also implemented the run function for OpenAI models, handling both streaming and non-streaming scenarios. Added a callback handler to support sending incremental updates back to the caller.
Add implementation for all CRUD operations. Next up is tests and some additional APIs for parameterization.
Still WIP
This allows each model parser to determine how to parse an Output object as a string value.
* Add some CRUD operations to be able to interact with AIConfig Output objects more easily * Also some minor refactorings to consolidate output handling in the AIConfigRuntime class.
This was referenced Oct 9, 2023
rholinshead
reviewed
Oct 13, 2023
| ); | ||
| } | ||
|
|
||
| const existingOutputs = [...(prompt.outputs || [])]; |
There was a problem hiding this comment.
nit, it seems a bit weird to return the outputs that are deleted; it seems to set an expectation that they can be operated on, but that seems like it could lead to issues
rholinshead
reviewed
Oct 13, 2023
| * @param promptName The name of the prompt whose outputs to delete. | ||
| * @returns The outputs that were deleted from the prompt. | ||
| */ | ||
| public deleteOutput(promptName: string) { |
There was a problem hiding this comment.
Singular name makes it sound like it will only delete 1 output (e.g. latest). Should probably be deleteOutputs. Can consider adding a param to specify latest-only
rholinshead
reviewed
Oct 13, 2023
| prompt = this.getPrompt(prompt)!; | ||
| if (!prompt) { | ||
| throw new Error( | ||
| `E1029: Cannot delete outputs. Prompt ${prompt} not found in AIConfig.` |
There was a problem hiding this comment.
Should be a different error
| */ | ||
| public getLatestOutput(prompt: string | Prompt): Output | undefined { | ||
| if (typeof prompt === "string") { | ||
| prompt = this.getPrompt(prompt)!; |
There was a problem hiding this comment.
nit, prefer to use a different variable instead of reassigning the function param
| prompt = this.getPrompt(prompt)!; | ||
| if (!prompt) { | ||
| throw new Error( | ||
| `E1030: Cannot delete outputs. Prompt ${prompt} not found in AIConfig.` |
| */ | ||
| public getModelName(prompt: string | Prompt) { | ||
| if (typeof prompt === "string") { | ||
| prompt = this.getPrompt(prompt)!; |
There was a problem hiding this comment.
nit, same as above r.e. reassigning
| prompt = this.getPrompt(prompt)!; | ||
| if (!prompt) { | ||
| throw new Error( | ||
| `E1031: Cannot delete outputs. Prompt ${prompt} not found in AIConfig.` |
| return modelParser.getOutputText(prompt, this, output); | ||
| } | ||
|
|
||
| // TODO: saqadri - log a warning if the model parser isn't parameterized |
There was a problem hiding this comment.
Can remove this TODO as no longer relevant
| @@ -31,7 +31,11 @@ export class ModelParserRegistry { | |||
| } | |||
|
|
|||
| public static getModelParserForPrompt(prompt: Prompt) { | |||
| const id = ModelParser.getModelName(prompt); | |||
| const id = | |||
There was a problem hiding this comment.
Nice. This resolves a comment from earlier PR
This was referenced Oct 13, 2023
Merged
This was referenced Oct 13, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
[13/n] Add output CRUD operations to AIConfig
Stack created with Sapling. Best reviewed with ReviewStack.