-
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
Make --serialized-file argument optional #994
Conversation
Looks good to me. Can you please confirm the behavior of Torchserve upon testing. |
@@ -67,6 +66,9 @@ def initialize(self, context): | |||
self.model = self._load_pickled_model(model_dir, model_file, model_pt_path) | |||
else: | |||
logger.debug("Loading torchscript model") | |||
if not os.path.isfile(model_pt_path): |
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.
Looks like presence / absence of --model-file arg in the manifest determines if this is an eager mode / torchscript model. Given usage of this in AWS Inf tolkit
model_archiver_cmd = [
"torch-model-archiver",
"--model-name",
DEFAULT_TS_MODEL_NAME,
"--handler",
handler_service,
"--serialized-file",
os.path.join(environment.model_dir, DEFAULT_TS_MODEL_SERIALIZED_FILE),
"--export-path",
DEFAULT_TS_MODEL_DIRECTORY,
"--extra-files",
os.path.join(environment.model_dir, DEFAULT_TS_CODE_DIR, environment.Environment().module_name + ".py"),
"--version",
"1",
]
Does AWS Inf toolkit even support Eager mode models ? Per documentation --model-file is a required arg for Eagermode models.
Should --model-file be passed ?
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.
toolkit missed option "--model-file " for eager mode. It should be fixed in toolkit.
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.
If customers have been using TS archiver without --model-file
arg, we should make sure this doesn't cause any backward compatibility issues
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.
On TS side, the only change for option "--serialized-file" is that it is optional in eager mode (instead of mandatory) . So it is backward compatible.
Description
Please include a summary of the feature or issue being fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #(issue)
#993
Type of change
Please delete options that are not relevant.
Feature/Issue validation/testing
Please describe the tests [UT/IT] that you ran to verify your changes and relevent result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.
archive:
torch-model-archiver --model-name densenet161 --version 1.0 --model-file examples/image_classifier/densenet_161/model.py --extra-files examples/image_classifier/index_to_name.json --handler image_classifier
torchserve --model-store /home/model-server/model-store --models densenet161.mar
curl http://127.0.0.1:8080/predictions/densenet161 -T image_classifier/kitten.jpg
{
"ashcan": 0.003682350507006049,
"maraca": 0.003106366842985153,
"groenendael": 0.0025914516299962997,
"reflex_camera": 0.002574876882135868,
"macaw": 0.0025670137256383896
}
UT/IT execution results
Logs
Checklist: