Azure-Samples / chat-with-your-data-solution-accelerator

A Solution Accelerator for the RAG pattern running in Azure, using Azure AI Search for retrieval and Azure OpenAI large language models to power ChatGPT-style and Q&A experiences. This includes most common requirements and best practices.
https://azure.microsoft.com/products/search
MIT License
787 stars 395 forks source link

feat: Generate embeddings for images #892

Closed cecheta closed 4 months ago

cecheta commented 4 months ago

Required by #748

Purpose

Does this introduce a breaking change?

Testing:

github-actions[bot] commented 4 months ago

Coverage

Coverage Report •
FileStmtsMissCoverMissing
code/backend/batch/utilities/helpers
   azure_computer_vision_client.py470100% 
   env_helper.py1331092%221, 226–227, 230–232, 244, 248–250
code/backend/batch/utilities/helpers/config
   config_helper.py1340100% 
code/backend/batch/utilities/helpers/embedders
   embedder_factory.py10370%12–13, 15
   push_embedder.py510100% 
TOTAL238367371% 

Tests Skipped Failures Errors Time
194 0 :zzz: 0 :x: 0 :fire: 11.979s :stopwatch:
gaurarpit commented 4 months ago

tests are failing.. looking into them

gaurarpit commented 4 months ago

tests are failing.. looking into them

fixed.

superhindupur commented 4 months ago

Regarding the use of a mock http server for unit tests:

I understand the point about not wanting to tie our mocks tightly to the underlying implementation (http requests vs sdk), but couldn't we resolve this issue by just using an interface and implementation? Mock the interface behavior in the tests, and the implementation details will remain hidden. Also, mocks in general tend to be tied to underlying implementation. Why the special consideration for this case?

I would vote for keeping unit tests free of http servers - and reserve this type of testing for our functional tests. Consistency helps readers reason about the code base. I'd argue that in this case readability is more important than the possible rework of the test mocks if/when we switch the underlying calls from http to sdk.

adamdougal commented 4 months ago

Bhavana and I caught up on a call. Decided to keep the tests as they are, but added a comment to the test class explaining the rationale.