Skip to content

feat: Updating files/content response to return additional fields #3054

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

feat: Updating files/content response to return additional fields #3054

wants to merge 5 commits into from

Conversation

franciscojavierarceo
Copy link
Collaborator

@franciscojavierarceo franciscojavierarceo commented Aug 6, 2025 •
edited
Loading

What does this PR do?

  1. Adds 4 fields to VectorStoreContent which are returned in openai/v1/vector_stores/{vector_store_id}/files/{file_id}/content
    • These fields are used to populate the admin portal in feat(ui): Adding Vector Store Files to Admin UI #3041.
    • Unit tests and integration tests are added to ensure this doesn't break OpenAI compatibility
      • In the integration test, I identified a bug on the client where we instantiate the VectorStoreContent class in VectorStoreFileContentsResponse, which is inconsistent with OpenAI's client which just returns dict[str, Any]. I will provide a follow up PR to fix that and we'll have to update the client in the next client release.
  2. Updates .github/actions/setup-test-environment/action.yml to set the OLLAMA_URL during replay mode, which was causing failures from the new integration test.
    • This was a bit opaque to figure out and suggests we should improve the logging here. I'll follow up with this in the future.

Test Plan

Added unit test and integration test.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 6, 2025
@franciscojavierarceo franciscojavierarceo added re-record-tests Spin up ollama, inference and record responses for later use re-record-vision-tests Whether the relevant workflow should be triggered for re-recording vision tests labels Aug 8, 2025
@franciscojavierarceo franciscojavierarceo removed re-record-tests Spin up ollama, inference and record responses for later use re-record-vision-tests Whether the relevant workflow should be triggered for re-recording vision tests labels Aug 8, 2025
@franciscojavierarceo franciscojavierarceo changed the title feat: Updating files/content response to return additional fields feat: Updating files/content response to return additional fields Aug 9, 2025
@franciscojavierarceo
Copy link
Collaborator Author

@ashwinb any chance you can take a look at this PR?

Do you have any info on running the ./scripts/integration-tests.sh locally in replay mode? I used the settings I found in the GH action run but that resulted in different errors than what appear in the CI. I figured I'd see if you had any tips her.

franciscojavierarceo
franciscojavierarceo commented Aug 9, 2025
View reviewed changes
tests/integration/vector_io/test_openai_vector_stores.py
assert len(file_contents.content) > 0

for content_item in file_contents.content:
if isinstance(compat_client, LlamaStackClient):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is where we are incompatible with OpenAI so the test currently handles it here.

@franciscojavierarceo franciscojavierarceo mentioned this pull request Aug 13, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashwinb ashwinb Awaiting requested review from ashwinb ashwinb will be requested when the pull request is marked ready for review ashwinb is a code owner

@yanxi0830 yanxi0830 Awaiting requested review from yanxi0830 yanxi0830 will be requested when the pull request is marked ready for review yanxi0830 is a code owner

@hardikjshah hardikjshah Awaiting requested review from hardikjshah hardikjshah will be requested when the pull request is marked ready for review hardikjshah is a code owner

@raghotham raghotham Awaiting requested review from raghotham raghotham will be requested when the pull request is marked ready for review raghotham is a code owner

@ehhuang ehhuang Awaiting requested review from ehhuang ehhuang will be requested when the pull request is marked ready for review ehhuang is a code owner

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan terrytangyuan will be requested when the pull request is marked ready for review terrytangyuan is a code owner

@leseb leseb Awaiting requested review from leseb leseb will be requested when the pull request is marked ready for review leseb is a code owner

@bbrowning bbrowning Awaiting requested review from bbrowning bbrowning will be requested when the pull request is marked ready for review bbrowning is a code owner

@reluctantfuturist reluctantfuturist Awaiting requested review from reluctantfuturist reluctantfuturist will be requested when the pull request is marked ready for review reluctantfuturist is a code owner

@mattf mattf Awaiting requested review from mattf mattf will be requested when the pull request is marked ready for review mattf is a code owner

@slekkala1 slekkala1 Awaiting requested review from slekkala1 slekkala1 will be requested when the pull request is marked ready for review slekkala1 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@franciscojavierarceo