-
-
Notifications
You must be signed in to change notification settings - Fork 9
Universal LlmSchemaComposer
#215
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
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.
Pull request overview
This PR introduces a "universal" implementation of LlmSchemaComposer, replacing the previous model-specific dispatcher pattern with a unified implementation. The changes include a complete rewrite of LlmSchemaComposer.ts (from ~97 lines to 789 lines) and updates the API from model-based curried functions to direct property-based functions.
- Complete rewrite of
LlmSchemaComposer.tswith universal implementations ofparameters,schema,separate, andinvertfunctions - Attempted update of
HttpLlmApplicationComposer.tsto use the newseparatemethod instead ofseparateParameters - API change from
method(model)({props})pattern tomethod({props})pattern
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/composers/LlmSchemaComposer.ts | Complete rewrite implementing universal schema composition logic, replacing model-specific dispatcher with direct implementations of parameters, schema, separate, and invert functions |
| src/composers/HttpLlmApplicationComposer.ts | Attempts to update method call from separateParameters to separate, but incorrectly retains the old API pattern of passing model as parameter |
Comments suppressed due to low confidence (4)
src/composers/HttpLlmApplicationComposer.ts:237
- The call to
LlmSchemaComposer.separateis incorrect. The newseparatefunction (lines 364-427 in LlmSchemaComposer.ts) takes a props object directly, not a model parameter that returns a function. This code attempts to callseparate(props.model)which would pass the model as the props object, and then tries to invoke the result as a function, which will fail.
The call should be changed to:
LlmSchemaComposer.separate({
predicate: props.config.separate as any,
parameters: llmParameters.value satisfies ILlmSchema.ModelParameters[Model] as any,
equals: props.config.equals ?? false,
})
Note: This same issue exists for LlmSchemaComposer.parameters (line 179) and LlmSchemaComposer.schema (line 189) which also changed their API to accept props directly instead of a model parameter, but were not updated in this PR.
? (LlmSchemaComposer.separate(props.model)({
predicate: props.config.separate as any,
parameters:
llmParameters.value satisfies ILlmSchema.ModelParameters[Model] as any,
equals: props.config.equals ?? false,
}) as ILlmFunction.ISeparated<Model>)
src/composers/HttpLlmApplicationComposer.ts:237
- The PR description states this is a "targeted update" that "replaces the use of
LlmSchemaComposer.separateParameterswithLlmSchemaComposer.separate". However, the actual changes are much more extensive:
- The entire
LlmSchemaComposer.tsfile was completely rewritten (from ~97 lines to 789 lines) - The API changed from model-based curried functions (e.g.,
method(model)({props})) to direct prop-based functions (e.g.,method({props})) - This change affects not just
separate/separateParameters, but alsoparameters,schema,invert, and other methods - The update in this file is incomplete - it only partially updates
separateParameterstoseparatebut doesn't fix the API usage pattern, and leavesparametersandschemacalls unchanged despite them also having new APIs
The PR description significantly understates the scope of changes.
? (LlmSchemaComposer.separate(props.model)({
predicate: props.config.separate as any,
parameters:
llmParameters.value satisfies ILlmSchema.ModelParameters[Model] as any,
equals: props.config.equals ?? false,
}) as ILlmFunction.ISeparated<Model>)
src/composers/LlmSchemaComposer.ts:600
- Variable 'llm' cannot be of type null, but it is compared to an expression of type null.
llm !== null
src/composers/LlmSchemaComposer.ts:606
- Variable 'human' cannot be of type null, but it is compared to an expression of type null.
human !== null
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* Unified to `ILlmSchema`, no more separation. (#213) * Unified to `ILlmSchema` * Complete the new `ILlmSchema` type * detailed description comments * Update src/structures/ILlmSchema.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/structures/ILlmSchema.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/structures/ILlmSchema.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * prettier --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Unify `ILlmApplication` and `ILlmFunction` too. (#214) * Universal `LlmSchemaComposer` (#215) * Universal LlmSchemaComposer * complete `LlmSchemaComposer.schema()` function * fix logics * Universal `LlmTypeChecker` (#216) * Universal `LlmTypeChecker` * Fix `LlmSchemaComposer` to utilize `LlmTypeCheckeer` * JSDoc comments on universal LLM types. (#217) * Universal `HttpLlm` (#218) * Universal `HttpLlm` * Update src/HttpLlm.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/HttpLlm.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix configuration comments * fix more thing --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Remove individual LLM schemas (#219) * Remove individual LLM schemas * fix * Unify test functions about LLM schemas (#220) * Unify test functions about LLM schemas * Update test/src/utils/LlmApplicationFactory.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fixed most of test functions * fixed most of test functions * completed everything --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Publish v6 * Re-write README for universal LLM schemas (#221) --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request makes a targeted update to the logic for separating parameters in the
HttpLlmApplicationComposer. The change replaces the use ofLlmSchemaComposer.separateParameterswithLlmSchemaComposer.separate, likely reflecting a refactor or simplification in the schema composition utilities.HttpLlmApplicationComposerto useLlmSchemaComposer.separateinstead ofseparateParameters, aligning with updated schema composition methods.