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
654 stars 326 forks source link

fix: handle BlobDelete event type in `batch_push_results` #893

Closed liammoat closed 1 month ago

liammoat commented 1 month ago

Purpose

Does this introduce a breaking change?

How to Test

  1. Run the updated Azure Function
  2. #1 Create a blob, uploading via Azure Storage
  3. #2 Delete the blob from Azure Storage
  4. #3 Trigger `BatchStartProcessing function

What to Check

Verify that the following are valid

  1. #1 BatchPushResults is triggered, and the document is indexed.
  2. #2 BatchPushResults is triggered, and the associated indexes are removed.
  3. #3 BatchPushResults is triggered, as normal, and the documents are indexed.

Other Information

Remaining Actions

The following is outstanding for this PR:

github-actions[bot] commented 1 month ago

Coverage

Coverage Report •
FileStmtsMissCoverMissing
code/backend/batch
   batch_push_results.py360100% 
   function_app.py17170%1–8, 10, 12–14, 16, 19–22
code/backend/batch/utilities/search
   search_handler_base.py482352%13–15, 18–20, 24, 28, 32, 36, 40, 44, 48, 51–52, 54–56, 58–59, 62–63, 65
TOTAL242468471% 

Tests Skipped Failures Errors Time
201 0 :zzz: 0 :x: 0 :fire: 12.926s :stopwatch:
liammoat commented 1 month ago

Team - Couple of things to note about this PR:

  1. There is likely a better way to do this - however, I have tried to implement it with as few downstream changes as possible (i.e. I don't want to refactor everything to fix this)
  2. With this change in mind, I propose the admin app should delete the blob, and let the function handle the index.
  3. I will leave this in draft until I have introduce some tests to cover the core behaviour
superhindupur commented 1 month ago

Team - Couple of things to note about this PR:

  1. There is likely a better way to do this - however, I have tried to implement it with as few downstream changes as possible (i.e. I don't want to refactor everything to fix this)
  2. With this change in mind, I propose the admin app should delete the blob, and let the function handle the index.
  3. I will leave this in draft until I have introduce some tests to cover the core behaviour

Definitely agree with " I propose the admin app should delete the blob, and let the function handle the index." - could you please create an issue to cover this if there isn't one already?

Update: I see that @komalg1 is working on this in #876 - would be worth the two of you catching up.

superhindupur commented 1 month ago

@liammoat thanks so much for this, really appreciate it! ❤️