Skip to content
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

Image ingestion improvements #2

Merged

Conversation

mhbuehler
Copy link
Owner

@mhbuehler mhbuehler commented Oct 21, 2024

Description

Improves previous image ingestion implementation by reusing the existing /generate_captions endpoint instead of creating a new one, reducing code. I also updated READMEs and tests in both repos. Goes with this PR in GenAIExamples.

Issues

RFC

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)
  • Others (enhancement, documentation, validation, etc.)

Dependencies

No new dependencies

Tests

  • test_dataprep_multimodal_redis_langchain.sh (updated with new functionality and fixed proxy variables)

Signed-off-by: Melanie Buehler <[email protected]>
Signed-off-by: Melanie Buehler <[email protected]>
Signed-off-by: Melanie Buehler <[email protected]>
Signed-off-by: Melanie Buehler <[email protected]>
Copy link
Collaborator

@dmsuehir dmsuehir left a comment

Choose a reason for hiding this comment

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

LGTM, the only changes that I would be unsure of are the proxy changes in the test file, because it looks like the tests are run with GHA (for example here), and the docker builds and tests seem to be successful, so it's hard to know if the proxy changes would break anything from GHA runs.

exit 1
else
echo "[ $SERVICE_NAME ] Content is as expected."
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, one comment that I have after reviewing Omar's PR - in the test for get_videos (which he's renaming to get_files) it's currently checking RESPONSE_BODY for the video name, but can you add to make it also check for the image name?

Signed-off-by: Melanie Buehler <[email protected]>
Copy link
Collaborator

@dmsuehir dmsuehir left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

mhbuehler and others added 3 commits October 24, 2024 09:21
Signed-off-by: Melanie Buehler <[email protected]>
…hbuehler/GenAIComps into melanie/combined_image_video_ingestion
@mhbuehler mhbuehler merged commit a51f0aa into melanie/mm-rag-enhanced Oct 24, 2024
@mhbuehler mhbuehler deleted the melanie/combined_image_video_ingestion branch October 24, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants