-
Notifications
You must be signed in to change notification settings - Fork 4.9k
WIP: New approach to multimodal document ingestion #2558
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
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
Check Broken PathsWe have automatically detected the following broken relative paths in your files. Check the file paths and associated broken paths inside them.
|
@mattgotteiner is still working on integrated vectorization, but I'm going to ask Copilot for a review on the rest of 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.
Pull Request Overview
This PR updates the test suite to align with the new multimodal document ingestion approach, adds tests for the new MultimodalModelDescriber
, renames configuration flags, and expands blob manager coverage including ADLS support.
- Added tests for
MultimodalModelDescriber.describe_image
and behavior on empty responses - Removed obsolete tests for
fetch_image
and updated blob manager tests for resource_group/subscription_id and image uploads - Updated app config tests to use the new
showMultimodalOptions
flag and refreshed many snapshot files to include image citations
Reviewed Changes
Copilot reviewed 160 out of 170 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/test_mediadescriber.py | New tests for multimodal image describer, including mock OpenAI client |
tests/test_fetch_image.py | Removed old fetch_image tests |
tests/test_blob_manager.py | Updated constructor params, added ADLS upload/download tests |
tests/test_app_config.py | Renamed feature flag from showGPT4VOptions to showMultimodalOptions |
tests/snapshots/**/* | Snapshots updated to include new image citations and metadata |
Comments suppressed due to low confidence (3)
tests/test_app_config.py:125
- The configuration API has been updated to use showMultimodalOptions; ensure the backend implementation and any related documentation are updated accordingly to expose this new flag and remove references to the old showGPT4VOptions.
assert result["showMultimodalOptions"] is False
tests/test_blob_manager.py:24
- The BlobManager constructor parameters have been renamed to resource_group and subscription_id; ensure that the production code signature matches these updated names, otherwise this test will fail.
resource_group=os.environ["AZURE_STORAGE_RESOURCE_GROUP"],
@@ -133,3 +139,115 @@ def mock_put(self, *args, **kwargs): | |||
) | |||
with pytest.raises(Exception): | |||
await describer_bad_analyze.describe_image(b"imagebytes") | |||
|
|||
|
|||
class MockAsyncOpenAI: |
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.
[nitpick] MockAsyncOpenAI and MockChatCompletions helper classes are defined inline; extracting them into pytest fixtures or shared utilities would improve test maintainability and reduce duplication.
Copilot uses AI. Check for mistakes.
Purpose
As I've discussed in various issues and live streams, our current "GPT vision approach" has some drawbacks, specifically:
The new multimodal approach:
This is not yet complete, but I'm sharing the PR in early WIP form so that developers can see the direction and provide feedback.
Does this introduce a breaking change?
When developers merge from main and run the server, azd up, or azd deploy, will this produce an error?
If you're not sure, try it out on an old environment.
Does this require changes to learn.microsoft.com docs?
This repository is referenced by this tutorial
which includes deployment, settings and usage instructions. If text or screenshot need to change in the tutorial,
check the box below and notify the tutorial author. A Microsoft employee can do this for you if you're an external contributor.
Type of change
Code quality checklist
See CONTRIBUTING.md for more details.
python -m pytest
).python -m pytest --cov
to verify 100% coverage of added linespython -m mypy
to check for type errorsruff
andblack
manually on my code.