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.

Sign up for GitHub

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

Jump to bottom

Added ignored RoPE block #3049

Merged
merged 6 commits into from
Nov 6, 2024
Merged

Added ignored RoPE block #3049

merged 6 commits into from
Nov 6, 2024

Conversation

KodiaqQ
Copy link
Collaborator

@KodiaqQ KodiaqQ commented Oct 31, 2024 •
edited
Loading

Changes

  • As stated in the title.
  • Added Sin and Cos OV operations.

Reason for changes

  • Accuracy improvement for models with ModelType.TRANSFORMERS type.

Related tickets

  • 155511

Tests

  • Added graph test for OV.

image

@github-actions github-actions bot added NNCF Common Pull request that updates NNCF Common NNCF OpenVINO Pull requests that updates NNCF OpenVINO labels Oct 31, 2024
@github-actions github-actions bot added NNCF PT Pull requests that updates NNCF PyTorch NNCF ONNX Pull requests that updates NNCF ONNX labels Oct 31, 2024
@KodiaqQ KodiaqQ marked this pull request as ready for review October 31, 2024 14:25
@KodiaqQ KodiaqQ requested a review from a team as a code owner October 31, 2024 14:25
@KodiaqQ
Copy link
Collaborator Author

KodiaqQ commented Oct 31, 2024

@openvinotoolkit/nncf-maintainers, can someone review this, please. Thank you.
All detailed other was placed in the ticket.

alexsu52
alexsu52 reviewed Nov 1, 2024
View reviewed changes
tests/torch/test_pattern_manager.py
@@ -74,6 +74,7 @@
IGNORING_IGNORED_PATTERN_REASONS = {
IgnoredPatternNames.FC_BN_HSWISH_ACTIVATION: "Not relevant for Torch.",
IgnoredPatternNames.EQUAL_LOGICALNOT: "Not relevant for Torch.",
IgnoredPatternNames.ROPE: "Not relevant for Torch.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this pattern is not relevant for other backends?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are no examples of this pattern in other backends. I can not add this for ONNX/PT since I have no models with this structure from these frameworks.

Copy link
Contributor

Choose a reason for hiding this comment

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

What models did you use for testing the RoPE pattern for OV backend? If source models are PyTorch models you can reuse them to specify RoPE pattern for ONNX/PT backends.

Copy link
Collaborator Author

@KodiaqQ KodiaqQ Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know who would need this, but OK.
I've set PR to draft until other backends are covered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added patterns with the corresponding tests as you asked. Need to properly review it, since I've had to create it without strong ONNX/PT research due to lack of time.

Copy link
Contributor

Choose a reason for hiding this comment

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

On which models have you tested the pattern?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For OV backend: phi-3, llama2-3.

@KodiaqQ KodiaqQ marked this pull request as draft November 1, 2024 17:48
@KodiaqQ KodiaqQ marked this pull request as ready for review November 4, 2024 15:45
@KodiaqQ KodiaqQ requested a review from alexsu52 November 4, 2024 15:45
alexsu52
alexsu52 reviewed Nov 5, 2024
View reviewed changes
nncf/onnx/quantization/ignored_patterns.py Outdated


@ONNX_IGNORED_PATTERNS.register(IgnoredPatternNames.ROPE)
def create_rope() -> GraphPattern:
Copy link
Contributor

@alexsu52 alexsu52 Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@kshpv, please review this pattern.

nncf/torch/quantization/ignored_patterns.py Outdated


@PT_IGNORED_PATTERNS.register(IgnoredPatternNames.ROPE)
def create_rope() -> GraphPattern:
Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexanderDokuchaev, please review this pattern.

tests/cross_fw/test_templates/helpers.py Outdated Show resolved Hide resolved
@MaximProshin
Copy link
Collaborator

@KodiaqQ , @alexsu52 , let's split it into 2 PRs: first for OV and second for other backends.

@KodiaqQ KodiaqQ mentioned this pull request Nov 5, 2024
Merged
@KodiaqQ
Copy link
Collaborator Author

KodiaqQ commented Nov 5, 2024

@KodiaqQ , @alexsu52 , let's split it into 2 PRs: first for OV and second for other backends.

#3059

@KodiaqQ KodiaqQ requested a review from alexsu52 November 5, 2024 09:58
@KodiaqQ KodiaqQ mentioned this pull request Nov 5, 2024
Merged
alexsu52
alexsu52 approved these changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@alexsu52 alexsu52 left a comment

Choose a reason for hiding this comment

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

LGTM

@alexsu52 alexsu52 merged commit 69d4412 into openvinotoolkit:develop Nov 6, 2024
14 checks passed
KodiaqQ added a commit that referenced this pull request Nov 7, 2024
@KodiaqQ
Added ignored RoPE for ONNX/PT (#3059)
80df08b 
On top of #3049

### Changes

- As stated in the title.
- Added Sin and Cos ONNX/PT operations.

### Reason for changes

- Accuracy improvement for models with ModelType.TRANSFORMERS type.

### Related tickets

- 155511

### Tests

- Added graph test for ONNX/PT.
KodiaqQ added a commit that referenced this pull request Nov 27, 2024
@KodiaqQ @AlexKoff88
FP8 example (#3062)
2db9fb9 
On top of #3049

### Changes

- Added FP8 example.

### Reason for changes

- Examples coverage.

### Related tickets

- 155923

### Tests

- ubuntu test_examples 627 - passed
- windows test-examples 288 - passed
- GA Test examples 135 - passed

---------

Co-authored-by: Alexander Kozlov <[email protected]>
daniil-lyakhov pushed a commit to daniil-lyakhov/nncf that referenced this pull request Dec 2, 2024
@KodiaqQ @AlexKoff88 @daniil-lyakhov
FP8 example (openvinotoolkit#3062)
6b37af0 
On top of openvinotoolkit#3049

### Changes

- Added FP8 example.

### Reason for changes

- Examples coverage.

### Related tickets

- 155923

### Tests

- ubuntu test_examples 627 - passed
- windows test-examples 288 - passed
- GA Test examples 135 - passed

---------

Co-authored-by: Alexander Kozlov <[email protected]>
daniil-lyakhov pushed a commit to daniil-lyakhov/nncf that referenced this pull request Dec 2, 2024
@KodiaqQ @AlexKoff88 @daniil-lyakhov
FP8 example (openvinotoolkit#3062)
adfb507 
On top of openvinotoolkit#3049

### Changes

- Added FP8 example.

### Reason for changes

- Examples coverage.

### Related tickets

- 155923

### Tests

- ubuntu test_examples 627 - passed
- windows test-examples 288 - passed
- GA Test examples 135 - passed

---------

Co-authored-by: Alexander Kozlov <[email protected]>
nikita-savelyevv pushed a commit to nikita-savelyevv/nncf that referenced this pull request Dec 11, 2024
@KodiaqQ @nikita-savelyevv
Added ignored RoPE for ONNX/PT (openvinotoolkit#3059)
7e24ece 
On top of openvinotoolkit#3049

### Changes

- As stated in the title.
- Added Sin and Cos ONNX/PT operations.

### Reason for changes

- Accuracy improvement for models with ModelType.TRANSFORMERS type.

### Related tickets

- 155511

### Tests

- Added graph test for ONNX/PT.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexsu52 alexsu52 alexsu52 approved these changes

Assignees
No one assigned
Labels
Code Freeze NNCF Common Pull request that updates NNCF Common NNCF ONNX Pull requests that updates NNCF ONNX NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF PT Pull requests that updates NNCF PyTorch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KodiaqQ @MaximProshin @alexsu52