-
Notifications
You must be signed in to change notification settings - Fork 879
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
PyTorch profiler integration #1297
PyTorch profiler integration #1297
Conversation
Signed-off-by: Shrinath Suresh <shrinath@ideas2it.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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 good.
Signed-off-by: Shrinath Suresh <shrinath@ideas2it.com>
Signed-off-by: Shrinath Suresh <shrinath@ideas2it.com>
…resh/serve into profiler-integration
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -19,37 +19,8 @@ def teardown_module(module): | |||
test_utils.torchserve_cleanup() | |||
|
|||
|
|||
def model_archiver_command_builder(model_name=None, version=None, model_file=None, serialized_file=None, handler=None, extra_files=None, force=False): |
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.
moved this method to test_utils.py
to reuse
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.
Thanks @shrinath-suresh, it looks great. I think adding some notes to docs probably in configuration would be helpful.
self.profiler_args["record_shapes"] = True | ||
|
||
if "on_trace_ready" not in self.profiler_args: | ||
result_path = "/tmp/pytorch_profiler" |
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.
@shrinath-suresh I wonder if we give this option to user to choose the result_path and if not provided we fall back to default 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.
Also adding/extending one of the examples, like Resnet or BERT on using this profilers would be very helpful to show the usage.
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.
@HamidShojanazeri Sure Hamid.. I will pick any one example (mnist or resnet) to showcase profiler integration and update the readme accordingly. Thanks for the suggestion
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.
@HamidShojanazeri Regarding the profiler path.. By default the outputs get stored into "/tmp/pytorch_profiler". User can override this behaviour like this.
@HamidShojanazeri Do you have any suggestions for default path ? At base_handler.py
i couldn't find any default path (model store?) to store profiler trace files. /tmp/pytorch_profiler
looks hardcoded.. Any suggestion ?
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.
For the profiler path I'd suggest using the same path the pytorch profiler uses by default. If 2 succesive runs of a profile just create new directories in the /tmp/pytorch_profiler
then there is no need to overwrite but if they do then it's best to allow users to configure what the output path is
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.
I will pick any one example (mnist or resnet) to showcase profiler integration and update the readme accordingly. Thanks for the suggestion
let's have a separate PR on the example.
if not self._is_explain(): | ||
output = self.inference(data_preprocess) | ||
output = self.postprocess(output) | ||
is_profiler_enabled = os.environ.get("ENABLE_TORCH_PROFILER", None) |
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.
@shrinath-suresh I wonder if this could be added to the config.properties, this would help to make sure all the configs are gathered in one place. An example of adding to the config can be found here.
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.
@chauhang your thoughts on this ?
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.
I agree with Hamid, it's a simple change that'll make sure people don't need to think about both OS environment variables and config.properties. It's how we manage all configs. Let us know if you have any questions about how this would work
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.
Hi @shrinath-suresh any update on this? it's much cleaner for us to use config.properties
so users can discover all the available configuration options. That way it'll be much easier for people to discover this feature too
Thanks for your review comments. I will work on addressing them |
Signed-off-by: Shrinath Suresh <shrinath@ideas2it.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
export ENABLE_TORCH_PROFILER=true | ||
``` | ||
|
||
Note: Ensure`enable_envvars_config` is set to `true` in torchserve config |
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.
@shrinath-suresh is this gonna be added to the config.properties?
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.
I added a note here, since the env based setup doesnt work unless user sets enable_envvars_config
to true
in config.properties.
I will add config.properties
file in this example and update the instructions accordingly.
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.
@shrinath-suresh as mentioned in previous comment, you can add a setting to the config following this example. This basically would not work for now.
Install torch-tb-profiler and run the following command to view the results in UI | ||
|
||
``` | ||
tensorboard --logdir /tmp/pytorch_profiler/mnist/ |
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.
@shrinath-suresh in the handler below seems, "on_trace_ready" to override the path is missing.
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_trace_ready is not a mandatory argument. If user provides the argument, the trace files will get generated into the user specified location. if the user doesn't provide the argument, the trace files get generated inside "/tmp/pytorch_profiler/<model_name>" folder. Its handled here
For demonstration, we can add on_trace_ready in handler file. Again, we cannot set the on_trace_ready
to os.getcwd()
as it will be a temp directory in run time. May be we can try use home directory or something .. Your thoughts ?
Signed-off-by: Shrinath Suresh <shrinath@ideas2it.com>
Signed-off-by: Shrinath Suresh <shrinath@ideas2it.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
@shrinath-suresh please trying adding the config using the example shared in the comments.
export ENABLE_TORCH_PROFILER=true | ||
``` | ||
|
||
Note: Ensure`enable_envvars_config` is set to `true` in torchserve config |
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.
@shrinath-suresh as mentioned in previous comment, you can add a setting to the config following this example. This basically would not work for now.
Signed-off-by: Shrinath Suresh shrinath@ideas2it.com
Description
This pull request contains changes in base handler to integrate PyTorch profiler - https://pytorch.org/blog/introducing-pytorch-profiler-the-new-and-improved-performance-tool/ with torchserve
When user opts for the profiler, the preprocess/inference/postprocess calls are profiled and the traces are saved as a json file . User can load the trace json into pytorch profiler and view the detailed output.
This feature will help users to debug the issues in preprocess or inference.
the basis structure of the implementation is in place. More detailed requirements and unit tests to be captured. once it is done, WIP tag will be removed.
Initial Requirements:
Fixes #(issue)
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.
Test A
Test B
UT/IT execution results
Yet to be added
Checklist: