-
Notifications
You must be signed in to change notification settings - Fork 5k
Adjust defaults for multimodal-related parameters to work for default deployments #2717
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
text_sources.append(f"{citation}: {nonewlines(' . '.join([cast(str, c.text) for c in doc.captions]))}") | ||
else: | ||
text_sources.append(f"{citation}: {nonewlines(doc.content or '')}") | ||
if include_text_sources: |
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.
The only change here is the very top line-
if download_image_sources: | ||
return DataPoints(text=text_sources, images=image_sources, citations=citations) | ||
return DataPoints(text=text_sources, citations=citations) | ||
return DataPoints(text=text_sources, images=image_sources, citations=citations) |
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.
This makes image_sources [] when not including image sources, which is consistent with text_sources being [] when not including text sources. Snapshots already aligned with this, and it simplified code.
// Set default vector field settings | ||
setSearchTextEmbeddings(true); | ||
setSearchImageEmbeddings(true); | ||
// Initialize from server config so defaults follow deployment settings |
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.
This is how /chat was already doing it, fixed for consistency
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 fixes multimodal-related parameter defaults to prevent errors in deployments without multimodal features enabled. The frontend was incorrectly defaulting multimodal parameters to true
, causing backend errors when multimodal was disabled.
Key changes:
- Updated frontend defaults for
sendImageSources
andsearchImageEmbeddings
fromtrue
tofalse
- Added backend guards to ensure multimodal parameters can only be enabled when multimodal is actually available
- Fixed missing
send_text_sources
support in the/ask
endpoint to match/chat
functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
app/frontend/src/pages/chat/Chat.tsx | Changed default values for image-related parameters from true to false |
app/frontend/src/pages/ask/Ask.tsx | Changed default values for image-related parameters from true to false and updated initialization logic |
app/backend/approaches/chatreadretrieveread.py | Added multimodal guards and support for send_text_sources parameter |
app/backend/approaches/retrievethenread.py | Added multimodal guards and support for send_text_sources parameter |
app/backend/approaches/approach.py | Updated get_sources_content method to handle include_text_sources parameter |
tests/test_app.py | Added tests for send_text_sources and multimodal parameter validation |
tests/e2e.py | Updated e2e test to validate correct default parameter values |
if download_image_sources: | ||
return DataPoints(text=text_sources, images=image_sources, citations=citations) | ||
return DataPoints(text=text_sources, citations=citations) | ||
return DataPoints(text=text_sources, images=image_sources, citations=citations) |
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.
The return statement always includes image_sources
even when download_image_sources
is False. This should conditionally include images only when download_image_sources
is True, otherwise pass None
or an empty list for the images parameter.
return DataPoints(text=text_sources, images=image_sources, citations=citations) | |
return DataPoints( | |
text=text_sources, | |
images=image_sources if download_image_sources else None, | |
citations=citations | |
) |
Copilot uses AI. Check for mistakes.
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.
@copilot But it will be an empty list in that case, since image_sources is only populated when download_image_sources is True?
search_index_filter = self.build_filter(overrides, auth_claims) | ||
send_text_sources = overrides.get("send_text_sources", True) | ||
send_image_sources = overrides.get("send_image_sources", True) | ||
send_image_sources = overrides.get("send_image_sources", self.multimodal_enabled) and self.multimodal_enabled |
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.
great :)
Purpose
Fixes #2710
Our frontend was setting the multimodal-parameters sendImageSources and searchImageEmbedding parameters to true, which caused an error in backends with multimodal disabled.
This corrects the frontend to default to False AND adds an extra guard to backend to ensure they can only be set to true if multimodal is enabled.
Also made sure /ask supports send_text_sources like /chat does, the logic was missing.
Test wise, I adjusted an e2e test to confirm the defaults, and added two backend tests.
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.