Conversation
|
seems like we have a testing gap that should have caught this previously? |
| if (!model_path.IsEmpty()) { | ||
| // Get model name | ||
| PathString leaf = model_path.GetComponents().back(); | ||
| std::string model_name = ToUTF8String(leaf.c_str()); |
There was a problem hiding this comment.
If we hash model name here, will it be a case that a model has different model names and ends up with different hashed ID?
There was a problem hiding this comment.
yes. If user just changed model name and model itself doesn't change, ORT will treat it as a different model and generate a new engine.
|
|
||
| // Test loading same model in different way, when hash id is generated via model name/model content/env metadata | ||
| TEST(TensorrtExecutionProviderTest, TRTMetadefIdGeneratorUsingModelHashing) { | ||
| TEST(TensorrtExecutionProviderTest, TRTModelIdGeneratorUsingModelHashing) { |
There was a problem hiding this comment.
Hi Steven this test compares model hash when importing same model from file path/byte. Since model imported from byte has no path, model name can't be parsed from it and hash will be different from model imported from file. Will this be a valid use case?
There was a problem hiding this comment.
Yes, that's something we discussed internally. If user loads model bytes rather than onnx model, a new engine will be generated. I will update ORT document to reflect this.
Maybe there is a way to check hash id collision. Will think about it and add a test case in a separated PR |
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.
This reverts commit c4ecbb9.
4 participants
There are some issues in #13015,
This PR fixes those issues.