checksum support , improve control with new cmd line switches by cforce · Pull Request #2134 · Azure-Samples/azure-search-openai-demo

cforce · 2024-11-10T11:04:20Z

This PR leverages the MD5 checksum available through local, blob, or Data Lake storage metadata fields, based on the discussion in #2113.

By default, the PR introduces a check during ingestion (for both local and Azure Data Lake Storage sources) to see if each file already exists in the target storage. The MD5 checksum is saved in the index as a verification method, enabling a lookup that ensures at least one record with the same category/filename is found. This also permits the same file to be copied to multiple categories if needed.

Additionally, a new command-line parameter, --folder <name>, allows specifying a dedicated subfolder within the data folder or Data Lake container for each script run. This enables isolated runs with explicitly controlled parameters and a specific subset of files.

The new --enablevision true/false option allows per-folder and per-category control over whether vision embeddings should be generated. This option is beneficial when dealing with folders containing predominantly heavy image content, providing control over resource use.

Example usage:

./.venv/bin/python ./app/backend/prepdocs.py './data/createindex_dummy/*' --category newtest --folder test --enablevision false --verbose

The PR also improves progress reporting by displaying the number of files processed and providing updates every 10 files. This enhances visibility into the processing speed, which is particularly useful when working with partially processed data. Additionally, the log level of some messages has been adjusted from INFO to DEBUG to reduce console noise and retain progress updates longer.

Finally, MD5 checksum comparisons are introduced at various stages: a top-level check (to avoid redundant processing), and checks for blobs, text, and image embeddings. A flag to bypass the top-level check for known MD5 values in the index may be introduced later (e.g., --ignore_checksum).

pamelafox · 2024-11-11T18:36:46Z

+        file_md5 = file.metadata.get('md5')
+        blob_md5 = blob_metadata.get('md5')
+
+        # Remove md5 from file metadata if it matches the blob metadata


This comment doesn't seem to describe the next line?

pamelafox · 2024-11-11T18:37:04Z

+                                return await container_client.upload_blob(blob_name, reopened_file, overwrite=True, metadata=file.metadata)
+
+    async def _file_blob_update_needed(self, blob_client: BlobClient, file : File) -> bool:
+        md5_check : int = 0 # 0= not done, 1, positive,. 2 negative


Did you end up not using this variable?

pamelafox · 2024-11-11T18:37:36Z

        self.subscriptionId = subscriptionId
        self.user_delegation_key: Optional[UserDelegationKey] = None

+    #async def upload_blob(self, file: File, container_client:ContainerClient) -> Optional[List[str]]:


This line can be rm'ed now?

pamelafox · 2024-11-11T18:38:16Z

-            # Re-open and upload the original file
+
+             # Re-open and upload the original file
+            md5_check : int = 0 # 0= not done, 1, positive,. 2 negative


I think a Python Enum would be clearer here?

pamelafox · 2024-11-11T18:38:39Z

+             # Re-open and upload the original file
+            md5_check : int = 0 # 0= not done, 1, positive,. 2 negative
+
+            # upload  the file local storage zu azure  storage


Suggested change

# upload the file local storage zu azure storage

# upload the file from local storage to Azure storage

pamelafox · 2024-11-11T18:39:29Z

+                        md5_check = 1                        
+                file.url = blob_client.url
+
+            if md5_check!=1 and self.store_page_images:


Please install the pre-commit (see CONTRIBUTING guide) as it'll fix Python formatting issues for you, like the whitespace around operators. Thanks!

pamelafox

Thank you! I've started a review but am OOO today, will continue review later.

pamelafox · 2024-11-20T17:15:52Z

-            logger.info("Converting page %s to image and uploading -> %s", i, blob_name)
+
+            blob_client = container_client.get_blob_client(blob_name)
+            if await blob_client.exists():


Do we need this additional check here, given the check that happens in the function that calls this code?

pamelafox · 2024-11-20T17:20:04Z

                self.user_delegation_key = await service_client.get_user_delegation_key(start_time, expiry_time)

-            if blob_client.account_name is not None:
+            if container_client.account_name is not None:


Am curious why the need to go from blob_client to container_client here? Does it matter?

pamelafox · 2024-11-20T17:20:44Z

        logger.info("Skipping '%s', no parser found.", file.filename())
        return []
-    logger.info("Ingesting '%s'", file.filename())
+    logger.debug("Ingesting '%s'", file.filename())


Looks like you changed the levels to debug, does the current output feel too verbose? I find it helpful.

pamelafox · 2024-11-20T17:30:56Z

            await self.blob_manager.remove_blob()
            await search_manager.remove_content()

+    async def process_file(self, file, search_manager):


These two functions seem to be unused currently- perhaps your thought was to refactor the code above to call these two functions. I'll remove them for now to make the diff smaller.

pamelafox · 2024-11-20T17:34:56Z

+        """
+
+        # Create a BlobServiceClient using account URL and credentials
+        service_client = BlobServiceClient(


Can you use BlobServiceClient to interact with a DataLake Storage account? That's what you seem to be doing here, but I didn't realize that was possible.

pamelafox · 2024-11-20T17:40:56Z

+        blob_metadata = blob_properties.metadata
+
+        # Check if the md5 values are the same
+        file_md5 = file.metadata.get("md5")


I don't see logic in the main prepdocs for setting the md5 of the local file metadata, I only see that in ADLS2. How does this work when not using the ADLS2 strategy?

pamelafox · 2024-11-20T17:42:06Z

-                    ),
-                    SearchableField(
-                        name="content",
+            fields = [


How necessary is it to store md5 as a field in the search index? I was thinking we'd also need to store it in the blob/file metadata? @mattgotteiner What do you think of storing md5 in search index, have you seen this approach before?

pamelafox · 2024-11-20T17:42:54Z

I've gone in and made the changes I recommended, but I have some higher-level questions about the approach, which I've left as comments.

cforce and others added 18 commits November 4, 2024 08:17

add scan option

a608e57

add scandir option an example folder

3206092

update docs

a482213

fix if issue

ce4bffe

fix findings, add metadata like md5

285ae7d

Update requirements

bfb95ab

Update requirements

d07c9eb

conditional upload - check blob md5 before upload

4769133

Merge branch 'Azure-Samples:main' into cforce-2065

9a385a4

Merge branch 'main' into cforce-2065

66b5506

add checksum, reduce log verbose

f4a504c

add datetime

4ac3780

add datetime

283d6d2

support checksum

797347c

reduce verbose

66438f2

support checksum

15f34a5

reduce verbose

2375e70

support checksum

9d2dbf1

cforce changed the title ~~checksum support , improve control with new cm line swicthes~~ checksum support , improve control with new cmd line swicthes Nov 10, 2024

cforce changed the title ~~checksum support , improve control with new cmd line swicthes~~ checksum support , improve control with new cmd line switches Nov 10, 2024

pamelafox reviewed Nov 11, 2024

View reviewed changes

Addressed some of my comments

4ef5622

pamelafox reviewed Nov 20, 2024

View reviewed changes

Make diff more minimal

7a1c07f

pamelafox reviewed Nov 20, 2024

View reviewed changes

Remove unused functions

0b43812

pamelafox reviewed Nov 20, 2024

View reviewed changes

Remove unneeded datetime module

d7f1c80

pamelafox reviewed Nov 20, 2024

View reviewed changes

	# upload the file local storage zu azure storage
	# upload the file from local storage to Azure storage

Uh oh!

Conversation

cforce commented Nov 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pamelafox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pamelafox commented Nov 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants