-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(api): oasdiff OpenAI openAPI spec against ours #3529
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
base: main
Are you sure you want to change the base?
Conversation
67b7c6e to
b509e85
Compare
b509e85 to
8a2bb5f
Compare
.github/workflows/conformance.yml
Outdated
| if: steps.cache-openapi.outputs.cache-hit != 'true' | ||
| run: | | ||
| mkdir -p ~/openai-openapi | ||
| curl -L https://app.stainless.com/api/spec/documented/openai/openapi.documented.yml -o ~/openai-openapi/openai-openapi.yml |
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.
do we not want to pin the version of the openai api?
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.
sure, I can pin it. Do we have a version in mind we want to use?
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.
I think the pinning is important, what's the latest version so we can get started with it?
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.
see my comment below, added openapi spec to the repo, there is not a versioned URL.
.github/workflows/conformance.yml
Outdated
| id: cache-openapi | ||
| uses: actions/cache@0400d5f644dc74513175e3cd8d07132dd4860809 | ||
| with: | ||
| path: ~/openai-openapi |
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.
why use ~ here?
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.
I believe the cache action by default has access only to a runners home dir? I could be wrong but that is where I have been storing and writing the cached binaries+files to/from
| key: openai-openapi.yml | ||
|
|
||
| # Cache oasdiff to avoid checksum failures and speed up builds | ||
| - name: Cache oasdiff |
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.
we should probably also support a force no-cache conformance check as well - we should probably run it at the time of release.
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.
I agree, but should I make this a separate action so it can be triggered by maintainers manually?
da078af to
c5bd55e
Compare
leseb
left a comment
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.
I think this action could result in a "badge" on the repo once we feel confident, having the compatibility ratio again openai is very important :)
d7e8b77 to
5e68691
Compare
|
this now checks all of our APIs against all of the openAI apis. The removal of |
f423104 to
d9abb2e
Compare
|
@cdoern do you plan to pick this up? |
|
yes @ashwinb I can get back on this today/tomorrow. thanks for the reminder! |
d9abb2e to
867e0a4
Compare
|
@ashwinb this should be set now for review After my last commit, I realized that while the oasdiff command was working, our CI was still vulnerable to upstream changes in the OpenAI spec since we were fetching it live on every run. My initial thought was to find a permanent, version-locked URL to pin against, but after some investigation, it turns out there isn't a stable, versioned URL available for the exact spec we need, openai only published the most recent one to a stainless URL. To guarantee build stability and make our conformance test fully hermetic, I've changed my approach to vendor the OpenAI spec directly into our repository at docs/static/openai-spec.yml. |
diff the `/v1/` routes that are OpenAI compatible against the OpenAI openAPI spec. This will of course only trigger on PRs where the spec is changed. This will catch errors with new handwritten additions to our openAI compat routes. Instead of fetching the OpenAPI spec from a dynamic URL, which could cause non-deterministic build failures, this change uses a local copy stored at `docs/static/openai-spec.yml`. This makes the conformance check fully reproducible and prevents CI failures caused by uncontrolled upstream changes. Signed-off-by: Charlie Doern <[email protected]>
| continue-on-error: true | ||
| shell: bash | ||
| run: | | ||
| OPENAI_SPEC=docs/static/openai-spec.yml |
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.
Rather than manually download this, should we be downloading it dynamically?
In the OpenAI repo here: https://github.com/openai/openai-openapi/blob/master/README.md
They state the current version is here: https://app.stainless.com/api/spec/documented/openai/openapi.documented.yml
Obviously this will be brittle when things are updated but we could instead have versioned saved and versioned nightly or something.
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.
I had it this way in a previous version of the PR, but there is a request here to stick with a specific version of the openai spec.
Since they don't seem to have a versioned URL there really isn't a way for me to "version" it if we pull it dynamically.
One idea I had is we could cache it and maybe if we pull it and the version has changed, don't cache it again? I don't know if this is possible though, wdyt?
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.
yeah, that's reasonable. I think we can store this file to seed it and we could run a nightly job to check if it needs to be updated. We could either automatically update it or make a PR right?
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.
We could add a version either to the name of the file or somewhere in the file itself
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.
the file has a version: 2.3.0 which I was planning on using once the file is pulled to see if that has changed. I could do that or I could even use oasdiff To diff the old openai spec against the new one.
@franciscojavierarceo do you think I should do that in this PR or a separate one since this should probably be a different workflow that runs nightly, right?
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.
yeah can do in a follow up (this PR is also fine to me tbh since it's heavily related)
867e0a4 to
a8928b6
Compare
leseb
left a comment
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.
Thanks!
| shell: bash | ||
| run: | | ||
| OPENAI_SPEC=docs/static/openai-spec.yml | ||
| LOCAL_SPEC=docs/static/llama-stack-spec.yaml |
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.
| LOCAL_SPEC=docs/static/llama-stack-spec.yaml | |
| LLAMA_STACK_SPEC=docs/static/llama-stack-spec.yaml |
If you feel like this is better :)
| info: | ||
| title: OpenAI API | ||
| description: The OpenAI REST API. Please see https://platform.openai.com/docs/api-reference for more details. | ||
| version: 2.3.0 |
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.
I'm wondering if we should either:
- put this into a 2.3.0 directory
- or add -2.3.0 to the file name
Or perhaps, are we always replacing / overwritting with a new spec?
What does this PR do?
diff the
/v1/routes that are OpenAI compatible against the OpenAI openAPI spec. This will of course only trigger on PRs where the spec is changed.This will catch errors with new handwritten additions to our openAI compat routes.
Instead of fetching the OpenAPI spec from a dynamic URL, which could cause non-deterministic build failures,
this change uses a local copy stored at
docs/static/openai-spec.yml.This makes the conformance check fully reproducible and prevents CI failures caused by uncontrolled upstream changes.
I am marking this test with
continue-on-error: true, until we get rid of all of the errors. Nevertheless, this is a nice utility to have so folks know if their spec changes introduce more breaking changes or fix breakages when comparing to the OpenAI openapi spec.Test Plan
test should pass.