-
Notifications
You must be signed in to change notification settings - Fork 863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix interpretation of filename from model archive URL #2416
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
@@ -55,7 +54,7 @@ public static ModelArchive downloadModel( | |||
throw new ModelNotFoundException("empty url"); | |||
} | |||
|
|||
String marFileName = FilenameUtils.getName(url); | |||
String marFileName = ArchiveUtils.getFilenameFromUrl(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=signature
Interpreted 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=signature
Expected model archive filename from URL:
resnet-18.mar
This causes the download to fail.
Fixes #1293
Type of change
Feature/Issue validation/testing
Checklist: