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

Fix subgraph quantization regression in onnxruntime 1.17 #19421

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

fxmarty
Copy link
Contributor

@fxmarty fxmarty commented Feb 5, 2024

As per title, fixes #19418

ONNX Runtime 1.17 broke the quantization of ONNX models with subgraphs where initializers are placed on the top-level graph, while different subgraphs use the same initializer.

@fxmarty
Copy link
Contributor Author

fxmarty commented Feb 5, 2024

cc @tianleiwu @yufenglee @xadupre

@xadupre
Copy link
Member

xadupre commented Feb 5, 2024

Is it possible to add a unit test to make sure it does not break again?

@fxmarty
Copy link
Contributor Author

fxmarty commented Feb 5, 2024

Sure will do!

@fxmarty
Copy link
Contributor Author

fxmarty commented Feb 6, 2024

@xadupre I added a test.

@tianleiwu
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows ARM64 QNN CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, ONNX Runtime React Native CI Pipeline, Windows x64 QNN CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run Linux MIGraphX CI Pipeline, orttraining-amd-gpu-ci-pipeline

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@tianleiwu
Copy link
Contributor

@fxmarty, could you setup lintrunner and run lintrunner -a to format python script. See:
https://github.com/microsoft/onnxruntime/blob/main/docs/Coding_Conventions_and_Standards.md#linting

for attr in node.attribute:
if attr.type == onnx.AttributeProto.GRAPH:
for initializer in attr.g.initializer:
self.assertTrue("shared.weight" not in initializer.name)

Check notice

Code scanning / CodeQL

Imprecise assert Note

assertTrue(a not in b) cannot provide an informative message. Using assertNotIn(a, b) instead will give more informative messages.
@tianleiwu
Copy link
Contributor

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows ARM64 QNN CI Pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, ONNX Runtime React Native CI Pipeline, Windows x64 QNN CI Pipeline

@tianleiwu
Copy link
Contributor

/azp run Linux MIGraphX CI Pipeline, orttraining-amd-gpu-ci-pipeline, Big Models

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@sophies927 sophies927 removed the triage:approved Approved for cherrypicks for release label Feb 13, 2024
@tianleiwu tianleiwu merged commit 1e10cdb into microsoft:main Feb 13, 2024
76 of 78 checks passed
YUNQIUGUO pushed a commit that referenced this pull request Feb 14, 2024
As per title, fixes
#19418

ONNX Runtime 1.17 broke the quantization of ONNX models with subgraphs
where initializers are placed on the top-level graph, while different
subgraphs use the same initializer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ORT 1.17 regression] AttributeError: 'NoneType' object has no attribute 'HasField'
5 participants