-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(files): fix expires_after API shape #3604
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
Conversation
- Add Form() annotations to purpose and expires_after parameters in file upload endpoints - Add support for optional multipart form parameters in OpenAPI generator - Generated spec now properly mirrors OpenAI format with schema refs
@ashwinb @raghotham this is what you'd expect the api def to be, but it doesn't work. a lot of time went into figuring out the shape that works for fastapi. unfortunately it all got condensed to a terse comment - https://github.com/llamastack/llama-stack/pull/3604/files#diff-6b15e491d326a7148daaefcf182dbb2b1aabd88efdfada7c4973c58f9ce1e0b2L116 run minio -
run files=remote::s3 stack -
run tests -
test directly with curl, note expires_at comes back as null -
there are no expires_after unit tests because the crux is passing through fastapi. we still need to split out the ci-tests distro so we can add the remote::s3 integration tests to the ci suite. please revert this. |
Let me see if I can fix this forward early morning (unless you have reverted it?) |
i have not reverted it |
…3612) #3604 broke multipart form data field parsing for the Files API since it changed its shape -- so as to match the API exactly to the OpenAI spec even in the generated client code. The underlying reason is that multipart/form-data cannot transport structured nested fields. Each field must be str-serialized. The client (specifically the OpenAI client whose behavior we must match), transports sub-fields as `expires_after[anchor]` and `expires_after[seconds]`, etc. We must be able to handle these fields somehow on the server without compromising the shape of the YAML spec. This PR "fixes" this by adding a dependency to convert the data. The main trade-off here is that we must add this `Depends()` annotation on every provider implementation for Files. This is a headache, but a much more reasonable one (in my opinion) given the alternatives. ## Test Plan Tests as shown in #3604 (comment) pass.
This was just quite incorrect. See source here: https://platform.openai.com/docs/api-reference/files/create