Update engine hash id generator with model name/model content/metadata #13015
Conversation
|
As we discussed in yesterday's meeting, please also add following changes:
It's also better to check the model with multiple partitions/engines, for example, FasterRCNN and MaskRCNN. |
| @@ -665,6 +665,10 @@ struct ProviderHost { | |||
| virtual bool Graph__GetInitializedTensor(const Graph* p, const std::string& tensor_name, const ONNX_NAMESPACE::TensorProto*& value) = 0; | |||
|
|
|||
| virtual const Node* Graph__ParentNode(const Graph* p) const = 0; | |||
| virtual const Graph* Graph__ParentGraph(const Graph* p) const = 0; | |||
| virtual const std::string& Graph__Name(const Graph* p) noexcept = 0; | |||
There was a problem hiding this comment.
I suggest make it:
virtual const std::string& Graph__Name(const Graph* p) const noexcept = 0;
to be aligned with ORT Graph API.
Same for:
virtual const std::vector<const NodeArg*>& Graph__GetInputsIncludingInitializers(const Graph* p) const noexcept = 0;
There was a problem hiding this comment.
Thanks for pointing that out. Those have been included in the 2nd commit
|
Can you address the cpplint warnings (if they make sense)? |
|
|
||
| // get the hash for the model when loaded from file | ||
| HashValue model_hash; | ||
| int id = TRTGenerateMetaDefId(viewer, model_hash); |
There was a problem hiding this comment.
That's good to test the correctness and functionality of TRTGenerateMetaDefId().
I think it's better to test the TRT EP inference run and check the hash file name of the engine cache in the filesystem as well, either manually or in the unit test. So that we can make sure the code path of TRT EP calling TRTGenerateMetaDefId() is okay.
There was a problem hiding this comment.
This has been manually verified by running FastRCNN locally. Thanks for sharing the instruction!
|
Do we've plans to test this in the Triton context? Here's the issue. |
What's the best way to verify the fix in theTriton context? I'm not familiar with the Triton testing, could they simply pick up the main branch and test? |
#13015) **Update engine hash id generator with model name/model content/metadata** **Description**: * Updated engine id generator, which use model name/model inputs & outputs/env metadata (instead of model path) to generate hash * New bridged API were introduced in order to enable id generator in the TRTEP utility **Motivation and Context** - Why is this change required? What problem does it solve? To fix this [issue](triton-inference-server/server#4587) caused by id generator using model path How to use: * Call [TRTGenerateMetaDefId(const GraphViewer& graph_viewer, HashValue& model_hash)](https://github.com/microsoft/onnxruntime/blob/0fcce74a565478b4c83fac5a3230e9786bb53ab3/onnxruntime/core/providers/tensorrt/tensorrt_execution_provider.cc#L715) to generate hash id for TRT engine cache How to test: * On WIndows, run: * .\onnxruntime_test_all.exe --gtest_filter=TensorrtExecutionProviderTest.TRTMetadefIdGeneratorUsingModelHashing * .\onnxruntime_test_all.exe --gtest_filter=TensorrtExecutionProviderTest.TRTSubgraphIdGeneratorUsingModelHashing **Appendix** * [Existing engine id generator that uses model path](https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/core/framework/execution_provider.cc#L112-L182)
There are some issues in #13015, 1. Model name should be used rather than graph name in the model ID generator. 2. Hash collision is observed in ID cache, which means different model may have the same key and thus load same hash id from the cache. 3. For the class and function that generate model id, MetaDef in the name is not appropriate. 4. Should reuse murmurhash3 rather than copy it over to TRT EP This PR fixes those issues.
There are some issues in microsoft#13015, 1. Model name should be used rather than graph name in the model ID generator. 2. Hash collision is observed in ID cache, which means different model may have the same key and thus load same hash id from the cache. 3. For the class and function that generate model id, MetaDef in the name is not appropriate. 4. Should reuse murmurhash3 rather than copy it over to TRT EP This PR fixes those issues.
There are some issues in #13015, 1. Model name should be used rather than graph name in the model ID generator. 2. Hash collision is observed in ID cache, which means different model may have the same key and thus load same hash id from the cache. 3. For the class and function that generate model id, MetaDef in the name is not appropriate. 4. Should reuse murmurhash3 rather than copy it over to TRT EP This PR fixes those issues.
Update engine hash id generator with model name/model content/metadata
Description:
Motivation and Context
How to use:
How to test:
Appendix