-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: introduce api leveling proposal #3317
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
cc @nathan-weinberg since I know you wanted a look at this :) |
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.
focusing only on the openai compat apis, how would this framework classify the following and why -
- /v1/chat/completions
- /v1/completions
- /v1/files
- /v1/batches
great point @mattf As the Off the top of my head though: /v1/chat/completions -- seems stable, so likely would remain with completions and chat completions though I know there are various features going in as we approach 0.3.0, so we'd need to evaluate if any of these are breaking, if they are -- it'd need to be /v1/files -- I have seen a few major enhancements to files go in recently like (#3283) and the s3 provider in general, so I'd imagine this would be /v1/batches -- given the large changes like #3309 and maybe #3261, #3171, etc etc I think this should be
and just a note -- the reason I chose alpha and not beta is as the doc states, beta is almost a stepping stone briefly between alpha and v1 where not much major development should happen. |
working through some examples will help, at least me, understand the framework.
🙏
by definition, the shape of these apis (path, input, output) is set and as stable as openai makes them. there are variations in completeness of the implementation.
the first of these is arguably a gap to close. the second is arguably a bug to fix, but may not be feasible. for instance, the nvidia service does not honor the number of logprobs requested. the llama api service is stricter about json schema for tool calls than other providers. how would these provider differences impact the classification of the api under this framework? how would you describe the third using this framework?
also by definition, the shape is stable. it may be new to stack, but it isn't changing. in the case of /v1/files, the localfs adapter does not implement expiration, while the s3 adapter does. how does the difference in adapter implementation impact the classification of the api?
by definition here, the shape is also stable. missing from the implementation is support for /v1/embeddings and /v1/responses, which happen to be part of the openapi spec (endpoint is an enum of /v1/responses, /v1/chat/completinos, /v1/embeddings, /v1/completions). the api shape for a Batch includes a status enum with fields validating, failed, in_progress, finalizing, completed, expired, cancelling, cancelled. the adapter will not produce a finalizing status. unlike the inference and files endpoints, /v1/batches only has one inline provider. how do these aspects impact the classification?
a practical consideration here, when using the
|
@mattf I think simply put: If any of our OpenAI compatible APIs are not "API complete" in the sense that a new route is added to the API itself (not a provider), or a breaking change to the api datatypes is made (like changing of required params for a route or return type) that is when something needs to be so if our OpenAI compatible APIs are missing something that is in the OpenAI spec, I think that merits a less than an example: lets say post_training needs a massive change and however, lets say the So generally: provider changes do not correlate to API maturity, but rather API level datatype or structural changes to required endpoints necessitate a lower level than Does this align with your thinking? |
2107957
to
0f3ba22
Compare
I think there are two aspects here:
And it is not clear whether one should merge both concerns into a single token "v1alpha1". I am sure this issue has been thought of by other projects before? |
docs/source/apis/api_leveling.md
Outdated
|
||
## Different Levels | ||
|
||
### v1alpha1 |
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.
/alpha/v1
or /beta/v1/
feel slightly more readable?
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.
perhaps, I chose v1alpha1
, and v1beta1
to mimic the leveling in k8s, and also a single level in the URL like: localhost:8321/v1alpha1/post_training/...
seems clean? I am fine with whichever honestly.
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.
Just for my understanding of the why k8s chose this format: is there an expectation that we would create v1alpha2? /v1alpha and /v2alpha seem to make sense as versions that are going to go into the eventual stable /v1 or /v2 versions.
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 don't think v1alpha2
would exist. If it helps clear it up I can switch both to v1alpha
and v1beta
without the trailing 1. I agree, It could be nice to extend this in the case of a v2
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.
switched for now, let me know if v1alpha
and v1beta
make sense here @ashwinb @raghotham 🙏
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.
Nice work, @cdoern! This looks great, thank you!
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.
Suggested some small improvements though.
yeah @ashwinb that is the proper delineation. I think in LLS specifically, what matters most is the API definiton: datatypes, API routes+parameter+return types I kind of view the providers similarly to operators in k8s, where the maturity of an individual operator is not correlated to the maturity of all high level APIs. Of course, there is some intertwined nature, but this proposal is basically saying: Providers can iterate as much as they want on functionality as long as they work within the bounds of an API, if they need to change the API, then the API should not be |
going to make some of the above suggestions and repush the proposal as is, generally. |
0f3ba22
to
6195e99
Compare
Great work, @cdoern ! Thanks! |
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.
lgtm
d863ee0
to
12af6e5
Compare
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.
Getting close!
12af6e5
to
e335ce7
Compare
e335ce7
to
2a574ae
Compare
2a574ae
to
7947171
Compare
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.
Solid foundation to start measuring our current APIs and onwards. Thanks!
7947171
to
4ba538c
Compare
rebased |
4ba538c
to
84a4adc
Compare
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.
lgtm
one last nit would be to include a proposal over the current state of APIs in this
thanks @franciscojavierarceo ! I think this warrants its own piece of work as a follow up. I was imagining this would merge and then the work to actually define which apis are at which level would happen immediately after so that no assumptions are made without research into the actual stability. hope that makes sense! |
84a4adc
to
06ae8ae
Compare
@mattf changed the verbiage discussing
|
06ae8ae
to
dbfc850
Compare
+1 -- let's handle it separately (both defining which APIs are which, and figuring out how to reflect it in the docs) |
|
||
Providers can iterate as much as they want on functionality as long as they work within the bounds of an API. If they need to change the API, then the API should not be `/v1`, or those breaking changes can only happen on a y-stream release basis. | ||
|
||
### Approval and Announcement Process for Breaking Changes |
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.
should probably also include something like this to define a protocol for when there is a breaking change - #3260. A PR that is titled a specific way will not fail the oasdiff check.
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.
+1 it'll make it easier for calling out in the release notes and any sorts of additional announcements (e.g., in discord, email, etc.).
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.
so by this you mean: I should add a bullet here describing how the PR title and commit message should include an indicator of a breaking change? I can add that!
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 added a section here, luckily conventional commits outlines how to handle this: https://www.conventionalcommits.org/en/v1.0.0/#specification
dbfc850
to
884d541
Compare
this document outlines different API stability levels, how to enforce them, and next steps Signed-off-by: Charlie Doern <[email protected]>
884d541
to
f4f2f8e
Compare
What does this PR do?
this document outlines different API stability levels, how to enforce them, and next steps
Next Steps
Following the adoption of this document, all existing APIs should follow the enforcement protocol.
relates to #3237