Fix ordering of value info in GraphProto creation #20691
Conversation
|
/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 successfully started running 9 pipeline(s). |
|
/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 |
|
Azure Pipelines successfully started running 10 pipeline(s). |
|
/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run Linux Android Emulator QNN CI Pipeline |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@skottmckay this is a similar issue as an issue discussed previously at #14631 (comment) |
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. |
@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 |
|
Azure Pipelines successfully started running 9 pipeline(s). |
|
/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 |
|
Azure Pipelines successfully started running 9 pipeline(s). |
|
/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 |
|
Azure Pipelines successfully started running 9 pipeline(s). |
|
/azp run ONNX Runtime React Native CI Pipeline, orttraining-amd-gpu-ci-pipeline, Linux Android Emulator QNN CI Pipeline |
|
Azure Pipelines successfully started running 10 pipeline(s). |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run Big Models |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run ONNX Runtime React Native CI Pipeline |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Graph member value_info_ (unordered_set) is ordered before its values are added to the graph proto.
Motivation and Context
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: #14631