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

Force Name Parameter #66

Merged
merged 8 commits into from
Jan 8, 2024
Merged

Force Name Parameter #66

merged 8 commits into from
Jan 8, 2024

Conversation

ethanleifer
Copy link
Contributor

@ethanleifer ethanleifer commented Jan 7, 2024

  • Change responseModel<T>.name to be required
  • Add the response_model.name parameter to all examples and tests
  • Make generic types extend z.ZodTypeAny to force schemas to error
  • Remove async from chatCompletion to remove a layer of nested Promise's

Is there anyway to test these cases of typechecking? @roodboi

response_model.name is undefined:
Screenshot 2024-01-07 at 7 22 08 PM

If you just have max_retires it also errors (so if you have max_retries you also need response_model but not the other way around)
Screenshot 2024-01-07 at 7 23 03 PM

Type inference still works:
Screenshot 2024-01-07 at 7 24 17 PM

Copy link

changeset-bot bot commented Jan 7, 2024

🦋 Changeset detected

Latest commit: bac8740

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@instructor-ai/instructor Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@roodboi
Copy link
Collaborator

roodboi commented Jan 8, 2024

This looks sick - do the stream results show the right ones too?

@ethanleifer
Copy link
Contributor Author

Worth double checking. All tests pass.

I think we need to figure out how to make sure we or someone else doesn't break these inferences moving forward

@roodboi
Copy link
Collaborator

roodboi commented Jan 8, 2024

Re: testing types - even though bun will run typescript it won't do any runtime typechecking - so im not sure what good ways to check might be - maybe have a test file that sets something up in a way that depends on the inference, such that if it was wrong the build would fail or something?

@ethanleifer
Copy link
Contributor Author

How is there no way to test types other than with vitetest which doesn't support bun

Feel like this would be super common for maintain a level of dx

@ethanleifer
Copy link
Contributor Author

ethanleifer commented Jan 8, 2024

THIS WORKS!!! https://github.com/loreanvictor/ts-inference-check

I don't get how this isn't a common use case with library authors

Pretty clean too: expect(type(completionOpenAI).is<OpenAI.Chat.ChatCompletion>(true)).toBeTrue()

Left an issue to make sure there is a reason to not use this...

export function OAIBuildMessageBasedParams<T extends z.ZodTypeAny>(
definition: ParseParams,
params: Omit<ChatCompletionCreateParamsWithModel<T>, "response_model">,
mode: MODE // This type should be typeof MODE.JSON | typeof MODE.JSON_SCHEMA | typeof MODE.MD_JSON
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/instructor-ai/instructor-js/blob/main/src/constants/modes.ts#L9

maybe i did it wrong but I thought keyof typeof gave you the equivalent union type

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe it's just a naming collision, Mode maybe for the type?

@roodboi roodboi merged commit dc22633 into instructor-ai:main Jan 8, 2024
1 check passed
@roodboi roodboi mentioned this pull request Jan 8, 2024
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