Skip to content
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

[FX] Dynamic Shapes Support #3225

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

anzr299
Copy link
Contributor

@anzr299 anzr299 commented Jan 29, 2025

Changes

Modify NNCF Graph Builder for FX backend to correctly get and insert the dynamic shapes into NNCFGraph

Reason for changes

To support quantization of Torch FX models exported with dynamic shapes

Tests

test is added to tests/torch/fx/test_models.py in test_quantized_models(). Currently only the synthetic transformer is tested because torch.export.dynamic_shapes.Dim.DYNAMIC is not supported in pytorch but is supported in upcoming releases. https://pytorch.org/tutorials/intermediate/torch_export_tutorial.html#constraints-dynamic-shapes

test_dynamic_edge() is also added in tests/torch/fx/test_models.py to check that the tensor shape in NNCF Graph edge has values only of type int or str and not SymInt.

@anzr299 anzr299 requested a review from a team as a code owner January 29, 2025 13:58
@github-actions github-actions bot added NNCF PT Pull requests that updates NNCF PyTorch experimental labels Jan 29, 2025
@@ -171,18 +176,29 @@ def test_model(test_case: ModelCase):
)


Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some test cases are duplicated: non synthetic transformer models are still being checked twice with the static shapes. Please refactor the test to prevent redundant cases, and please try to avoid pytest.skip calls as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you suggest I rather remove the graph tests and only retain test_dynamic_edge() test since most of the transformer models in this test are not correctly traced with dynamic shapes

@daniil-lyakhov
Copy link
Collaborator

NNCFGraphEdge and NNCF algorithms expect the edge shape to be a List[int] https://github.com/openvinotoolkit/nncf/blob/develop/nncf/common/graph/graph.py. The use of an str as a shape dimension could lead to unexpected errors in the algorithms: what would happened if a channel dim will be "s1" for a per-channel quantization?

Could you please show an example with a quantization of a dynamic model? Perhaps we can cover the case with less drastic change (using -1 as a shape dim, for example)

@anzr299
Copy link
Contributor Author

anzr299 commented Jan 30, 2025

NNCFGraphEdge and NNCF algorithms expect the edge shape to be a List[int] https://github.com/openvinotoolkit/nncf/blob/develop/nncf/common/graph/graph.py. The use of an str as a shape dimension could lead to unexpected errors in the algorithms: what would happened if a channel dim will be "s1" for a per-channel quantization?

Could you please show an example with a quantization of a dynamic model? Perhaps we can cover the case with less drastic change (using -1 as a shape dim, for example)

I can add an exception for the case where the in_channel has a non-static .
using integer such as -1 could cause more complex dynamic shape definition (for example s0//2*3+s1) in int to resolve to another value. Would you recommend simply catching the known cases? or to brainstorm on other ways to represent the dynamic shapes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental NNCF PT Pull requests that updates NNCF PyTorch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants