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

Merged
merged 19 commits into from
Mar 5, 2025

Conversation

anzr299
Copy link
Collaborator

@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
@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
Collaborator 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

@daniil-lyakhov
Copy link
Collaborator

@anzr299, please check a two ways of dynamic shapes handling:

@anzr299
Copy link
Collaborator Author

anzr299 commented Jan 31, 2025

  1. I have tried with -1 as dynamic shape. It seems to work fine and pass all tests. I also tested with a random constant shape for all the tensor shape values except for channel shape. For example, [1,3,224,224] -> [1000, 3, 1000, 1000] where the random constant is 1000. This also did not cause any issues for torch FX tests. I tested this for all the shapes in every model. So it looks like there is no issue with the tensor shapes.
  2. I am looking into ways to obtain the maximum value. I did find this which talks about how torch utilizes the hint value of symbolic data types. I think this could also be a good replacement for dynamic shape in nncf graph.

Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov left a comment

Choose a reason for hiding this comment

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

Can we reuse existing reference dot files? AFAIK they look identical

@daniil-lyakhov
Copy link
Collaborator

Test case have caught the issue with dynamic shape model here ynimmaga/executorch#26
@anzr299, please prioritize this fix

@@ -196,7 +196,9 @@ def get_edge_params(
else:
tensor = source_node.meta["val"]
if isinstance(tensor, torch.Tensor):
tensor_shape = tuple(tensor.shape)
tensor_shape = tuple(-1 if isinstance(i, torch.SymInt) else i for i in tensor.shape)
if isinstance(tensor, torch.SymInt):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if isinstance(tensor, torch.SymInt):
elif isinstance(tensor, torch.SymInt):

Co-authored-by: Alexander Dokuchaev <alexander.dokuchaev@intel.com>
@AlexanderDokuchaev AlexanderDokuchaev merged commit 77d2556 into openvinotoolkit:develop Mar 5, 2025
17 checks passed
shumaari pushed a commit to shumaari/nncf that referenced this pull request Mar 8, 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.

---------

Co-authored-by: Alexander Dokuchaev <alexander.dokuchaev@intel.com>
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