Skip to content

Add ONNX rewriter#82

Merged
mht-sharma merged 15 commits into
huggingface:mainfrom
Giuseppe5:onnx_rewriter
Mar 22, 2024
Merged

Add ONNX rewriter#82
mht-sharma merged 15 commits into
huggingface:mainfrom
Giuseppe5:onnx_rewriter

Conversation

@Giuseppe5

@Giuseppe5 Giuseppe5 commented Feb 22, 2024

Copy link
Copy Markdown
Contributor

Depends on #110

@Giuseppe5 Giuseppe5 changed the base branch from brevitas-compatibility to main March 8, 2024 13:18
@Giuseppe5 Giuseppe5 force-pushed the onnx_rewriter branch 2 times, most recently from 25f2f66 to 1c39f0a Compare March 14, 2024 16:34
@Giuseppe5 Giuseppe5 marked this pull request as ready for review March 14, 2024 16:36
@Giuseppe5 Giuseppe5 mentioned this pull request Mar 15, 2024
@Giuseppe5 Giuseppe5 force-pushed the onnx_rewriter branch 2 times, most recently from ebfa390 to 2cf0db0 Compare March 19, 2024 10:55
@mht-sharma mht-sharma mentioned this pull request Mar 19, 2024
5 tasks
@Giuseppe5 Giuseppe5 requested a review from mht-sharma March 19, 2024 13:31
Comment thread tests/brevitas/test_onnx_export.py Outdated
# The number of Matmul+Gemm has to be less compared to the model pre-transformation
# This is not zero since there are matmul that are not linear layers so they are not replaced
# and some linears layers can be excluded from quantization
assert matmul_gemm_counter <= original_matmul_gemm_counter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A better test would have us monitoring exactly how many Matmul and Gemm we expect to have before/after transformations, and similarly with matmulinteger.

Considering all the other changes that we are doing plus new tests that we will be adding, maybe we could wait for this kind of implementation when everything is more stable.

@Giuseppe5

Copy link
Copy Markdown
Contributor Author

It seems that onnx_tool has a bad typing which is making the tests to fail

@mht-sharma mht-sharma requested a review from fxmarty March 19, 2024 13:48
@mht-sharma

mht-sharma commented Mar 19, 2024

Copy link
Copy Markdown
Contributor

It seems that onnx_tool has a bad typing which is making the tests to fail

@Giuseppe5 for tests could you use python>=3.9. It should work with this

@fxmarty

fxmarty commented Mar 20, 2024

Copy link
Copy Markdown
Contributor

python 3.8 is still largely used (see https://pypistats.org/packages/transformers), although EOL is in a few months. We thus probably don't want to have python_requires=">3.9" in setup.py, but should raise a meaningful error if the onnx_tool has issues with python 3.8

@fxmarty fxmarty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work!

Comment thread optimum/amd/brevitas/export.py Outdated
Comment thread optimum/amd/brevitas/export.py Outdated
Comment thread optimum/amd/brevitas/export.py Outdated
Comment thread optimum/amd/brevitas/export.py Outdated
Comment thread optimum/amd/brevitas/export.py Outdated
Comment thread optimum/amd/brevitas/export.py Outdated
Comment thread optimum/amd/brevitas/export.py Outdated
Comment thread optimum/amd/brevitas/export.py Outdated
@Giuseppe5 Giuseppe5 requested a review from fxmarty March 20, 2024 10:03

@fxmarty fxmarty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@mht-sharma mht-sharma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! LGTM

Comment thread tests/brevitas/test_onnx_export.py Outdated
# The number of Matmul+Gemm has to be less compared to the model pre-transformation
# This is not zero since there are matmul that are not linear layers so they are not replaced
# and some linears layers can be excluded from quantization
assert matmul_gemm_counter <= original_matmul_gemm_counter

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert matmul_gemm_counter <= original_matmul_gemm_counter
self.assertTrue(matmul_gemm_counter <= original_matmul_gemm_counter)

@mht-sharma mht-sharma merged commit ca32e8e into huggingface:main Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants