-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
Remove model name check for REST models #356
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.
Looks good to me - thanks for the quick turnover!
I cancelled the external tests because 1) OpenAI API is down and 2) the tests don't actually cover the new model names, right? I guess that would be overkill, to test every combination.
Nevertheless, I'd like us to hold off a bit on merging. If OpenAI's API comes back online, I'd like to just do a quick test locally.
Yes, that applies to some older models as well. We are at a point where we don't test every model. We should discuss whether that's still something we want to do. If so, we should implement the smallest possible smoke test and parallelize them as much as possible. |
…l names. Remove model checks for REST models.
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.
Few small suggestions.
Co-authored-by: Sofie Van Landeghem <[email protected]>
Co-authored-by: Sofie Van Landeghem <[email protected]>
Co-authored-by: Sofie Van Landeghem <[email protected]>
…y-llm into chore/openai-model-update
Description
Closes #351. Note that new,
Literal
-less registry functions for other REST models still have to be added.Corresponding documentation PR
explosion/spaCy#13119
Types of change
Enhancement.
Checklist
tests
andusage_examples/tests
, and all new and existing tests passed. This includespytest
ran with--external
)pytest
ran with--gpu
)