Skip to content
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

Add validated check #23

Merged
merged 18 commits into from
Jan 4, 2024
Merged

Add validated check #23

merged 18 commits into from
Jan 4, 2024

Conversation

jxnl
Copy link
Collaborator

@jxnl jxnl commented Jan 3, 2024

if we throw an error we should catch it and update the response

@jxnl jxnl requested a review from roodboi January 3, 2024 05:09
@@ -41,3 +104,22 @@ describe("FunctionCall", () => {
expect(user.age).toEqual(30)
})
})

describe("FunctionCallValidated", () => {
test("Name should be uppercase based on validation check", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulled this down and played around a bit - looks like we are really struggling to get things in uppercase

The only pass I was able to get was to switch over to using MD_JSON - my guess is likely because that mode inserts the schema in a system prompt and includes some instruction

function calls (via tools or otherwise) dont really have any system level instruction at all - maybe we should include something

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the goal was that the error messages "this should be in uppercase" should prompt the llm to do that, lets try with gpt-4

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should change the prompt to not just try again but to fix the erros

@jxnl
Copy link
Collaborator Author

jxnl commented Jan 3, 2024

is it me or is the original zod error throwing vs the llm error?

@roodboi
Copy link
Collaborator

roodboi commented Jan 3, 2024

so quick check on the json schema being generated byu the zod to json schema package and it looks like we are not getting any description data in the schema

{
  type: "object",
  properties: {
    age: {
      type: "number",
    },
    name: {
      type: "string",
    },
  },
  required: [ "age", "name" ],
  additionalProperties: false,
}

this is what its producing on that uppercase refinement - think this also highlights an issue with retry prompt, but this is one of the reasons I was initially advocating for baking the conversion in house for better control

@roodboi
Copy link
Collaborator

roodboi commented Jan 3, 2024

If I add a .describe then we pass right away, as that will get added as the description on the property

It would be nice to be able to transform the refinement messages or other zod effects into descriptions where possible - but adding description seems to work.

also, having a lot of trouble with functions lately - moving to tools with same function works a lot better, not sure if theres been an update to the base models but it seems like standard function calling has gotten worse.

Maybe not the right place for this question but, given that fns are being deprecated, would it make sense to just omit that mode from the start in this lib all together, or at least re-map it to tools?

@jxnl
Copy link
Collaborator Author

jxnl commented Jan 3, 2024

we don't want the describe since the goal is that the ERROR message is what fixes the issue, not the prompt

tests/functions.test.ts Outdated Show resolved Hide resolved
src/instructor.ts Outdated Show resolved Hide resolved
@jxnl
Copy link
Collaborator Author

jxnl commented Jan 3, 2024

tool_choice: {
    type: "function",
    function: {
      name: "response_model",
    },
  },
  tools: [
    {
      type: "function",
      function: {
        name: "response_model",
        description: undefined,
        parameters: {

@roodboi noticed. that the name function is "response_model" but it should be the name of the variable. is there a way to access that?

@roodboi
Copy link
Collaborator

roodboi commented Jan 3, 2024

what variable?

@jxnl
Copy link
Collaborator Author

jxnl commented Jan 3, 2024

name: "response_model",

name: "response_model" should be the name of the object/schema.

@jxnl
Copy link
Collaborator Author

jxnl commented Jan 3, 2024

i hate js lol i dont understand anything

@jxnl
Copy link
Collaborator Author

jxnl commented Jan 4, 2024

holy shit

@jxnl jxnl merged commit ee520d6 into main Jan 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants