-
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
torch compile config standardization update #3166
Conversation
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.
LGTM, just left some nits
@@ -23,7 +23,7 @@ Ex: `cd examples/image_classifier/resnet_18` | |||
In this example , we use the following config | |||
|
|||
``` | |||
echo "pt2 : {backend: inductor, mode: reduce-overhead}" > model-config.yaml | |||
echo "pt2:\n compile:\n enable: True\n backend: inductor\n mode: reduce-overhead" > model-config.yaml |
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 you just use multiple lines people will be able to visualize the new format much better:
echo "pt2:
compile:
enable: True
backend: inductor
mode: reduce-overhead" > model-config.yaml
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.
Makes sense. Done
examples/pt2/torch_compile/README.md
Outdated
@@ -19,7 +19,7 @@ Ex: `cd examples/pt2/torch_compile` | |||
In this example , we use the following config | |||
|
|||
``` | |||
echo "pt2 : {backend: inductor, mode: reduce-overhead}" > model-config.yaml | |||
echo "pt2:\n compile:\n enable: True" > model-config.yaml |
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.
See above regarding multiline echo.
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.
Makes sense. Done
examples/pt2/torch_compile/README.md
Outdated
@@ -76,7 +76,7 @@ After a few iterations of warmup, we see the following | |||
#### Measure inference time with `torch.compile` | |||
|
|||
``` | |||
echo "pt2: {backend: inductor, mode: reduce-overhead}" > model-config.yaml && \ | |||
echo "pt2:\n compile:\n enable: True\n backend: inductor\n mode: reduce-overhead" > model-config.yaml && \ |
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.
ditto
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.
Makes sense. Done
test/pytest/test_torch_compile.py
Outdated
@@ -146,3 +176,90 @@ def test_serve_inference(self): | |||
"Compiled model with backend inductor, mode reduce-overhead" | |||
in model_log | |||
) | |||
|
|||
def test_compile_inference_enable_true_default(self, chdir_example): |
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.
You should parametrize these tests to make it more obvious what is the difference between them. See
@pytest.mark.parametrize(("compile"), ("false", "true")) |
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.
This is cool. Done
Description
This PR standardizes how TorchServe would be supporting any PyTorch 2.x APIs config
Define the following structure for a PyTorch 2.x feature config in
model-config.yaml
or
For
torch.compile
, we would specify the following optionsThis PR also fixes the issues with torch compile pytests
Fixes #(
issue1
issue2
issue3
)
Type of change
Please delete options that are not relevant.
Feature/Issue validation/testing
Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.
Checklist: