onnxruntime
onnxruntime copied to clipboard
microsoft
Fix ordering of value info in GraphProto creation
Description
Graph member value_info_ (unordered_set) is ordered before its values are added to the graph proto.
Motivation and Context
- Without this ordering, the model proto used by the OpenVINO EP is not deterministic and varies across runs.
- Since the model proto varies, it affects caching attempts by OpenVINO.
Q: If creating a vector to have ordered elements is costly, should we make value_info_ a std::set that is sorted according to NodeArg names?
Related PR about ordering initializers: https://github.com/microsoft/onnxruntime/pull/14631
/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
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
/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, Windows x64 QNN CI Pipeline, Linux MIGraphX CI Pipeline, Big Models
/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline
@skottmckay this is a similar issue as an issue discussed previously at https://github.com/microsoft/onnxruntime/pull/14631#discussion_r1101731248 any objections to this?
@skottmckay this is a similar issue as an issue discussed previously at #14631 (comment) any objections to this?
I think it should be fine. Typical inferencing usage doesn't involve calling ToGraphProto so it will have limited effect elsewhere, so maybe not worth adding a param to ToGraphProto to control whether they are sorted or not
Please add comments to the code explaining why it is necessary to sort the values. Without context it seems like an unnecessary thing to do, and if I saw that my first temptation would be to delete it if the reason was not captured in a comment.
Please add comments to the code explaining why it is necessary to sort the values. Without context it seems like an unnecessary thing to do, and if I saw that my first temptation would be to delete it if the reason was not captured in a comment.
@skottmckay Added comment that clarifies why this sorting is necessary
/azp run Windows ARM64 QNN CI Pipeline, Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows x64 QNN CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed
/azp run Big Models, Linux Android Emulator QNN CI Pipeline, Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, Linux QNN CI Pipeline, MacOS CI Pipeline
/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
/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, Windows x64 QNN CI Pipeline, Linux MIGraphX CI Pipeline, Big Models
/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline, Linux Android Emulator QNN CI Pipeline