Fix interpretation of filename from model archive URL #2416
Conversation
6553560 to
7b1141e
Compare
Codecov Report
@@ Coverage Diff @@
## master #2416 +/- ##
=======================================
Coverage 72.01% 72.01%
=======================================
Files 78 78
Lines 3648 3648
Branches 58 58
=======================================
Hits 2627 2627
Misses 1017 1017
Partials 4 4 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
@namannandan the pre-signed url is provided by user. According to #1293, the url does have "https://xxxx".
{
"code": 400,
"type": "DownloadArchiveException",
"message": "Failed to download archive from: https://log-analyzer-torchserve-mar.s3.amazonaws.com/test_service/stage/uw1/21_10_26_00_23/anomaly_detection_1635207765.7685282.mar?AWSAccessKeyId=****&Signature=****=&x-amz-security-token=****&Expires=1635283752"
}
Please provide end 2 end test result.
| } | ||
|
|
||
| String marFileName = FilenameUtils.getName(url); | ||
| String marFileName = ArchiveUtils.getFilenameFromUrl(url); |
There was a problem hiding this comment.
FilenameUtils.getName is used to extract the mar filename from the url. it is correct function. The original url is passed to function ArchiveUtils.downloadArchive.
The issue most likely happen in downloadArchive.
There was a problem hiding this comment.
Yes, this is correct, the call path is ModelArchive.downloadModel -> ArchiveUtils.downloadArchive -> HttpUtils.copyURLToFile -> FileUtils.copyURLToFile.
org.apache.commons.io.FileUtils.copyURLToFile throws IOException because of the destination we pass to it in the case of S3 presigned URL.
For example, in the current implementation, in case of S3 pre-signed URL, we call org.apache.commons.io.FileUtils.copyURLToFile with the following arguments:
source: https://test-account.s3.us-west-2.amazonaws.com/mar_files/resnet-18.mar?response-content-disposition=inline&X-Amz-Security-Token=token&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20230614T182131Z&X-Amz-SignedHeaders=host&X-Amz-Expires=43200&X-Amz-Credential=credential&X-Amz-Signature=signature
destination: resnet-18.mar?response-content-disposition=inline&X-Amz-Security-Token=token&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20230614T182131Z&X-Amz-SignedHeaders=host&X-Amz-Expires=43200&X-Amz-Credential=credential&X-Amz-Signature=signature
and this fails with IOException.
For testing, I hardcoded the destination in the HttpUtils.copyURLToFile to resnet-18.mar and the download succeeded.
Therefore, I've fixed the the implementation to correctly identify filename from model archive URL that contains additional parameters after the filename in ModelArchive.java which is eventually passed to org.apache.commons.io.FileUtils.copyURLToFile.
With the fix in this PR, the arguments are passed correctly:
source: https://test-account.s3.us-west-2.amazonaws.com/mar_files/resnet-18.mar?response-content-disposition=inline&X-Amz-Security-Token=token&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20230614T182131Z&X-Amz-SignedHeaders=host&X-Amz-Expires=43200&X-Amz-Credential=credential&X-Amz-Signature=signature
destination: resnet-18.mar
|
@lxning, here is the end to end test result With current implementation: With fix in this PR: |
Description
Current implementation incorrectly interprets the model archive filename from the S3 presigned URL as follows:
S3 presigned URL:
https://test-account.s3.us-west-2.amazonaws.com/mar_files/resnet-18.mar?response-content-disposition=inline&X-Amz-Security-Token=token&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20230614T182131Z&X-Amz-SignedHeaders=host&X-Amz-Expires=43200&X-Amz-Credential=credential&X-Amz-Signature=signatureInterpreted model archive filename from URL:
resnet-18.mar?response-content-disposition=inline&X-Amz-Security-Token=token&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20230614T182131Z&X-Amz-SignedHeaders=host&X-Amz-Expires=43200&X-Amz-Credential=credential&X-Amz-Signature=signatureExpected model archive filename from URL:
resnet-18.marThis causes the download to fail.
Fixes #1293
Type of change
Feature/Issue validation/testing
Checklist: