Fix model download for ONNX embedder

[Josh-XT]

Description of changes

The current function is looking for the tar.gz file instead of checking if the folder already exists, so if the tar.gz gets deleted after extraction, it downloads it again.. This PR resolves this and checks for the model in the extracted folder before attempting to download or extract again.

Test plan

By using it

Documentation Changes

I didn't find any documentation about how this does the download.

[github-actions]

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

• Can you think of any use case in which the code does not behave as intended? Have they been tested?
• Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
• If appropriate, are there adequate property based tests?
• If appropriate, are there adequate unit tests?
• Should any logging, debugging, tracing information be added or removed?
• Are error messages user-friendly?
• Have all documentation changes needed been made?
• Have all non-obvious changes been commented?

System Compatibility

• Are there any potential impacts on other parts of the system or backward compatibility?
• Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

• Is this code of a unexpectedly high quality (Readbility, Modularity, Intuitiveness)

[Josh-XT]

For more context on the problem this solves:

In my software, I am using the ONNX embedder, but we don't want to make it download every time someone launches the container, so we changed the DOWNLOAD_PATH to our software folder and downloaded the tar.gz and extracted it in the folder in order to make it work. This PR will make it to where if you have either the tar.gz or the onnx ( or EXTRACTED_FOLDER_NAME ) folder in the DOWNLOAD_PATH location, it will continue on to initializing the model. Currently you need to have both for it to work properly.

[HammadB]

To check if the mode is correctly downloaded, can we check for the presence of all the files in the tar and validate their integrity?

config.json
model.onnx
special_tokens_map.json
tokenizer_config.json
tokenizer.json
vocab.txt

?

There can be situations where the extraction fails part way and then the model files will partially there leading to errors.

Additionally, can we store the md5 hash of each file and verify it is correct

So logic becomes

If all the files exist
- Check their hashes
- If OK: nothing
- If not: delete the partial files, proceed to next line
If all the files don't exist / partial
- Clean up partial files if any
- Check for archive, if no archive, download it.
- Extract all files, validate hash.

[Josh-XT]

Those sound like great ideas for another PR. This fix satisfies the need to download the model properly, additional features would make more sense in another PR, right?

[HammadB]

I don't think another PR 'makes more sense' - if you do not want to make those changes I am happy to make them instead.

This only half way fixes the problem, since a partial extract will still fail. I'd rather we simply address the problem properly. I'll submit a PR with the fix later this week.

[tazarov]

@HammadB , @Josh-XT , do you think it is reasonable to do the following steps in the respective order of "difficulty"

• Attempt to load the model from the expected path
• If the above fails, checksum the file and extract
• If the above fails, download the file, checksum, and extract it

[HammadB]

I think you have to checksum the individual files in addition to the tar because the extraction can be partial.

[tazarov]

I think you have to checksum the individual files in addition to the tar because the extraction can be partial.

As a more "brute-force" approach, one can simply checksum tarball and extract every time. (possibly remove any model dir that may exist).

[Josh-XT]

I just added an iteration for checking if each file exists in _download_model_if_not_exists. I didn't add anything for checksum checking. I wasn't saying that it should be another PR out of laziness, just that it isn't functionality that exists currently or would make this PR any more useful (in my opinion at least) to do the checksum part. As someone using this embedder, I'd personally prefer to not have to worry about that hash changing and me needing to scramble to update my software to accomodate changes made elsewhere.

[HammadB]

Thanks @Josh-XT this change makes sense to me incrementally. We can add checksumming as an extra step in a subsequent PR. Much appreciated!